From b26e6e42b38183138a61a1a6813b345eccc76b3b Mon Sep 17 00:00:00 2001 From: David Cowden Date: Tue, 26 May 2020 01:38:24 -0700 Subject: [PATCH 1/2] acme: Return 501 for the key-change route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/smallstep/certificates/issues/209 --- acme/api/handler.go | 7 +++++++ acme/errors.go | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index e848e64d..83a5d800 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -59,6 +59,7 @@ 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)) r.MethodFunc("POST", getLink(acme.NewOrderLink, "{provisionerID}", false, nil), extractPayloadByKid(h.NewOrder)) r.MethodFunc("POST", getLink(acme.OrderLink, "{provisionerID}", false, nil, "{ordID}"), extractPayloadByKid(h.isPostAsGet(h.GetOrder))) r.MethodFunc("POST", getLink(acme.OrdersByAccountLink, "{provisionerID}", false, nil, "{accID}"), extractPayloadByKid(h.isPostAsGet(h.GetOrdersByAccount))) @@ -66,6 +67,7 @@ func (h *Handler) Route(r api.Router) { r.MethodFunc("POST", getLink(acme.AuthzLink, "{provisionerID}", false, nil, "{authzID}"), extractPayloadByKid(h.isPostAsGet(h.GetAuthz))) r.MethodFunc("POST", getLink(acme.ChallengeLink, "{provisionerID}", false, nil, "{chID}"), extractPayloadByKid(h.GetChallenge)) r.MethodFunc("POST", getLink(acme.CertificateLink, "{provisionerID}", false, nil, "{certID}"), extractPayloadByKid(h.isPostAsGet(h.GetCertificate))) + } // GetNonce just sets the right header since a Nonce is added to each response @@ -89,6 +91,11 @@ func (h *Handler) GetDirectory(w http.ResponseWriter, r *http.Request) { api.JSON(w, dir) } +// NotImplemented returns a 501. This is a place holder for future functionality. +func (h *Handler) NotImplemented(w http.ResponseWriter, r *http.Request) { + api.WriteError(w, acme.NotImplemented(nil).ToACME()) +} + // GetAuthz ACME api for retrieving an Authz. func (h *Handler) GetAuthz(w http.ResponseWriter, r *http.Request) { acc, err := acme.AccountFromContext(r.Context()) diff --git a/acme/errors.go b/acme/errors.go index dd6f4aff..878a1fdc 100644 --- a/acme/errors.go +++ b/acme/errors.go @@ -194,6 +194,16 @@ func ServerInternalErr(err error) *Error { } } +// NotImplemented returns a new acme error. +func NotImplemented(err error) *Error { + return &Error{ + Type: notImplemented, + Detail: "The requested operation is not implemented", + Status: 501, + Err: err, + } +} + // TLSErr returns a new acme error. func TLSErr(err error) *Error { return &Error{ @@ -296,6 +306,8 @@ const ( unsupportedIdentifierErr // Visit the “instance” URL and take actions specified there userActionRequiredErr + // The operation is not implemented + notImplemented ) // String returns the string representation of the acme problem type, @@ -350,6 +362,8 @@ func (ap ProbType) String() string { return "unsupportedIdentifier" case userActionRequiredErr: return "userActionRequired" + case notImplemented: + return "notImplemented" default: return "unsupported type" } @@ -398,10 +412,28 @@ func (e *Error) Cause() error { return e.Err } +// Official returns true if this error is registered with the IETF. +// +// The RFC says: +// +// This list is not exhaustive. The server MAY return errors whose +// "type" field is set to a URI other than those defined above. Servers +// MUST NOT use the ACME URN namespace for errors not listed in the +// appropriate IANA registry (see Section 9.6). Clients SHOULD display +// the "detail" field of all errors. +// +func (e *Error) Official() bool { + return e.Type != notImplemented +} + // ToACME returns an acme representation of the problem type. func (e *Error) ToACME() *AError { + prefix := "urn:step:acme:error" + if e.Official() { + prefix = "urn:ietf:params:acme:error:" + } ae := &AError{ - Type: "urn:ietf:params:acme:error:" + e.Type.String(), + Type: prefix + e.Type.String(), Detail: e.Error(), Status: e.Status, } From a26b5f322d2c1d7926d4dc5998eec8f7c945c4f1 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 27 May 2020 17:08:47 -0700 Subject: [PATCH 2/2] acme/api: Brush up documentation on key-change Add more specific wording describing what a 501 means and add more color explaining how official vs unofficial error types should be handled. --- acme/api/handler.go | 4 ++-- acme/errors.go | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 83a5d800..dd30c7c1 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -67,7 +67,6 @@ func (h *Handler) Route(r api.Router) { r.MethodFunc("POST", getLink(acme.AuthzLink, "{provisionerID}", false, nil, "{authzID}"), extractPayloadByKid(h.isPostAsGet(h.GetAuthz))) r.MethodFunc("POST", getLink(acme.ChallengeLink, "{provisionerID}", false, nil, "{chID}"), extractPayloadByKid(h.GetChallenge)) r.MethodFunc("POST", getLink(acme.CertificateLink, "{provisionerID}", false, nil, "{certID}"), extractPayloadByKid(h.isPostAsGet(h.GetCertificate))) - } // GetNonce just sets the right header since a Nonce is added to each response @@ -91,7 +90,8 @@ func (h *Handler) GetDirectory(w http.ResponseWriter, r *http.Request) { api.JSON(w, dir) } -// NotImplemented returns a 501. This is a place holder for future functionality. +// NotImplemented returns a 501 and is generally a placeholder for functionality which +// MAY be added at some point in the future but is not in any way a guarantee of such. func (h *Handler) NotImplemented(w http.ResponseWriter, r *http.Request) { api.WriteError(w, acme.NotImplemented(nil).ToACME()) } diff --git a/acme/errors.go b/acme/errors.go index 878a1fdc..a4dd8159 100644 --- a/acme/errors.go +++ b/acme/errors.go @@ -412,9 +412,15 @@ func (e *Error) Cause() error { return e.Err } -// Official returns true if this error is registered with the IETF. +// Official returns true if this error's type is listed in §6.7 of RFC 8555. +// Error types in §6.7 are registered under IETF urn namespace: // -// The RFC says: +// "urn:ietf:params:acme:error:" +// +// and should include the namespace as a prefix when appearing as a problem +// document. +// +// RFC 8555 also says: // // This list is not exhaustive. The server MAY return errors whose // "type" field is set to a URI other than those defined above. Servers @@ -422,11 +428,15 @@ func (e *Error) Cause() error { // appropriate IANA registry (see Section 9.6). Clients SHOULD display // the "detail" field of all errors. // +// In this case Official returns `false` so that a different namespace can +// be used. func (e *Error) Official() bool { return e.Type != notImplemented } // ToACME returns an acme representation of the problem type. +// For official errors, the IETF ACME namespace is prepended to the error type. +// For our own errors, we use an (yet) unregistered smallstep acme namespace. func (e *Error) ToACME() *AError { prefix := "urn:step:acme:error" if e.Official() {