From 3eeae33adc27b2660759979c97902d2aaa890346 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 14 Feb 2019 13:08:15 -0800 Subject: [PATCH 1/6] Make existing strict-only behaviors the default. --- va/va.go | 5 --- wfe/wfe.go | 93 +++++++++++++++++------------------------------------- 2 files changed, 29 insertions(+), 69 deletions(-) diff --git a/va/va.go b/va/va.go index 89c114be..13e0f8f5 100644 --- a/va/va.go +++ b/va/va.go @@ -396,11 +396,6 @@ func (va VAImpl) validateTLSALPN01(task *vaTask) *core.ValidationRecord { for _, ext := range leafCert.Extensions { if ext.Critical { hasAcmeIdentifier := IdPeAcmeIdentifier.Equal(ext.Id) - // For backwards compatibility, check old identifier - // as well if strict mode is not enabled - if !va.strict && IdPeAcmeIdentifierV1Obsolete.Equal(ext.Id) { - hasAcmeIdentifier = true - } if hasAcmeIdentifier { var extValue []byte if _, err := asn1.Unmarshal(ext.Value, &extValue); err != nil { diff --git a/wfe/wfe.go b/wfe/wfe.go index 350f9a5e..33bda8aa 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -270,17 +270,10 @@ func (wfe *WebFrontEndImpl) Handler() http.Handler { wfe.HandleFunc(m, acctPath, wfe.UpdateAccount, "POST") wfe.HandleFunc(m, keyRolloverPath, wfe.KeyRollover, "POST") wfe.HandleFunc(m, revokeCertPath, wfe.RevokeCert, "POST") - - // GET and POST handlers - wfe.HandleFunc(m, certPath, wfe.Certificate, "GET", "POST") - - // POST handlers with legacy GET support - // NOTE(@cpu): These handlers will eventually not support GET. It is presently - // allowed when `-strict=false` to support ACME <=draft-14 which allowed - // unauthenticated GET access to these resources. - wfe.HandleFunc(m, orderPath, wfe.Order, "GET", "POST") - wfe.HandleFunc(m, authzPath, wfe.Authz, "GET", "POST") - wfe.HandleFunc(m, challengePath, wfe.Challenge, "GET", "POST") + wfe.HandleFunc(m, certPath, wfe.Certificate, "POST") + wfe.HandleFunc(m, orderPath, wfe.Order, "POST") + wfe.HandleFunc(m, authzPath, wfe.Authz, "POST") + wfe.HandleFunc(m, challengePath, wfe.Challenge, "POST") return m } @@ -362,10 +355,8 @@ func (wfe *WebFrontEndImpl) Nonce( request *http.Request) { statusCode := http.StatusNoContent // The ACME specification says GET requets should receive http.StatusNoContent - // and HEAD requests should receive http.StatusOK. We only return StatusOK for - // HEAD requests in strict mode because this was the legacy behaviour and it - // may break clients to change. - if wfe.strict && request.Method == "HEAD" { + // and HEAD requests should receive http.StatusOK. + if request.Method == "HEAD" { statusCode = http.StatusOK } response.WriteHeader(statusCode) @@ -486,19 +477,17 @@ func (wfe *WebFrontEndImpl) lookupJWK(request *http.Request, jws *jose.JSONWebSi } func (wfe *WebFrontEndImpl) validPOST(request *http.Request) *acme.ProblemDetails { - if wfe.strict { - // Section 6.2 says to reject JWS requests without the expected Content-Type - // using a status code of http.UnsupportedMediaType - if _, present := request.Header["Content-Type"]; !present { - return acme.UnsupportedMediaTypeProblem( - `missing Content-Type header on POST. ` + - `Content-Type must be "application/jose+json"`) - } - if contentType := request.Header.Get("Content-Type"); contentType != expectedJWSContentType { - return acme.UnsupportedMediaTypeProblem( - `Invalid Content-Type header on POST. ` + - `Content-Type must be "application/jose+json"`) - } + // Section 6.2 says to reject JWS requests without the expected Content-Type + // using a status code of http.UnsupportedMediaType + if _, present := request.Header["Content-Type"]; !present { + return acme.UnsupportedMediaTypeProblem( + `missing Content-Type header on POST. ` + + `Content-Type must be "application/jose+json"`) + } + if contentType := request.Header.Get("Content-Type"); contentType != expectedJWSContentType { + return acme.UnsupportedMediaTypeProblem( + `Invalid Content-Type header on POST. ` + + `Content-Type must be "application/jose+json"`) } if _, present := request.Header["Content-Length"]; !present { @@ -758,7 +747,7 @@ func (wfe *WebFrontEndImpl) UpdateAccount( // if this update contains no contacts or deactivated status, // simply return the existing account and return early. if updateAcctReq.Contact == nil && updateAcctReq.Status != acme.StatusDeactivated { - if wfe.strict && !postData.postAsGet { + if !postData.postAsGet { wfe.sendError(acme.MalformedProblem("Use POST-as-GET to retrieve account data instead of doing an empty update"), response) return } @@ -1365,7 +1354,7 @@ func (wfe *WebFrontEndImpl) Order( request *http.Request) { var account *core.Account - if request.Method == "GET" && wfe.strict { + if request.Method == "GET" { response.Header().Set("Allow", "POST") wfe.sendError(acme.MethodNotAllowed(), response) return @@ -1586,14 +1575,6 @@ func (wfe *WebFrontEndImpl) Authz( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - - // Only allow GET when not being strict - if request.Method == "GET" && wfe.strict { - response.Header().Set("Allow", "POST") - wfe.sendError(acme.MethodNotAllowed(), response) - return - } - authzID := strings.TrimPrefix(request.URL.Path, authzPath) authz := wfe.db.GetAuthorizationByID(authzID) if authz == nil { @@ -1679,15 +1660,6 @@ func (wfe *WebFrontEndImpl) Challenge( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - - // Unauthenticated GETs to challenges are only allowed when not running in - // strict mode. - if request.Method == "GET" && wfe.strict { - response.Header().Set("Allow", "POST") - wfe.sendError(acme.MethodNotAllowed(), response) - return - } - chalID := strings.TrimPrefix(request.URL.Path, challengePath) chal := wfe.db.GetChallengeByID(chalID) if chal == nil { @@ -1836,9 +1808,8 @@ func (wfe *WebFrontEndImpl) updateChallenge( // unnecessary, the server can calculate this itself. We could ignore this if // sent (and that's what Boulder will do) but for Pebble we'd like to offer // a way to be more aggressive about pushing clients implementations in the - // right direction, so we treat this as a malformed request when running in - // strict mode. - if wfe.strict && chalResp.KeyAuthorization != nil { + // right direction, so we treat this as a malformed request. + if chalResp.KeyAuthorization != nil { wfe.sendError( acme.MalformedProblem( "Challenge response body contained legacy KeyAuthorization field, "+ @@ -1919,21 +1890,15 @@ func (wfe *WebFrontEndImpl) Certificate( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - if request.Method == "GET" && wfe.strict { - response.Header().Set("Allow", "POST") - wfe.sendError(acme.MethodNotAllowed(), response) + postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) + if prob != nil { + wfe.sendError(prob, response) + return + } + _, prob = wfe.validPOSTAsGET(postData) + if prob != nil { + wfe.sendError(prob, response) return - } else if request.Method == "POST" { - postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) - if prob != nil { - wfe.sendError(prob, response) - return - } - _, prob = wfe.validPOSTAsGET(postData) - if prob != nil { - wfe.sendError(prob, response) - return - } } serial := strings.TrimPrefix(request.URL.Path, certPath) From 014a4843f13a3bdb20c10f3bbd287da73cce0dac Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 14 Feb 2019 14:37:50 -0800 Subject: [PATCH 2/6] Check out branch of Certbot. --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 716e93e5..546270be 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,7 +16,7 @@ services: - docker before_install: - - git clone https://github.com/certbot/certbot + - git checkout -b no-keyauthorization https://github.com/certbot/certbot - cd certbot - ./certbot-auto --os-packages-only -n - ./tools/venv.py From c23f6987fad8dc8448a70d266bc89ce7d2ebef7b Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 14 Feb 2019 14:56:51 -0800 Subject: [PATCH 3/6] Fix travis. --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 546270be..570310d7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,7 +16,7 @@ services: - docker before_install: - - git checkout -b no-keyauthorization https://github.com/certbot/certbot + - git clone --depth=1 -b no-keyauthorization https://github.com/certbot/certbot - cd certbot - ./certbot-auto --os-packages-only -n - ./tools/venv.py From fe5d2f3e83d716d9614eb8a94eee87d931673ab1 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 15 Feb 2019 14:05:09 -0800 Subject: [PATCH 4/6] Review feedback. --- .travis.yml | 1 + va/va.go | 5 ----- wfe/wfe.go | 28 ++++++++++------------------ 3 files changed, 11 insertions(+), 23 deletions(-) diff --git a/.travis.yml b/.travis.yml index 570310d7..1953a619 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,6 +16,7 @@ services: - docker before_install: + # TODO(#204): Change back to cloning master once Certbot omits # keyAuthorization. - git clone --depth=1 -b no-keyauthorization https://github.com/certbot/certbot - cd certbot - ./certbot-auto --os-packages-only -n diff --git a/va/va.go b/va/va.go index 13e0f8f5..bd824646 100644 --- a/va/va.go +++ b/va/va.go @@ -73,11 +73,6 @@ const ( // (https://tools.ietf.org/html/draft-ietf-acme-tls-alpn-04#page-4) var IdPeAcmeIdentifier = asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 31} -// This is the identifier defined in draft-01. This is only supported for backwards -// compatibility. -// (https://tools.ietf.org/html/draft-ietf-acme-tls-alpn-01#page-4) -var IdPeAcmeIdentifierV1Obsolete = asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 30, 1} - func userAgent() string { return fmt.Sprintf( "%s (%s; %s)", diff --git a/wfe/wfe.go b/wfe/wfe.go index 33bda8aa..ff8ceb01 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1354,23 +1354,17 @@ func (wfe *WebFrontEndImpl) Order( request *http.Request) { var account *core.Account - if request.Method == "GET" { - response.Header().Set("Allow", "POST") - wfe.sendError(acme.MethodNotAllowed(), response) + postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) + if prob != nil { + wfe.sendError(prob, response) + return + } + acct, prob := wfe.validPOSTAsGET(postData) + if prob != nil { + wfe.sendError(prob, response) return - } else if request.Method == "POST" { - postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) - if prob != nil { - wfe.sendError(prob, response) - return - } - acct, prob := wfe.validPOSTAsGET(postData) - if prob != nil { - wfe.sendError(prob, response) - return - } - account = acct } + account = acct orderID := strings.TrimPrefix(request.URL.Path, orderPath) order := wfe.db.GetOrderByID(orderID) @@ -1697,9 +1691,7 @@ func (wfe *WebFrontEndImpl) Challenge( chal.RLock() defer chal.RUnlock() - // If there was an account authenticating this GET request then make sure it - // owns the challenge. - if account != nil && chal.Authz.Order.AccountID != account.ID { + if chal.Authz.Order.AccountID != account.ID { response.WriteHeader(http.StatusUnauthorized) wfe.sendError(acme.UnauthorizedProblem( "Account authenticating request is not the owner of the challenge"), response) From 6597be987563231023633284fd4a6c19fd412100 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 15 Feb 2019 14:14:04 -0800 Subject: [PATCH 5/6] Fix Challenge and Authorization handlers. --- wfe/wfe.go | 140 ++++++++++++++++++++++++++--------------------------- 1 file changed, 68 insertions(+), 72 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index ff8ceb01..3d1487fb 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1569,6 +1569,15 @@ func (wfe *WebFrontEndImpl) Authz( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { + // There are two types of requests we might get: + // A) a POST to update the authorization + // B) a POST-as-GET to get the authorization + postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) + if prob != nil { + wfe.sendError(prob, response) + return + } + authzID := strings.TrimPrefix(request.URL.Path, authzPath) authz := wfe.db.GetAuthorizationByID(authzID) if authz == nil { @@ -1576,66 +1585,55 @@ func (wfe *WebFrontEndImpl) Authz( return } - // If the request was a POST there are two options: - // A) a POST to update the authorization - // B) a POST-as-GET to get the authorization - if request.Method == "POST" { - postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) + // If the postData is not a POST-as-GET, treat this as case A) and update + // the authorization based on the postData + if !postData.postAsGet { + existingAcct, prob := wfe.getAcctByKey(postData.jwk) if prob != nil { wfe.sendError(prob, response) return } - // If the postData is not a POST-as-GET, treat this as case A) and update - // the authorization based on the postData - if !postData.postAsGet { - existingAcct, prob := wfe.getAcctByKey(postData.jwk) - if prob != nil { - wfe.sendError(prob, response) - return - } - - if authz.Order.AccountID != existingAcct.ID { - wfe.sendError(acme.UnauthorizedProblem( - "Account does not own authorization"), response) - return - } + if authz.Order.AccountID != existingAcct.ID { + wfe.sendError(acme.UnauthorizedProblem( + "Account does not own authorization"), response) + return + } - var deactivateRequest struct { - Status string - } - err := json.Unmarshal(postData.body, &deactivateRequest) - if err != nil { - wfe.sendError(acme.MalformedProblem( - fmt.Sprintf("Malformed authorization update: %s", - err.Error())), response) - return - } + var deactivateRequest struct { + Status string + } + err := json.Unmarshal(postData.body, &deactivateRequest) + if err != nil { + wfe.sendError(acme.MalformedProblem( + fmt.Sprintf("Malformed authorization update: %s", + err.Error())), response) + return + } - if deactivateRequest.Status != "deactivated" { - wfe.sendError(acme.MalformedProblem( - fmt.Sprintf("Malformed authorization update, status must be \"deactivated\" not %q", - deactivateRequest.Status)), response) - return - } - authz.Status = acme.StatusDeactivated - } else { - // Otherwise this was a POST-as-GET request and we need to verify it - // accordingly and ensure the authorized account owns the authorization - // being fetched. - account, prob := wfe.validPOSTAsGET(postData) - if prob != nil { - wfe.sendError(prob, response) - return - } + if deactivateRequest.Status != "deactivated" { + wfe.sendError(acme.MalformedProblem( + fmt.Sprintf("Malformed authorization update, status must be \"deactivated\" not %q", + deactivateRequest.Status)), response) + return + } + authz.Status = acme.StatusDeactivated + } else { + // Otherwise this was a POST-as-GET request and we need to verify it + // accordingly and ensure the authorized account owns the authorization + // being fetched. + account, prob := wfe.validPOSTAsGET(postData) + if prob != nil { + wfe.sendError(prob, response) + return + } - if authz.Order.AccountID != account.ID { - response.WriteHeader(http.StatusForbidden) - wfe.sendError(acme.UnauthorizedProblem( - "Account authorizing the request is not the owner of the authorization"), - response) - return - } + if authz.Order.AccountID != account.ID { + response.WriteHeader(http.StatusForbidden) + wfe.sendError(acme.UnauthorizedProblem( + "Account authorizing the request is not the owner of the authorization"), + response) + return } } @@ -1654,6 +1652,16 @@ func (wfe *WebFrontEndImpl) Challenge( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { + // There are two possibilities: + // A) request is a POST to begin a challenge + // B) request is a POST-as-GET to poll a challenge + var account *core.Account + postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) + if prob != nil { + wfe.sendError(prob, response) + return + } + chalID := strings.TrimPrefix(request.URL.Path, challengePath) chal := wfe.db.GetChallengeByID(chalID) if chal == nil { @@ -1661,30 +1669,18 @@ func (wfe *WebFrontEndImpl) Challenge( return } - // If the request is a POST there are two possibilities: - // A) it is a POST to begin a challenge - // B) it is a POST-as-GET to poll a challenge - var account *core.Account - if request.Method == "POST" { - postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) + // If the post isn't a POST-as-GET its case A) + if !postData.postAsGet { + wfe.updateChallenge(ctx, postData, response, request) + return + } else { + // Otherwise it is case B) + acct, prob := wfe.validPOSTAsGET(postData) if prob != nil { wfe.sendError(prob, response) return } - - // If the post isn't a POST-as-GET its case A) - if !postData.postAsGet { - wfe.updateChallenge(ctx, postData, response, request) - return - } else { - // Otherwise it is case B) - acct, prob := wfe.validPOSTAsGET(postData) - if prob != nil { - wfe.sendError(prob, response) - return - } - account = acct - } + account = acct } // Lock the challenge for reading in order to write the response From 601e43d2f9bb594cc791c5829371719189f7781e Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 15 Feb 2019 14:31:03 -0800 Subject: [PATCH 6/6] Fix nits. --- .travis.yml | 2 +- wfe/wfe.go | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1953a619..7b81e7f7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,7 +16,7 @@ services: - docker before_install: - # TODO(#204): Change back to cloning master once Certbot omits # keyAuthorization. + # TODO(#204): Change back to cloning master once Certbot omits keyAuthorization. - git clone --depth=1 -b no-keyauthorization https://github.com/certbot/certbot - cd certbot - ./certbot-auto --os-packages-only -n diff --git a/wfe/wfe.go b/wfe/wfe.go index 3d1487fb..f86966d7 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1352,19 +1352,16 @@ func (wfe *WebFrontEndImpl) Order( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - - var account *core.Account postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) if prob != nil { wfe.sendError(prob, response) return } - acct, prob := wfe.validPOSTAsGET(postData) + account, prob := wfe.validPOSTAsGET(postData) if prob != nil { wfe.sendError(prob, response) return } - account = acct orderID := strings.TrimPrefix(request.URL.Path, orderPath) order := wfe.db.GetOrderByID(orderID) @@ -1655,7 +1652,6 @@ func (wfe *WebFrontEndImpl) Challenge( // There are two possibilities: // A) request is a POST to begin a challenge // B) request is a POST-as-GET to poll a challenge - var account *core.Account postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK) if prob != nil { wfe.sendError(prob, response) @@ -1670,17 +1666,17 @@ func (wfe *WebFrontEndImpl) Challenge( } // If the post isn't a POST-as-GET its case A) + var account *core.Account if !postData.postAsGet { wfe.updateChallenge(ctx, postData, response, request) return } else { // Otherwise it is case B) - acct, prob := wfe.validPOSTAsGET(postData) + account, prob = wfe.validPOSTAsGET(postData) if prob != nil { wfe.sendError(prob, response) return } - account = acct } // Lock the challenge for reading in order to write the response