Replace TLS-SNI-02 with TLS-ALPN-01. #112
Conversation
va/va.go
Outdated
| InsecureSkipVerify: true, | ||
| }) | ||
| func (va VAImpl) fetchCerts(hostPort string, config *tls.Config) ([]*x509.Certificate, *acme.ProblemDetails) { | ||
| config = config.Clone() |
There was a problem hiding this comment.
Given the config isn't being used anywhere else concurrently I think it's probably fine to not clone it and just work directly with the one passed in.
va/va.go
Outdated
| func (va VAImpl) fetchCerts(hostPort string, config *tls.Config) ([]*x509.Certificate, *acme.ProblemDetails) { | ||
| config = config.Clone() | ||
| config.InsecureSkipVerify = true | ||
| conn, err := tls.DialWithDialer(&net.Dialer{Timeout: time.Second * 5}, "tcp", hostPort, config) |
There was a problem hiding this comment.
I haven't tested this but from reading the crypto/tls docs for tls.ConnectionState I'm not sure this will enforce requiring that the negotiated protocol was one of those in config.NextProtos.
I think that conn.ConnectionState().NegotiatedProtocol still needs to be tested (and/or conn.ConnectionState().NegotiatedProtocolIsMutal) to be sure that the correct protocol is being used.
There was a problem hiding this comment.
Perhaps fetchCerts should actually be fetchConnectionState and the callers can then use the tls.ConnectionState to for what they want (although I guess now we don't have TLS-SNI this will be the only caller ¯_(ツ)_/¯).
va/va.go
Outdated
| return acme.MalformedProblem(msg) | ||
| if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != acme.ACMETLS1Protocol { | ||
| result.Error = acme.UnauthorizedProblem(fmt.Sprintf( | ||
| "Cannot connect using %s for %s challenge", |
There was a problem hiding this comment.
I think this error message should include the acronym "ALPN" somewhere, maybe: "Cannot mutually negotiate TLS ALPN %q for %s challenge" ?
va/va.go
Outdated
|
|
||
| if !validSanAName || !validSanBName { | ||
| // Verify SNI |
There was a problem hiding this comment.
Can you expand on this comment? I think it would be worth highlighting that you're using task.Identifier here and the importance of this check with respect to acme-tls/1 and the required properties of the challenge type.
va/va.go
Outdated
|
|
||
| if !validSanAName || !validSanBName { | ||
| // Verify SNI | ||
| if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], task.Identifier) { |
There was a problem hiding this comment.
Is strings.EqualFold too liberal for this purpose? I'm not 100% certain that case folding is required/wanted here. Can you expand on why you chose this instead of a direct comparison?
There was a problem hiding this comment.
SANs are case insensitive by rfcs, I must have seen this in some other code (though I can't find EqualFold anywhere in boulder now).
[0] https://security.stackexchange.com/questions/150770/are-certificate-sans-case-sensitive?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa
[1] https://github.com/globalsign/certlint/blob/033c8b6ba713114a228cd06b0b6a745cbde3efbe/checks/certificate/subjectaltname/subjectaltname.go#L79
va/va.go
Outdated
| if subtle.ConstantTimeCompare(h[:], ext.Value) == 1 { | ||
| return result | ||
| } | ||
| result.Error = acme.UnauthorizedProblem("Extension acmeValidationV1 value incorrect.") |
There was a problem hiding this comment.
I think it might be clearer if this error string matched the format of the errText above, e.g: "Incorrect validation certificate for %s challenge. Missing expected acmeValidation1 extension value"
There was a problem hiding this comment.
In fact there is a serious bug - we should return an error if the extension is missing.
|
@rolandshoemaker Can you give this another 🔍 ? |
This pull request contains a simple TLS-ALPN-01 implementation, as described in draft [0]
[0] https://github.com/rolandshoemaker/acme-tls-alpn