From f866b1bb0058e0ae4626698f29cf219b2ff5d87a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20D=C4=99bski?= Date: Mon, 9 Apr 2018 09:54:26 +0000 Subject: [PATCH 1/3] Replace TLS-SNI-02 with TLS-ALPN-01. --- acme/common.go | 8 ++-- va/va.go | 103 ++++++++++++++++++------------------------------- wfe/wfe.go | 2 +- 3 files changed, 44 insertions(+), 69 deletions(-) diff --git a/acme/common.go b/acme/common.go index 42210104..70e4e81f 100644 --- a/acme/common.go +++ b/acme/common.go @@ -11,11 +11,13 @@ const ( IdentifierDNS = "dns" - ChallengeHTTP01 = "http-01" - ChallengeTLSSNI02 = "tls-sni-02" - ChallengeDNS01 = "dns-01" + ChallengeHTTP01 = "http-01" + ChallengeTLSALPN01 = "tls-alpn-01" + ChallengeDNS01 = "dns-01" HTTP01BaseURL = ".well-known/acme-challenge/" + + ACMETLS1Protocol = "acme-tls/1" ) type Identifier struct { diff --git a/va/va.go b/va/va.go index bf5558b4..265690c5 100644 --- a/va/va.go +++ b/va/va.go @@ -5,8 +5,8 @@ import ( "crypto/subtle" "crypto/tls" "crypto/x509" + "encoding/asn1" "encoding/base64" - "encoding/hex" "fmt" "io/ioutil" "log" @@ -46,6 +46,8 @@ const ( noSleepEnvVar = "PEBBLE_VA_NOSLEEP" ) +var IdPeAcmeIdentifierV1 = asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 30, 1} + func userAgent() string { return fmt.Sprintf( "%s (%s; %s)", @@ -198,8 +200,8 @@ func (va VAImpl) performValidation(task *vaTask, results chan<- *core.Validation switch task.Challenge.Type { case acme.ChallengeHTTP01: results <- va.validateHTTP01(task) - case acme.ChallengeTLSSNI02: - results <- va.validateTLSSNI02(task) + case acme.ChallengeTLSALPN01: + results <- va.validateTLSALPN01(task) case acme.ChallengeDNS01: results <- va.validateDNS01(task) default: @@ -245,7 +247,7 @@ func (va VAImpl) validateDNS01(task *vaTask) *core.ValidationRecord { return result } -func (va VAImpl) validateTLSSNI02(task *vaTask) *core.ValidationRecord { +func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { portString := strconv.Itoa(va.tlsPort) hostPort := net.JoinHostPort(task.Identifier, portString) @@ -254,83 +256,54 @@ func (va VAImpl) validateTLSSNI02(task *vaTask) *core.ValidationRecord { ValidatedAt: va.clk.Now(), } - const tlsSNITokenID = "token" - const tlsSNIKaID = "ka" - const tlsSNISuffix = "acme.invalid" - - // Lock the challenge for reading while we validate - task.Challenge.RLock() - defer task.Challenge.RUnlock() - - // Compute the digest for the SAN b that will appear in the certificate - ha := sha256.Sum256([]byte(task.Challenge.Token)) - za := hex.EncodeToString(ha[:]) - sanAName := fmt.Sprintf("%s.%s.%s.%s", za[:32], za[32:], tlsSNITokenID, tlsSNISuffix) - - // Compute the digest for the SAN B that will appear in the certificate - expectedKeyAuthorization := task.Challenge.ExpectedKeyAuthorization(task.Account.Key) - hb := sha256.Sum256([]byte(expectedKeyAuthorization)) - zb := hex.EncodeToString(hb[:]) - sanBName := fmt.Sprintf("%s.%s.%s.%s", zb[:32], zb[32:], tlsSNIKaID, tlsSNISuffix) - - // Perform the validation - result.Error = va.validateTLSSNI02WithNames(hostPort, sanAName, sanBName) - return result -} - -func (va VAImpl) validateTLSSNI02WithNames(hostPort string, sanAName, sanBName string) *acme.ProblemDetails { - certs, problem := va.fetchCerts(hostPort, sanAName) + certs, problem := va.fetchCerts(hostPort, &tls.Config{ + ServerName: task.Identifier, + NextProtos: []string{acme.ACMETLS1Protocol}, + }) if problem != nil { - return problem + result.Error = problem + return result } leafCert := certs[0] - if len(leafCert.DNSNames) != 2 { - names := certNames(leafCert) - msg := fmt.Sprintf( - "%s challenge certificate doesn't include exactly 2 DNSName entries. "+ - "Received %d certificate(s), first certificate had names %q", - acme.ChallengeTLSSNI02, len(certs), names) - return acme.MalformedProblem(msg) - } - var validSanAName, validSanBName bool - for _, name := range leafCert.DNSNames { - if subtle.ConstantTimeCompare([]byte(name), []byte(sanAName)) == 1 { - validSanAName = true - } - - if subtle.ConstantTimeCompare([]byte(name), []byte(sanBName)) == 1 { - validSanBName = true - } - } - - if !validSanAName || !validSanBName { + // Verify SNI + if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], task.Identifier) { names := certNames(leafCert) - msg := fmt.Sprintf( + errText := fmt.Sprintf( "Incorrect validation certificate for %s challenge. "+ "Requested %s from %s. Received %d certificate(s), "+ "first certificate had names %q", - acme.ChallengeTLSSNI02, sanAName, hostPort, - len(certs), names) - return acme.UnauthorizedProblem(msg) + acme.ChallengeTLSALPN01, task.Identifier, hostPort, len(certs), names) + result.Error = acme.UnauthorizedProblem(errText) + return result } - return nil + // Verify key authorization in acmeValidation extension + expectedKeyAuthorization := task.Challenge.ExpectedKeyAuthorization(task.Account.Key) + h := sha256.Sum256([]byte(expectedKeyAuthorization)) + for _, ext := range leafCert.Extensions { + if IdPeAcmeIdentifierV1.Equal(ext.Id) && ext.Critical { + if subtle.ConstantTimeCompare(h[:], ext.Value) == 1 { + return result + } + result.Error = acme.UnauthorizedProblem("Extension acmeValidationV1 value incorrect.") + return result + } + } + + return result } -func (va VAImpl) fetchCerts(hostPort string, sni string) ([]*x509.Certificate, *acme.ProblemDetails) { - conn, err := tls.DialWithDialer( - &net.Dialer{Timeout: time.Second * 5}, "tcp", hostPort, - &tls.Config{ - ServerName: sni, - InsecureSkipVerify: true, - }) +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) if err != nil { // TODO(@cpu): Return better err - see parseHTTPConnError from boulder return nil, acme.UnauthorizedProblem( - fmt.Sprintf("Failed to connect to %s for the %s challenge", hostPort, acme.ChallengeTLSSNI02)) + fmt.Sprintf("Failed to connect to %s for the %s challenge", hostPort, acme.ChallengeTLSALPN01)) } // close errors are not important here @@ -341,7 +314,7 @@ func (va VAImpl) fetchCerts(hostPort string, sni string) ([]*x509.Certificate, * certs := conn.ConnectionState().PeerCertificates if len(certs) == 0 { return nil, acme.UnauthorizedProblem( - fmt.Sprintf("No certs presented for %s challenge", acme.ChallengeTLSSNI02)) + fmt.Sprintf("No certs presented for %s challenge", acme.ChallengeTLSALPN01)) } return certs, nil } diff --git a/wfe/wfe.go b/wfe/wfe.go index f67b6a2c..8ac12f02 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -886,7 +886,7 @@ func (wfe *WebFrontEndImpl) makeChallenges(authz *core.Authorization, request *h chals = []*core.Challenge{chal} } else { // Non-wildcard authorizations get all of the enabled challenge types - enabledChallenges := []string{acme.ChallengeHTTP01, acme.ChallengeTLSSNI02, acme.ChallengeDNS01} + enabledChallenges := []string{acme.ChallengeHTTP01, acme.ChallengeTLSALPN01, acme.ChallengeDNS01} for _, chalType := range enabledChallenges { chal, err := wfe.makeChallenge(chalType, authz, request) if err != nil { From 6786284ebaaf5403fa229bd52418ff49a769a653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20D=C4=99bski?= Date: Thu, 12 Apr 2018 09:06:47 +0000 Subject: [PATCH 2/3] Review fixes. --- va/va.go | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/va/va.go b/va/va.go index 265690c5..0b0d5c88 100644 --- a/va/va.go +++ b/va/va.go @@ -256,15 +256,30 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { ValidatedAt: va.clk.Now(), } - certs, problem := va.fetchCerts(hostPort, &tls.Config{ - ServerName: task.Identifier, - NextProtos: []string{acme.ACMETLS1Protocol}, + cs, problem := va.fetchConnectionState(hostPort, &tls.Config{ + ServerName: task.Identifier, + NextProtos: []string{acme.ACMETLS1Protocol}, + InsecureSkipVerify: true, }) if problem != nil { result.Error = problem return result } + if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != acme.ACMETLS1Protocol { + result.Error = acme.UnauthorizedProblem(fmt.Sprintf( + "Cannot connect using %s for %s challenge", + acme.ACMETLS1Protocol, + acme.ChallengeTLSALPN01, + )) + return result + } + + certs := cs.PeerCertificates + if len(certs) == 0 { + result.Error = acme.UnauthorizedProblem(fmt.Sprintf("No certs presented for %s challenge", acme.ChallengeTLSALPN01)) + return result + } leafCert := certs[0] // Verify SNI @@ -295,9 +310,7 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { return result } -func (va VAImpl) fetchCerts(hostPort string, config *tls.Config) ([]*x509.Certificate, *acme.ProblemDetails) { - config = config.Clone() - config.InsecureSkipVerify = true +func (va VAImpl) fetchConnectionState(hostPort string, config *tls.Config) (*tls.ConnectionState, *acme.ProblemDetails) { conn, err := tls.DialWithDialer(&net.Dialer{Timeout: time.Second * 5}, "tcp", hostPort, config) if err != nil { @@ -311,12 +324,8 @@ func (va VAImpl) fetchCerts(hostPort string, config *tls.Config) ([]*x509.Certif _ = conn.Close() }() - certs := conn.ConnectionState().PeerCertificates - if len(certs) == 0 { - return nil, acme.UnauthorizedProblem( - fmt.Sprintf("No certs presented for %s challenge", acme.ChallengeTLSALPN01)) - } - return certs, nil + cs := conn.ConnectionState() + return &cs, nil } func (va VAImpl) validateHTTP01(task *vaTask) *core.ValidationRecord { From 60d2fd74dc56cfcd03d1b8af423337f6926ae2ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20D=C4=99bski?= Date: Mon, 16 Apr 2018 14:35:09 +0000 Subject: [PATCH 3/3] review fixes 2 --- va/va.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/va/va.go b/va/va.go index 0b0d5c88..7668b046 100644 --- a/va/va.go +++ b/va/va.go @@ -268,7 +268,7 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != acme.ACMETLS1Protocol { result.Error = acme.UnauthorizedProblem(fmt.Sprintf( - "Cannot connect using %s for %s challenge", + "Cannot negotiate ALPN protocol %q for %s challenge", acme.ACMETLS1Protocol, acme.ChallengeTLSALPN01, )) @@ -282,7 +282,7 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { } leafCert := certs[0] - // Verify SNI + // Verify SNI - certificate returned must be issued only for the domain we are verifying. if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], task.Identifier) { names := certNames(leafCert) errText := fmt.Sprintf( @@ -302,11 +302,18 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { if subtle.ConstantTimeCompare(h[:], ext.Value) == 1 { return result } - result.Error = acme.UnauthorizedProblem("Extension acmeValidationV1 value incorrect.") + errText := fmt.Sprintf("Incorrect validation certificate for %s challenge. "+ + "Invalid acmeValidationV1 extension value.", acme.ChallengeTLSALPN01) + result.Error = acme.UnauthorizedProblem(errText) return result } } + errText := fmt.Sprintf( + "Incorrect validation certificate for %s challenge. "+ + "Missing acmeValidationV1 extension.", + acme.ChallengeTLSALPN01) + result.Error = acme.UnauthorizedProblem(errText) return result }