acme: Return 501 for the key-change route #276
Conversation
RFC 8555 § 7.3.5 is not listed as optional but we do not currently support it. Rather than 404, return a 501 to inform clients that this functionality is not yet implemented. The notImplmented error type is not an official error registered in the ietf:params:acme:error namespace, so prefix if with step:acme:error. An ACME server is allowed to return other errors and clients should display the message detail to users. Fixes: #209
Codecov Report
@@ Coverage Diff @@
## master #276 +/- ##
==========================================
+ Coverage 73.93% 74.17% +0.24%
==========================================
Files 76 79 +3
Lines 8670 8964 +294
==========================================
+ Hits 6410 6649 +239
- Misses 1920 1974 +54
- Partials 340 341 +1
Continue to review full report at Codecov.
|
| @@ -59,13 +59,15 @@ func (h *Handler) Route(r api.Router) { | |||
|
|
|||
| r.MethodFunc("POST", getLink(acme.NewAccountLink, "{provisionerID}", false, nil), extractPayloadByJWK(h.NewAccount)) | |||
| r.MethodFunc("POST", getLink(acme.AccountLink, "{provisionerID}", false, nil, "{accID}"), extractPayloadByKid(h.GetUpdateAccount)) | |||
| r.MethodFunc("POST", getLink(acme.KeyChangeLink, "{provisionerID}", false, nil, "{accID}"), extractPayloadByKid(h.NotImplemented)) | |||
There was a problem hiding this comment.
Any reason to ExtractPayloadByKid if we're just going to respond 501? That method might return a 4xx instead of a 501. @dopey
There was a problem hiding this comment.
Oh, sorry, were you just copying my comment to the right place?
There was a problem hiding this comment.
I figure we do as much as we can until we hit the "not implemented" point. That way we don't expose anything that doesn't need to be.
|
@dopey is there any reason we wouldn't or shouldn't support it eventually? If so maybe there's a different way to communicate it. But AFAIU it's part of the spec and probably worth it to support at some point. |
|
I'm guessing we would never support it if no one ever asks for it. It may not be a super useful feature for a private ACME server. New accounts are cheap, so maybe our stance will just be "create a new account". Doesn't matter, I'm fine with 501. You've left good documentation for why you chose that and if someone has a problem with it downstream they can open an issue. |
|
@dopey reworded the comment to make sure a 501 in no way is a guarantee of future functionality. |
|
Looks like travis is failing so let's make sure we get that sorted before merging. |
Add more specific wording describing what a 501 means and add more color explaining how official vs unofficial error types should be handled.
dcow
force-pushed
the
dcow/key-change-error
branch
from
c09db9b
to
a26b5f3
Compare
RFC 8555 § 7.3.5 is not listed as optional but we do not currently
support it. Rather than 404, return a 501 to inform clients that this
functionality is not yet implemented.
The notImplmented error type is not an official error registered in the
ietf:params:acme:error namespace, so prefix if with step:acme:error. An
ACME server is allowed to return other errors and clients should display
the message detail to users.
Fixes: #209