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

Implement TLS-ALPN-01 challenge and standalone TLS-ALPN server #5894

Merged
merged 7 commits into from Jun 4, 2018

Conversation

Projects
None yet
4 participants
@mdebski
Copy link
Contributor

commented Apr 23, 2018

The new challenge is described in https://github.com/rolandshoemaker/acme-tls-alpn.

@bmw

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

Thanks for the PR @mdebski.

We'll try and fully review this in the next couple weeks, but it looks like some tests are failing when trying to use older versions of our dependencies. We unfortunately have to keep support for some pretty old packages to allow for distro packaging in places like EPEL. In particular, we need to support pyOpenSSL as old as 0.13.1. The full list of these versions can be found in tools/oldest_constraints.txt.

As for how to accomplish this, we don't plan on using the TLS-ALPN challenge in Certbot anytime soon and I doubt other users of our ACME library are going to either. With this in mind, I think it's OK if you make the implementation of the TLS-ALPN-01 challenge pretty bare bones here. Implementing functionality similar to the other challenge types would be nice, but assuming this is pretty hard with the ancient version of pyOpenSSL, I think just adding the basic types so the library can parse the challenge type without error is good enough.

@bmw

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

Another option would be to put the code that tries to perform/verify the TLS-ALPN challenge behind a check that PyOpenSSL is a new enough version and/or has the features we need. If the check fails, you can raise an exception.

@mdebski

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

Ok, I'll try to make it work with older pyOpenssl. I prefer the second solution, as the feature that needs it is in fact in the core of tls-alpn challenge - it's for generating the challenge cert with acme extension.

Is cryptography>=1.0 ok?

@bmw

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

Thanks @mdebski. Bumping the acme requirement to cryptography>=1.0 is fine.

@mdebski

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

I tried working around this, but unfortunately it failed:
https://travis-ci.org/certbot/certbot/jobs/371503303#L744
for the very same reason I decided to go with cryptography.x509 instead of openssl.x509. I don't think I fully understand why is this failing (and how does it depend on versions of everything), I need to dig deeper into that.

Anyway, how should I implement your proposal to fail alpn on old pyopenssl? Should I just make sure everything that worked before still works, and possibly raise an exception in the new stuff? What about tests? Make alpn tests conditional on version, and simply do not run them on older?

@bmw

This comment has been minimized.

Copy link
Member

commented Apr 30, 2018

I don't think I fully understand why is this failing (and how does it depend on versions of everything)

I think what's going on is cryptography didn't support making certs with custom extensions until cryptography 1.3. This isn't documented in their changelog, but this seems to be suggested by pyca/cryptography#2747.

I'd like to avoid bumping our cryptography requirement higher than 1.2.3 though. This is the version in Ubuntu Xenial and we've been working to get a new version of Certbot shipped there for a long time now. Getting an update to a crypto library used by many other projects is a much harder sell than just updating Certbot itself.

Anyway, how should I implement your proposal to fail alpn on old pyopenssl? Should I just make sure everything that worked before still works, and possibly raise an exception in the new stuff?

Pretty much. I haven't dug into it so it might already work this way, but we shouldn't raise an exception if the CA includes a TLS-SNI-01 challenge in the authorization object regardless of the version of our dependencies. Instead, I'd like us to only raise an exception when trying to perform/verify the challenge.

What about tests? Make alpn tests conditional on version, and simply do not run them on older?

That works. If you're feeling ambitious, you could also add tests that mock out the libraries so the code is still tested when run with the older libraries. I don't think that's totally necessary though.

@mdebski mdebski force-pushed the mdebski:tlsalpn branch 2 times, most recently from 68ce017 to 3e2e255 May 15, 2018

@mdebski

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

Hi,

I've managed to work around missing support for custom extensions, and in fact only use pyopenssl for that. For the other problematic issue - alpn support - I just disabled the test if required feature in pyopenssl is missing.

Travis should be all green, I'd say this is fully ready now.

@mdebski

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

@bmw ^

@mdebski

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2018

The single failure (timeout) in travis seems to be flakiness, it worked fine a commit before. Could someone with write access retry it?

@bmw

This comment has been minimized.

Copy link
Member

commented May 17, 2018

The single failure (timeout) in travis seems to be flakiness, it worked fine a commit before. Could someone with write access retry it?

Done. Tests passed this time.

We'll try and give this a full review soon.

@bmw

This comment has been minimized.

Copy link
Member

commented May 23, 2018

@sydneyli, this is a larger PR, but if you think you would have time to get to it in the next couple days, taking it would help me out. If not, I can take it.

If you decide to review it, please take care when reviewing the public interfaces added here (which we have to live with for the foreseeable future) and that we handle TLS-ALPN when run with old versions of our dependencies gracefully. You can see the discussion above about the latter.

@Yoni116

This comment has been minimized.

Copy link

commented May 24, 2018

@bmw small question - will this implementation replaces the old tls challenge that was removed and allowed to do verification with using only port 443?

@mdebski

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2018

tls-alpn is designed to be a replacement for tls-sni. This PR is not supposed to modify any behaviours around tls-sni in certbot though.

As for 443 port - I believe no changes to that, this is still constrained to 443.

@sydneyli sydneyli self-requested a review May 26, 2018

@sydneyli
Copy link
Member

left a comment

@mdebski, thanks for the PR, this looks great! Left a couple of notes, all small things :)

@bmw, I'm not familiar with crypto_utils and would like a second look at that as well as the workaround mentioned here -- would you mind taking over here?

"""

IdPeAcmeIdentifierV1 = b"1.3.6.1.5.5.7.1.30.1"
ACMETLS1Protocol = "acme-tls/1"

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 29, 2018

Member

nit: prefer CAPITAL_SNAKE_CASE for naming class constants

:param unicode domain:
"""

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 29, 2018

Member

nit: I noticed an old comment in TLSSNI01Response.probe_cert that mentioned domain might be optional if a host IP were provided. While this is true for sni validation, comment here to make it explicit that domain is necessary for alpn validation?

names = crypto_util._pyopenssl_cert_or_req_all_names(cert)
logger.debug('Certificate %s. SANs: %s', cert.digest('sha256'), names)
if domain not in names:
return False

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 29, 2018

Member

The check here seems weaker than the one described in the rfc draft-- should domain be the only SAN present?

Verify ... that the certificate returned contains a subjectAltName extension containing the dNSName being validated and no other entries

This comment has been minimized.

Copy link
@mdebski

mdebski May 30, 2018

Author Contributor

Right, thanks! I also added case folding to mirror boulder impl:
https://github.com/letsencrypt/boulder/pull/3654/files#diff-4c51d1d7ca3ec3022d14b42809af0d7eR719

class TLSALPN01Server(TLSServer, ACMEServerMixin):
"""TLSALPN01 Server."""

ACMETLS1Protocol = b"acme-tls/1"

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 29, 2018

Member

nit: SNAKE_CASE (same as above)

verified.
:rtype: bool
"""

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 29, 2018

Member

My understanding is that the verification here is technically incomplete, since we don't check that the acme-tls/1 protocol was successfully negotiated. This is possibly O.K. since simple_verify is really only consumed by our own tests as a sanity check, and therefore not meant to be a full validation (see DNS01Response.simple_verify).

So, we could either do a full validation here (though this might involve a lot more plumbing) or leave it as-is, and explicitly document its incompleteness here.

This comment has been minimized.

Copy link
@mdebski

mdebski May 30, 2018

Author Contributor

OpenSSL will terminate connection on failure to terminate connection, so probe_sni will raise an exception.

:param OpenSSL.crypto.X509 cert: Optional certificate. If not
provided (``None``) certificate will be retrieved using
`probe_cert`.
:param int port: Port used to probe the certificate.

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 29, 2018

Member

the interface matches TLS-SNI-01's exactly, which is nice :) however, kwargs is a bit clunky, and since we really only expect host or port to be passed in, can we limit the set of arguments from * to just those two? Same goes for probe_cert.

@bmw
Copy link
Member

left a comment

Other than my comment about changing the parameters to SSLSocket and a couple comments about docstrings to make Sphinx happy, crypto_util looks good to me.

The TODO Sydney asked me to take a look at also seems fine to me. Unless I'm missing something, I think we're doing the best we can with our libraries available to us.

@@ -39,9 +39,11 @@ class SSLSocket(object): # pylint: disable=too-few-public-methods
:ivar method: See `OpenSSL.SSL.Context` for allowed values.
"""
def __init__(self, sock, certs, method=_DEFAULT_TLSSNI01_SSL_METHOD):
def __init__(self, sock, cert_selection, alpn_selection=None,

This comment has been minimized.

Copy link
@bmw

bmw May 30, 2018

Member

I appreciate you setting up the plumbing here so we can only serve the cert when it's requested using ALPN when openssl and pyopenssl support it and while this change is probably OK, we've been avoiding making breaking changes in acme since we entered beta in 2015. With this in mind, I'm thinking we should revert the cert -> cert_selection changes here for now and we can revisit this if desired when this feature is added to our dependencies.

Does this seem reasonable to you?

This comment has been minimized.

Copy link
@mdebski

mdebski May 30, 2018

Author Contributor

You're perfectly right about breaking changes. I think it's reasonable though to provide this callback feature, so I just refactored it to not break anything. I also changed the order so that the added parameters are the last ones (in case sb specified them as positional).

Is that ok for you?

This comment has been minimized.

Copy link
@bmw

bmw May 30, 2018

Member

That works for me! It certainly makes it easier for us to do cert selection based on ALPN when it's an option for us.

Thanks for catching the parameter order and moving the new ones to the end.

Only suggested change with this approach is maybe we should make certs an optional parameter now with a default value of None as it makes the interface a little nicer when you want to use cert_selection. If we did this, we should also check that one of certs or cert_selection is set. You're welcome to ignore this suggestion if you prefer.

This comment has been minimized.

Copy link
@mdebski

mdebski May 31, 2018

Author Contributor

done

@@ -123,6 +129,7 @@ def probe_sni(name, host, port=443, timeout=300,
:param tuple source_address: Enables multi-path probing (selection
of source interface). See `socket.creation_connection` for more
info. Available only in Python 2.7+.
:param list bytes alpn_protocols: Protocols to request using ALPN.

This comment has been minimized.

Copy link
@bmw

bmw May 30, 2018

Member

Syntax for this should be:

:param alpn_protocols: Protocols to request using ALPN.
:type alpn_protocols: `list` of `bytes`
"""Generate new self-signed certificate.
:type domains: `list` of `unicode`
:param OpenSSL.crypto.PKey key:
:param bool force_san:
:param extensions `list` of `OpenSSL.crypto.X509Extension`:

This comment has been minimized.

Copy link
@bmw

bmw May 30, 2018

Member

Syntax here should be:

:param extensions: <short description if desired>
:type extensions: `list` of `OpenSSL.crypto.X509Extension`

@mdebski mdebski force-pushed the mdebski:tlsalpn branch from a68ea39 to 5df1130 May 30, 2018

@mdebski mdebski force-pushed the mdebski:tlsalpn branch from 5df1130 to 2e8784a May 30, 2018

@mdebski

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@bmw , @sydneyli : done

@bmw bmw dismissed their stale review May 30, 2018

I have one suggestion which you can implement (or not) if you want at #5894 (comment), but otherwise, the pieces Sydney asked me to look at LGTM.

def verify_cert(self, domain, cert):
"""Verify tls-alpn-01 challenge certificate.
:param OpensSSL.crypto.X509 cert: Challenge certificate.

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 30, 2018

Member

nit: document domain

@sydneyli
Copy link
Member

left a comment

some small documentation/naming things, but otherwise lgtm 😄

def probe_cert(self, domain, host=None, port=None):
"""Probe tls-alpn-01 challenge certificate.
:param unicode domain: domain being validated, required.

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 30, 2018

Member

nit: document host and port

def cert_selection(self, connection):
"""Callback selecting certificate for connection."""
server_name = connection.get_servername()
return self.certs.get(server_name, None)

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 30, 2018

Member

Let's prefix cert_selection and alpn_selection with _ to indicate they're for internal use only?

This comment has been minimized.

Copy link
@mdebski

mdebski May 31, 2018

Author Contributor

I don't think they should be for internal use only.

This comment has been minimized.

Copy link
@sydneyli

sydneyli May 31, 2018

Member

how do you mean? the selection routines can be overriden by subclasses, but what use might someone have for consuming them in the API for TLSServer?

This comment has been minimized.

Copy link
@mdebski

mdebski Jun 4, 2018

Author Contributor

Oh, right, they definitely should be private. I was reading this on a phone and misread the context.

mdebski added some commits May 31, 2018

@sydneyli
Copy link
Member

left a comment

LGTM!

@bmw bmw merged commit 15f1405 into certbot:master Jun 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 98.724%
Details

@bmw bmw added this to the 0.25.0 milestone Jun 6, 2018

bmw added a commit that referenced this pull request Jun 13, 2018

bmw added a commit that referenced this pull request Jun 13, 2018

bmw added a commit that referenced this pull request Jun 22, 2018

Partially revert "Implement TLS-ALPN-01 challenge and standalone TLS-…
…ALPN server (#5894)"

This partially reverts commit 15f1405.

A basic tls-alpn-01 implementation is left so we can successfully parse the
challenge so it can be used in boulder's tests.

bmw added a commit that referenced this pull request Jun 26, 2018

Partially revert "Implement TLS-ALPN-01 challenge and standalone TLS-…
…ALPN server (#5894)"

This partially reverts commit 15f1405.

A basic tls-alpn-01 implementation is left so we can successfully parse the
challenge so it can be used in boulder's tests.

bmw added a commit that referenced this pull request Jun 26, 2018

Partially revert "Implement TLS-ALPN-01 challenge and standalone TLS-…
…ALPN server (#5894)"

This partially reverts commit 15f1405.

A basic tls-alpn-01 implementation is left so we can successfully parse the
challenge so it can be used in boulder's tests.

bmw added a commit that referenced this pull request Jun 26, 2018

bmw added a commit that referenced this pull request Jun 26, 2018

Partially revert "Implement TLS-ALPN-01 challenge and standalone TLS-…
…ALPN server (#5894)" (#6144)

This partially reverts commit 15f1405.

A basic tls-alpn-01 implementation is left so we can successfully parse the
challenge so it can be used in boulder's tests.

adferrand added a commit to adferrand/certbot that referenced this pull request Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.