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.

Sign up for GitHub

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

Promote -strict behaviors to the default #203

Merged
merged 6 commits into from
Feb 19, 2019
Merged

Promote -strict behaviors to the default #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3eeae33
Make existing strict-only behaviors the default.
jsha Feb 14, 2019
014a484
Check out branch of Certbot.
jsha Feb 14, 2019
c23f698
Fix travis.
jsha Feb 14, 2019
fe5d2f3
Review feedback.
jsha Feb 15, 2019
6597be9
Fix Challenge and Authorization handlers.
jsha Feb 15, 2019
601e43d
Fix nits.
jsha Feb 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ services:
- docker

before_install:
- git clone https://github.com/certbot/certbot
# TODO(#204): Change back to cloning master once Certbot omits keyAuthorization.
- git clone --depth=1 -b no-keyauthorization https://github.com/certbot/certbot
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you file a Pebble issue to link with a TODO here to revert to Certbot master when they've fixed this upstream?

- cd certbot
- ./certbot-auto --os-packages-only -n
- ./tools/venv.py
Expand Down
10 changes: 0 additions & 10 deletions va/va.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down Expand Up @@ -396,11 +391,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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also delete IdPeAcmeIdentifierV1Obsolete - nothing else uses it AFAICT

hasAcmeIdentifier = true
}
if hasAcmeIdentifier {
var extValue []byte
if _, err := asn1.Unmarshal(ext.Value, &extValue); err != nil {
Expand Down
245 changes: 97 additions & 148 deletions wfe/wfe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -1363,24 +1352,15 @@ func (wfe *WebFrontEndImpl) Order(
logEvent *requestEvent,
response http.ResponseWriter,
request *http.Request) {

var account *core.Account
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
}
account, 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
}

orderID := strings.TrimPrefix(request.URL.Path, orderPath)
Expand Down Expand Up @@ -1586,11 +1566,12 @@ 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)
// 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
}

Expand All @@ -1601,66 +1582,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
}
}

Expand All @@ -1679,12 +1649,12 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the diff to this function missed some complexity that could be reduced further down at L1700 (Github won't let me leave a comment on the line):

	// 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 {

There shouldn't be any case where account == nil with mandatory POST-as-GET right?

if request.Method == "GET" && wfe.strict {
response.Header().Set("Allow", "POST")
wfe.sendError(acme.MethodNotAllowed(), response)
// There are two possibilities:
// A) request is a POST to begin a challenge
// B) request is a POST-as-GET to poll a challenge
postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK)
if prob != nil {
wfe.sendError(prob, response)
return
}

Expand All @@ -1695,39 +1665,25 @@ 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
// If the post isn't a POST-as-GET its case A)
var account *core.Account
if request.Method == "POST" {
postData, prob := wfe.verifyPOST(ctx, logEvent, request, wfe.lookupJWK)
if !postData.postAsGet {
wfe.updateChallenge(ctx, postData, response, request)
return
} else {
// Otherwise it is case B)
account, 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
}
}

// Lock the challenge for reading in order to write the response
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)
Expand Down Expand Up @@ -1836,9 +1792,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, "+
Expand Down Expand Up @@ -1919,21 +1874,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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be capturing the acct return from validPOSTAsGet and checking that the account specified owns the order corresponding to the certificate serial being fetched. It looks like this was missed in the original code and might need a new way to map from the certificate serial to the order. Maybe better as a follow-up issue? If you agree will you file the ticket?

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)
Expand Down