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

Add test for ACME #27683

Closed
wants to merge 2 commits into from
Closed

Add test for ACME #27683

wants to merge 2 commits into from

Conversation

aszlig
Copy link
Member

@aszlig aszlig commented Jul 27, 2017

Currently this is only a very basic test which gets certificates via the enableACME option of the nginx module.

However the main reason why I'm not directly merging and putting this up for review is that the complexity here lies in the support-modules needed for the test. The support modules are for running a Boulder instance along with a DNS resolver (as a separate module).

For details about the implementation, see the commit messages and the comments at the start of the respective support modules.

Cc: @qknight

These modules implement a way to test ACME based on a test instance of
Letsencrypt's Boulder service. The service implementation is in
letsencrypt.nix and the second module (resolver.nix) is a support-module
for the former, but can also be used for tests not involving ACME.

The second module provides a DNS server which hosts a root zone
containing all the zones and /etc/hosts entries (except loopback) in the
entire test network, so this can be very useful for other modules that
need DNS resolution.

Originally, I wrote these modules for the Headcounter deployment, but
I've refactored them a bit to be generally useful to NixOS users. The
original implementation can be found here:

https://github.com/headcounter/deployment/tree/89e7feafb/modules/testing

Quoting parts from the commit message of the initial implementation of
the Letsencrypt module in headcounter/deployment@95dfb31110397567534f2:

    This module is going to be used for tests where we need to
    impersonate an ACME service such as the one from Letsencrypt within
    VM tests, which is the reason why this module is a bit ugly (I only
    care if it's working not if it's beautiful).

    While the module isn't used anywhere, it will serve as a pluggable
    module for testing whether ACME works properly to fetch certificates
    and also as a replacement for our snakeoil certificate generator.

Also quoting parts of the commit where I have refactored the same module
in headcounter/deployment@85fa481:

    Now we have a fully pluggable module which automatically discovers
    in which network it's used via the nodes attribute.

    The test environment of Boulder used "dns-test-srv", which is a fake
    DNS server that's resolving almost everything to 127.0.0.1. On our
    setup this is not useful, so instead we're now running a local BIND
    name server which has a fake root zone and uses the mentioned node
    attribute to automatically discover other zones in the network of
    machines and generate delegations from the root zone to the
    respective zones with the primaryIPAddress of the node.

    ...

    We want to use real letsencrypt.org FQDNs here, so we can't get away
    with the snakeoil test certificates from the upstream project but
    now roll our own.

    This not only has the benefit that we can easily pass the snakeoil
    certificate to other nodes, but we can (and do) also use it for an
    nginx proxy that's now serving HTTPS for the Boulder web front end.

The Headcounter deployment tests are simulating a production scenario
with real IPs and nameservers so it won't need to rely on
networking.extraHost. However in this implementation we don't
necessarily want to do that, so I've added auto-discovery of
networking.extraHosts in the resolver module.

Another change here is that the letsencrypt module now falls back to
using a local resolver, the Headcounter implementation on the other hand
always required to add an extra test node which serves as a resolver.

I could have squashed both modules into the final ACME test, but that
would make it not very reusable, so that's the main reason why I put
these modules in tests/common.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
The test here is pretty basic and only tests nginx, but it should get us
started to write tests for different webservers and different ACME
implementations.

Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

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

Awesome work!

Can't we factor Boulder into a proper package and a NixOS service? Maybe not very general purpose for now but still -- putting everything into one test seems painful to me.

@aszlig
Copy link
Member Author

aszlig commented Jul 27, 2017

@abbradar: Probably a good idea, although I think we should factor the components out one by one. My original intent was also to make it a generic service, but it has a shitton of configuration options that we need to take care of. If we split it up into packages now, we'd also need tests for the boulder service itself, because then package bumps can break the service as well.

What this does at the moment is taking the test configs and patching them, so that they closely resemble what they run in production (although we'd still need to switch to unbound, because that's what they're running).

@abbradar
Copy link
Member

Hm, indeed we may not want to advertise boulder service (and leave it in tests instead) but shouldn't we at least factor out the packages? This way seems more proper and doesn't add much in terms of maintenance (think pkgs.mysql without services.mysql).

aszlig added a commit that referenced this pull request Sep 13, 2017
This is a rebased version of the pull request with small fixes due to
changes in recent master.

Original description from the pull request:

  Currently this is only a very basic test which gets certificates via
  the enableACME option of the nginx module.

  However the main reason why I'm not directly merging and putting this
  up for review is that the complexity here lies in the support-modules
  needed for the test. The support modules are for running a Boulder
  instance along with a DNS resolver (as a separate module).

  For details about the implementation, see the commit messages and the
  comments at the start of the respective support modules.

I'm merging this first of all because other than @abbradar, none of the
other requested reviewers did comment on the changes and second because
the change here is adding a test, so even if the implementation would be
so disgusting and crappy it's better than having no test at all.

The comment of @abbradar was:

  Can't we factor Boulder into a proper package and a NixOS service?
  Maybe not very general purpose for now but still -- putting everything
  into one test seems painful to me.

My objection to this is that the components are heavily patched and some
of them don't even have a release, so I'm not sure whether infesting
pkgs/ with them is really a good idea.

Nevertheless, we can still do that later.

Cc: @fpletz, @domenkozar, @bjornfor
@aszlig
Copy link
Member Author

aszlig commented Sep 13, 2017

Merged in 62711f4.

@aszlig aszlig closed this Sep 13, 2017
@aszlig aszlig deleted the acme-tests branch September 13, 2017 21:33
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

2 participants