Permalink
Browse files

ACMEv2: Enforce POST Content-Type (when feature on) (#3532)

This commit adds a new WFE2 feature flag "EnforceV2ContentType". When
enabled, the WFE2's validPostRequest function will enforce that the
request carries a Content-Type header equal to
application/jose+json. This is required by ACME draft-10 per section
6.2 "Request Authentication".

This is behind a feature flag because it is likely to break
some number of existing ACMEv2 clients that may not be sending the
correct Content-Type.

We are defaulting to not setting the new feature flag in test/config-next
because it currently break's Certbot's acme module's revocation support
and we rely on this in our V2 integration tests.

Resolves #3529
  • Loading branch information...
cpu authored and rolandshoemaker committed Mar 8, 2018
1 parent 5c4f5e3 commit 3062662aad48db676ac0fc3fb6b49849082bce4d
Showing with 74 additions and 8 deletions.
  1. +2 −2 features/featureflag_string.go
  2. +4 −0 features/features.go
  3. +10 −0 probs/probs.go
  4. +21 −0 wfe2/verify.go
  5. +37 −6 wfe2/verify_test.go

Some generated files are not rendered by default. Learn more.

Oops, something went wrong.
@@ -25,6 +25,9 @@ const (
// Allow TLS-SNI in new-authz that are revalidating for previous issuance
TLSSNIRevalidation
CancelCTSubmissions
// Return errors to ACMEv2 clients that do not send the correct JWS
// Content-Type header
EnforceV2ContentType
)

// List of features and their default value, protected by fMu
@@ -39,6 +42,7 @@ var features = map[FeatureFlag]bool{
EnforceChallengeDisable: false, // deprecated
TLSSNIRevalidation: false,
CancelCTSubmissions: true,
EnforceV2ContentType: false,
}

var fMu = new(sync.RWMutex)
@@ -169,6 +169,16 @@ func ContentLengthRequired() *ProblemDetails {
}
}

// InvalidContentType returns a ProblemDetails suitable for a missing
// ContentType header, or an incorrect ContentType header
func InvalidContentType(detail string) *ProblemDetails {
return &ProblemDetails{
Type: MalformedProblem,
Detail: detail,
HTTPStatus: http.StatusUnsupportedMediaType,
}
}

// InvalidEmail returns a ProblemDetails representing an invalid email address
// error
func InvalidEmail(detail string) *ProblemDetails {
@@ -19,10 +19,14 @@ import (

"github.com/letsencrypt/boulder/core"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/features"
"github.com/letsencrypt/boulder/probs"
"github.com/letsencrypt/boulder/web"
)

// POST requests with a JWS body must have the following Content-Type header
const expectedJWSContentType = "application/jose+json"

var sigAlgErr = errors.New("no signature algorithms suitable for given key type")

func sigAlgorithmForECDSAKey(key *ecdsa.PublicKey) (jose.SignatureAlgorithm, error) {
@@ -142,6 +146,23 @@ func (wfe *WebFrontEndImpl) validPOSTRequest(request *http.Request) *probs.Probl
return probs.ContentLengthRequired()
}

if features.Enabled(features.EnforceV2ContentType) {
// Per 6.2 ALL POSTs should have the correct JWS Content-Type for flattened
// JSON serialization.
if _, present := request.Header["Content-Type"]; !present {
wfe.stats.httpErrorCount.With(prometheus.Labels{"type": "NoContentType"}).Inc()
return probs.InvalidContentType(fmt.Sprintf(
"No Content-Type header on POST. "+
"Content-Type must be %q", expectedJWSContentType))
}
if contentType := request.Header.Get("Content-Type"); contentType != expectedJWSContentType {
wfe.stats.httpErrorCount.With(prometheus.Labels{"type": "WrongContentType"}).Inc()
return probs.InvalidContentType(fmt.Sprintf(
"Invalid Content-Type header on POST. "+
"Content-Type must be %q", expectedJWSContentType))
}
}

// Per 6.4.1 "Replay-Nonce" clients should not send a Replay-Nonce header in
// the HTTP request, it needs to be part of the signed JWS request body
if _, present := request.Header["Replay-Nonce"]; present {
@@ -12,6 +12,7 @@ import (
"golang.org/x/net/context"

"github.com/letsencrypt/boulder/core"
"github.com/letsencrypt/boulder/features"
"github.com/letsencrypt/boulder/mocks"
"github.com/letsencrypt/boulder/probs"
"github.com/letsencrypt/boulder/test"
@@ -364,12 +365,13 @@ func TestValidPOSTRequest(t *testing.T) {
dummyContentLength := []string{"pretty long, idk, maybe a nibble or two?"}

testCases := []struct {
Name string
Headers map[string][]string
Body *string
HTTPStatus int
ProblemDetail string
ErrorStatType string
Name string
Headers map[string][]string
Body *string
HTTPStatus int
ProblemDetail string
ErrorStatType string
EnforceContentType bool
}{
// POST requests without a Content-Length should produce a problem
{
@@ -400,6 +402,31 @@ func TestValidPOSTRequest(t *testing.T) {
ProblemDetail: "No body on POST",
ErrorStatType: "NoPOSTBody",
},
{
Name: "POST without a Content-Type header",
Headers: map[string][]string{
"Content-Length": dummyContentLength,
},
HTTPStatus: http.StatusUnsupportedMediaType,
ProblemDetail: fmt.Sprintf(
"No Content-Type header on POST. Content-Type must be %q",
expectedJWSContentType),
ErrorStatType: "NoContentType",
EnforceContentType: true,
},
{
Name: "POST with an invalid Content-Type header",
Headers: map[string][]string{
"Content-Length": dummyContentLength,
"Content-Type": []string{"fresh.and.rare"},
},
HTTPStatus: http.StatusUnsupportedMediaType,
ProblemDetail: fmt.Sprintf(
"Invalid Content-Type header on POST. Content-Type must be %q",
expectedJWSContentType),
ErrorStatType: "WrongContentType",
EnforceContentType: true,
},
}

for _, tc := range testCases {
@@ -409,6 +436,10 @@ func TestValidPOSTRequest(t *testing.T) {
Header: tc.Headers,
}
t.Run(tc.Name, func(t *testing.T) {
_ = features.Set(map[string]bool{
"EnforceV2ContentType": tc.EnforceContentType,
})
defer features.Reset()
prob := wfe.validPOSTRequest(input)
test.Assert(t, prob != nil, "No error returned for invalid POST")
test.AssertEquals(t, prob.Type, probs.MalformedProblem)

2 comments on commit 3062662

@Kencdk

This comment has been minimized.

Kencdk replied Mar 29, 2018

With this change, the content-type is enforced to have an exact match of application/jose+json - not allowing for any charset to be specified. (eg: Content-Type: application/jose+json; charset=utf-8)

Is this by design or should it be considered a bug?

Suggestion: parse the content-type header, extract the mime part and validate it accordingly - additionally consider validating charset to be in valid ranges.

@cpu

This comment has been minimized.

Member

cpu replied Mar 29, 2018

Hi @Kencdk,

In the future it would be helpful if you could favour creating new issues instead of commenting on already merged commits. It's very easy to miss these comments and hard to track work against them. I opened #3603 with your comment text.

Thanks!

Please sign in to comment.