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 3 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
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ services:
- docker

before_install:
- git clone https://github.com/certbot/certbot
- 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
5 changes: 0 additions & 5 deletions va/va.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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
93 changes: 29 additions & 64 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 @@ -1365,7 +1354,7 @@ func (wfe *WebFrontEndImpl) Order(
request *http.Request) {

var account *core.Account
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 you can delete this explicit var & the account = acct assignment on L1367 by changing L1362 to account, prob := wfe.validPOSTAsGET(postData).

if request.Method == "GET" && wfe.strict {
if request.Method == "GET" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is meaningful anymore because the handle func is only registered for "POST" now:

wfe.HandleFunc(m, orderPath, wfe.Order, "POST")

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 this also means the else if that follows can be simplified.

response.Header().Set("Allow", "POST")
wfe.sendError(acme.MethodNotAllowed(), response)
return
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe unrelated to this PR but it looks like we're fetching the authz (& returning a 404 if it isn't found) before validating the POST. I think it would be better to do the opposite.

if authz == nil {
Expand Down Expand Up @@ -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.
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)
return
}

chalID := strings.TrimPrefix(request.URL.Path, challengePath)
chal := wfe.db.GetChallengeByID(chalID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe unrelated to this PR but it looks like we're fetching the challenge (& returning a 404 if it isn't found) before validating the POST. I think it would be better to do the opposite.

if chal == nil {
Expand Down Expand Up @@ -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, "+
Expand Down Expand Up @@ -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)
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