letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

csr: return berrors in VerifyCSR. (#4473) 

This also adds the badCSR error type specified by RFC 8555. It is a natural fit for the errors in VerifyCSR that aren't covered by badPublicKey. The web package function for converting a berror to
a problem is updated for the new badCSR error type.

The callers (RA and CA) are updated to return the berrors from VerifyCSR as is instead of unconditionally wrapping them as a berrors.MalformedError instance. Unit/integration tests are updated accordingly.

Resolves #4418
This commit is contained in:
Daniel McCarney 2019-10-09 20:11:11 -04:00 committed by Roland Bracewell Shoemaker
parent ddfc620c44
commit ecca3492e9
9 changed files with 47 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
ca/ca.go
View File

@ -595,7 +595,9 @@ func (ca *CertificateAuthorityImpl) issuePrecertificateInner(ctx context.Context
*issueReq.RegistrationID,
); err != nil {
ca.log.AuditErr(err.Error())
return nil, berrors.MalformedError(err.Error())
// VerifyCSR returns berror instances that can be passed through as-is
// without wrapping.
return nil, err
}
extensions, err := ca.extensionsFromCSR(csr)

15
ca/ca_test.go
View File

@ -588,6 +588,7 @@ func TestInvalidCSRs(t *testing.T) {
csrPath string
check func(t *testing.T, ca *CertificateAuthorityImpl, sa *mockSA)
errorMessage string
errorType berrors.ErrorType
}{
// Test that the CA rejects CSRs that have no names.
//
@ -595,7 +596,7 @@ func TestInvalidCSRs(t *testing.T) {
// * Random RSA public key.
// * CN = [none]
// * DNSNames = [none]
{"RejectNoHostnames", "./testdata/no_names.der.csr", nil, "Issued certificate with no names"},
{"RejectNoHostnames", "./testdata/no_names.der.csr", nil, "Issued certificate with no names", berrors.BadCSR},
// Test that the CA rejects CSRs that have too many names.
//
@ -603,7 +604,7 @@ func TestInvalidCSRs(t *testing.T) {
// * Random public key
// * CN = [none]
// * DNSNames = not-example.com, www.not-example.com, mail.example.com
{"RejectTooManyHostnames", "./testdata/too_many_names.der.csr", nil, "Issued certificate with too many names"},
{"RejectTooManyHostnames", "./testdata/too_many_names.der.csr", nil, "Issued certificate with too many names", berrors.BadCSR},
// Test that the CA rejects CSRs that have public keys that are too short.
//
@ -611,23 +612,23 @@ func TestInvalidCSRs(t *testing.T) {
// * Random public key -- 512 bits long
// * CN = (none)
// * DNSNames = not-example.com, www.not-example.com, mail.not-example.com
{"RejectShortKey", "./testdata/short_key.der.csr", nil, "Issued a certificate with too short a key."},
{"RejectShortKey", "./testdata/short_key.der.csr", nil, "Issued a certificate with too short a key.", berrors.BadPublicKey},
// CSR generated by Go:
// * Random RSA public key.
// * CN = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com
// * DNSNames = [none]
{"RejectLongCommonName", "./testdata/long_cn.der.csr", nil, "Issued a certificate with a CN over 64 bytes."},
{"RejectLongCommonName", "./testdata/long_cn.der.csr", nil, "Issued a certificate with a CN over 64 bytes.", berrors.BadCSR},
// CSR generated by OpenSSL:
// Edited signature to become invalid.
{"RejectWrongSignature", "./testdata/invalid_signature.der.csr", nil, "Issued a certificate based on a CSR with an invalid signature."},
{"RejectWrongSignature", "./testdata/invalid_signature.der.csr", nil, "Issued a certificate based on a CSR with an invalid signature.", berrors.BadCSR},
// CSR generated by Go:
// * Random public key
// * CN = not-example.com
// * Includes an extensionRequest attribute for an empty TLS Feature extension
{"TLSFeatureUnknown", "./testdata/tls_feature_unknown.der.csr", issueCertificateSubTestTLSFeatureUnknown, "Issued a certificate based on a CSR with an empty TLS feature extension."},
{"TLSFeatureUnknown", "./testdata/tls_feature_unknown.der.csr", issueCertificateSubTestTLSFeatureUnknown, "Issued a certificate based on a CSR with an empty TLS feature extension.", berrors.Malformed},
}
for _, testCase := range testCases {
@ -650,7 +651,7 @@ func TestInvalidCSRs(t *testing.T) {
issueReq := &caPB.IssueCertificateRequest{Csr: serializedCSR, RegistrationID: &arbitraryRegID}
_, err = ca.IssuePrecertificate(ctx, issueReq)
test.Assert(t, berrors.Is(err, berrors.Malformed), "Incorrect error type returned")
test.Assert(t, berrors.Is(err, testCase.errorType), "Incorrect error type returned")
test.AssertEquals(t, signatureCountByPurpose("cert", ca.signatureCount), 0)
test.AssertError(t, err, testCase.errorMessage)

23
csr/csr.go
View File

@ -3,11 +3,10 @@ package csr
import (
"crypto"
"crypto/x509"
"errors"
"fmt"
"strings"
"github.com/letsencrypt/boulder/core"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/goodkey"
"github.com/letsencrypt/boulder/identifier"
)
@ -33,13 +32,13 @@ var goodSignatureAlgorithms = map[x509.SignatureAlgorithm]bool{
}
var (
invalidPubKey = errors.New("invalid public key in CSR")
unsupportedSigAlg = errors.New("signature algorithm not supported")
invalidSig = errors.New("invalid signature on CSR")
invalidEmailPresent = errors.New("CSR contains one or more email address fields")
invalidIPPresent = errors.New("CSR contains one or more IP address fields")
invalidNoDNS = errors.New("at least one DNS name is required")
invalidAllSANTooLong = errors.New("CSR doesn't contain a SAN short enough to fit in CN")
invalidPubKey = berrors.BadPublicKeyError("invalid public key in CSR")
unsupportedSigAlg = berrors.BadCSRError("signature algorithm not supported")
invalidSig = berrors.BadCSRError("invalid signature on CSR")
invalidEmailPresent = berrors.BadCSRError("CSR contains one or more email address fields")
invalidIPPresent = berrors.BadCSRError("CSR contains one or more IP address fields")
invalidNoDNS = berrors.BadCSRError("at least one DNS name is required")
invalidAllSANTooLong = berrors.BadCSRError("CSR doesn't contain a SAN short enough to fit in CN")
)
// VerifyCSR checks the validity of a x509.CertificateRequest. Before doing checks it normalizes
@ -52,7 +51,7 @@ func VerifyCSR(csr *x509.CertificateRequest, maxNames int, keyPolicy *goodkey.Ke
return invalidPubKey
}
if err := keyPolicy.GoodKey(key); err != nil {
return fmt.Errorf("invalid public key in CSR: %s", err)
return berrors.BadPublicKeyError("invalid public key in CSR: %s", err)
}
if !goodSignatureAlgorithms[csr.SignatureAlgorithm] {
return unsupportedSigAlg
@ -73,10 +72,10 @@ func VerifyCSR(csr *x509.CertificateRequest, maxNames int, keyPolicy *goodkey.Ke
return invalidAllSANTooLong
}
if len(csr.Subject.CommonName) > maxCNLength {
return fmt.Errorf("CN was longer than %d bytes", maxCNLength)
return berrors.BadCSRError("CN was longer than %d bytes", maxCNLength)
}
if len(csr.DNSNames) > maxNames {
return fmt.Errorf("CSR contains more than %d DNS names", maxNames)
return berrors.BadCSRError("CSR contains more than %d DNS names", maxNames)
}
idents := make([]identifier.ACMEIdentifier, len(csr.DNSNames))
for i, dnsName := range csr.DNSNames {

5
csr/csr_test.go
View File

@ -11,6 +11,7 @@ import (
"testing"
"github.com/letsencrypt/boulder/core"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/goodkey"
"github.com/letsencrypt/boulder/identifier"
"github.com/letsencrypt/boulder/test"
@ -120,7 +121,7 @@ func TestVerifyCSR(t *testing.T) {
testingPolicy,
&mockPA{},
0,
errors.New("CN was longer than 64 bytes"),
berrors.BadCSRError("CN was longer than %d bytes", maxCNLength),
},
{
signedReqWithHosts,
@ -128,7 +129,7 @@ func TestVerifyCSR(t *testing.T) {
testingPolicy,
&mockPA{},
0,
errors.New("CSR contains more than 1 DNS names"),
berrors.BadCSRError("CSR contains more than 1 DNS names"),
},
{
signedReqWithBadNames,

5
errors/errors.go
View File

@ -26,6 +26,7 @@ const (
OrderNotReady
DNS
BadPublicKey
BadCSR
)
// BoulderError represents internal Boulder errors
@ -135,3 +136,7 @@ func DNSError(msg string, args ...interface{}) error {
func BadPublicKeyError(msg string, args ...interface{}) error {
return New(BadPublicKey, msg, args...)
}
func BadCSRError(msg string, args ...interface{}) error {
return New(BadCSR, msg, args...)
}

10
probs/probs.go
View File

@ -26,6 +26,7 @@ const (
BadSignatureAlgorithmProblem = ProblemType("badSignatureAlgorithm")
BadPublicKeyProblem = ProblemType("badPublicKey")
BadRevocationReasonProblem = ProblemType("badRevocationReason")
BadCSRProblem = ProblemType("badCSR")
V1ErrorNS = "urn:acme:error:"
V2ErrorNS = "urn:ietf:params:acme:error:"
@ -321,3 +322,12 @@ func BadRevocationReason(detail string, a ...interface{}) *ProblemDetails {
HTTPStatus: http.StatusBadRequest,
}
}
// BadCSR returns a ProblemDetails representing a BadCSRProblem.
func BadCSR(detail string, a ...interface{}) *ProblemDetails {
return &ProblemDetails{
Type: BadCSRProblem,
Detail: fmt.Sprintf(detail, a...),
HTTPStatus: http.StatusBadRequest,
}
}

4
ra/ra.go
View File

@ -1035,7 +1035,9 @@ func (ra *RegistrationAuthorityImpl) FinalizeOrder(ctx context.Context, req *rap
}
if err := csrlib.VerifyCSR(csrOb, ra.maxNames, &ra.keyPolicy, ra.PA, ra.forceCNFromSAN, *req.Order.RegistrationID); err != nil {
return nil, berrors.MalformedError(err.Error())
// VerifyCSR returns berror instances that can be passed through as-is
// without wrapping.
return nil, err
}
// Dedupe, lowercase and sort both the names from the CSR and the names in the

9
test/v2_integration.py
View File

@ -1129,7 +1129,7 @@ def test_long_san_no_cn():
# if we get to this raise the auth_and_issue call didn't fail, so fail the test
raise Exception("Issuance didn't fail when the only SAN in a certificate was longer than the max CN length")
except messages.Error as e:
if e.typ != "urn:ietf:params:acme:error:malformed":
if e.typ != "urn:ietf:params:acme:error:badCSR":
raise Exception('Expected malformed type problem, got {0}'.format(e.typ))
if e.detail != 'Error finalizing order :: issuing precertificate: CSR doesn\'t contain a SAN short enough to fit in CN':
raise Exception('Problem detail did not match expected')
@ -1252,17 +1252,12 @@ def test_blocked_key_cert():
try:
order = client.poll_and_finalize(order)
except acme_errors.Error as e:
# TODO(@cpu): this _should_ be type
# urn:ietf:params:acme:error:badPublicKey but this will require
# refactoring the `csr` package error handling.
# See https://github.com/letsencrypt/boulder/issues/4418
if e.typ != "urn:ietf:params:acme:error:malformed":
if e.typ != "urn:ietf:params:acme:error:badPublicKey":
raise Exception("problem did not have correct error type, had {0}".format(e.typ))
if e.detail != "Error finalizing order :: invalid public key in CSR: public key is forbidden":
raise Exception("problem did not have correct error detail, had {0}".format(e.detail))
testPass = True
if testPass is False:
raise Exception("expected cert creation to fail with Error when using blocked key")

2
web/probs.go
View File

@ -37,6 +37,8 @@ func problemDetailsForBoulderError(err *berrors.BoulderError, msg string) *probs
outProb = probs.OrderNotReady("%s :: %s", msg, err)
case berrors.BadPublicKey:
outProb = probs.BadPublicKey("%s :: %s", msg, err)
case berrors.BadCSR:
outProb = probs.BadCSR("%s :: %s", msg, err)
default:
// Internal server error messages may include sensitive data, so we do
// not include it.