Promote -strict behaviors to the default #203
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
Can you file a Pebble issue to link with a TODO here to revert to Certbot master when they've fixed this upstream?
| @@ -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) { | |||
There was a problem hiding this comment.
You can also delete IdPeAcmeIdentifierV1Obsolete - nothing else uses it AFAICT
wfe/wfe.go
Outdated
| @@ -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" { | |||
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
I think this also means the else if that follows can be simplified.
| wfe.sendError(prob, response) | ||
| return | ||
| } | ||
| _, prob = wfe.validPOSTAsGET(postData) |
There was a problem hiding this comment.
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?
wfe/wfe.go
Outdated
| wfe.sendError(acme.MethodNotAllowed(), response) | ||
| return | ||
| } | ||
|
|
||
| chalID := strings.TrimPrefix(request.URL.Path, challengePath) | ||
| chal := wfe.db.GetChallengeByID(chalID) |
There was a problem hiding this comment.
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.
| @@ -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. | |||
There was a problem hiding this comment.
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?
wfe/wfe.go
Outdated
| wfe.sendError(acme.MethodNotAllowed(), response) | ||
| return | ||
| } | ||
|
|
||
| authzID := strings.TrimPrefix(request.URL.Path, authzPath) | ||
| authz := wfe.db.GetAuthorizationByID(authzID) |
There was a problem hiding this comment.
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.
.travis.yml
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
tiny nit.
| # TODO(#204): Change back to cloning master once Certbot omits # keyAuthorization. | |
| # TODO(#204): Change back to cloning master once Certbot omits keyAuthorization. |
wfe/wfe.go
Outdated
| @@ -1365,23 +1354,17 @@ func (wfe *WebFrontEndImpl) Order( | |||
| request *http.Request) { | |||
|
|
|||
| var account *core.Account | |||
There was a problem hiding this comment.
I think you can delete this explicit var & the account = acct assignment on L1367 by changing L1362 to account, prob := wfe.validPOSTAsGET(postData).
This change keeps the -strict flag and related plumbing, in case we need it again in the future.
Since this rejects challenge POSTs with
keyAuthorization, and Certbot master still sends that,I've updated the Travis config to check out a branch of Certbot that is tweaked to not send it.
Resolves #192
Resolves #206