Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/httpd: support overridable virtual hosts #73113

Merged
merged 1 commit into from Dec 26, 2019

Conversation

aanderse
Copy link
Member

@aanderse aanderse commented Nov 9, 2019

I have split this PR into multiple commits to make it easier to review. Ignoring white space also helps in a few places. Before merging all commits should be squashed as the commits individually break various things.

Motivation for this change
  • the virtualHosts option is of type listOf instead of attrsOf which doesn't allow users to modify a virtualHost once created
  • each virtualHost defined is specific to either http or https which makes it extremely difficult to implement integration with security.acme.certs
  • httpd module code is overly complex
  • httpd module has fallen far behind the nginx module and as a result isn't used by many people on nixos
Things not yet done
  • merge a PR to rewrite of the awstats module, with offer to do so generously provided by @aristaeus
  • rewrite examples in the nixos manual under the abstractions section of the manual which reference services.httpd, something which @samueldr may have some ideas on
  • write release notes
  • squash all commits into a single before merging this PR
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@aanderse
Copy link
Member Author

aanderse commented Nov 9, 2019

Example of current test configuration:

{
  imports = [ /home/aaron/nixpkgs/nixos/modules/services/web-servers/apache-httpd/default.nix ];
  disabledModules = [
    "services/logging/awstats.nix"
    "services/web-servers/apache-httpd/default.nix"
  ];

  services.httpd = {
    enable = true;
    enablePHP = true;
    adminAddr = "webmaster@example.org";
    logPerVirtualHost = true;
    virtualHosts = {
      "test1.example.org" = {
        documentRoot = "/var/www/test1.example.org";
        addSSL = true;
        enableACME = true;
      };
      "test2.example.org" = {
        documentRoot = "/var/www/test2.example.org";
        forceSSL = true;
        useACMEHost = "test2.example.org";
      };
    };
  };
  
  security.acme.certs."test2.example.org" = {
    user = "wwwrun";
    group = "wwwrun";
    webroot = "/var/lib/acme/acme-challenges";
  };
}

@aanderse
Copy link
Member Author

aanderse commented Nov 9, 2019

ping @redvers, @FPtje, and @Izorkin as users of the httpd module. Any review, testing, and feedback is appreciated.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/what-are-your-goals-for-20-03/4773/3

@aanderse
Copy link
Member Author

aanderse commented Dec 6, 2019

cc @vanyaklimenko in case they have interest in testing this PR.

@aanderse
Copy link
Member Author

@GrahamcOfBorg test haproxy proxy upnp

@aanderse
Copy link
Member Author

@GrahamcOfBorg test limesurvey mediawiki
@GrahamcOfBorg test moodle wordpress

Copy link
Member

@ivan ivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied this to nixpkgs master, converted my configuration, and merged my http:// and https:// virtualhosts into a single virtualhost. That broke https:// because I had

        listen = [
          { ip = "1.2.3.4"; port = 80; }
          { ip = "1.2.3.4"; port = 443; }
        ];

which caused Apache to run HTTP without SSL on port 443.

From the implementation in nginx, I correctly guessed that I could do this here too:

        listen = [
          { ip = "1.2.3.4"; port = 80; }
          { ip = "1.2.3.4"; port = 443; ssl = true; }
        ];

but this is perplexingly undocumented in apache-httpd/per-server-options.nix. (Yes, it really does work, but I cannot figure out why.)

@ivan
Copy link
Member

ivan commented Dec 18, 2019

Sorry! I was looking at the wrong branch locally and see that it is documented.

Copy link
Member

@ivan ivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrating my configuration wasn't very painful and the migrated configuration appears to behave correctly.

I think this should be merged once someone else looks over it, or confirms that those six Apache-using modules in nixpkgs still work.

@aanderse aanderse marked this pull request as ready for review December 18, 2019 11:02
@aanderse aanderse requested a review from nbp as a code owner December 18, 2019 11:02
@aanderse
Copy link
Member Author

@ivan I just need to test awstats PR and then merge it, and get a few more people testing this then I'm satisfied. At this point though I'm looking for review and testing, so thank you very much!

…ualHosts option type from listOf to attrsOf, add ACME integration
@aanderse
Copy link
Member Author

@GrahamcOfBorg test limesurvey mediawiki
@GrahamcOfBorg test moodle wordpress
@GrahamcOfBorg test haproxy hitch proxy upnp

@aanderse
Copy link
Member Author

I've been running this on 3 machines for a while now, @ivan has given it a look over and migrated their configuration to use it, all outstanding issues/requirements addressed... so merging.

@aanderse aanderse merged commit 4d2dd15 into NixOS:master Dec 26, 2019
@aanderse aanderse deleted the httpd-vhost branch December 26, 2019 13:09
@dasJ dasJ mentioned this pull request Dec 29, 2019
16 tasks
@aanderse aanderse mentioned this pull request Dec 30, 2019
10 tasks
jtojnar added a commit to jtojnar/nixfiles that referenced this pull request Feb 5, 2020
The rewritten httpd module (NixOS/nixpkgs#73113) puts `AllowOverride None` into <Directory> block of each virtual host. Since the virtual host blocks are located after top level `extraConfig`, it takes precedence over `AllowOverride` set there. In order for `.htaccess` to work, we need to move that to virtual hosts’ `extraConfig`.
@immae
Copy link
Contributor

immae commented Apr 3, 2020

@aanderse
I’m a little surprised by the invasive enforced

        <Directory "${documentRoot}">
            Options Indexes FollowSymLinks
            AllowOverride None
            ${allGranted}
        </Directory> 

part of a vhost, is there no more way to define a "private" vhost? (like one where we want authenticated users only)

EDIT: seems like it’s not recent (I just never noticed it before) and it can be overriden by a subsequent Directory directive, so I guess it’s nothing new there..

@aanderse
Copy link
Member Author

aanderse commented Apr 4, 2020

@immae your edit sums up the situation nicely. Not something I put in place, and easily overridden if you read through apache documentation to understand how merging happens.

That being said... the situation in this module is less than perfect. I have spent some time trying to work through a solution to this but unfortunately ran into some roadblocks. I'm hoping to come up with a solution to improve things at some point in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants