From 979e0f8f51f7d49869b1d38e36368e60aedc9dad Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Fri, 28 Jul 2023 14:25:17 +0200
Subject: [PATCH 01/11] Add error details to select error cases for `apple`
 format

---
 acme/challenge.go | 36 ++++++++++++++++++++++++++++--------
 acme/errors.go    | 14 +++++++++++---
 2 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index e43b15b4..843bdbb4 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -30,6 +30,7 @@ import (
 	"golang.org/x/exp/slices"
 
 	"github.com/smallstep/go-attestation/attest"
+
 	"go.step.sm/crypto/jose"
 	"go.step.sm/crypto/keyutil"
 	"go.step.sm/crypto/pemutil"
@@ -398,6 +399,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 			}
 			return WrapErrorISE(err, "error validating attestation")
 		}
+
 		// Validate nonce with SHA-256 of the token.
 		if len(data.Nonce) != 0 {
 			sum := sha256.Sum256([]byte(ch.Token))
@@ -410,8 +412,26 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		// identifiers.
 		//
 		// Note: We might want to use an external service for this.
+		var subproblem *Subproblem
+		switch {
+		case data.UDID != ch.Value:
+			s := NewSubproblemWithIdentifier(
+				ErrorMalformedType,
+				Identifier{Type: "permanent-identifier", Value: ch.Value},
+				"challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.UDID,
+			)
+			subproblem = &s
+		case data.SerialNumber != ch.Value:
+			s := NewSubproblemWithIdentifier(
+				ErrorMalformedType,
+				Identifier{Type: "permanent-identifier", Value: ch.Value},
+				"challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.SerialNumber,
+			)
+			subproblem = &s
+		}
+
 		if data.UDID != ch.Value && data.SerialNumber != ch.Value {
-			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match"))
+			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(*subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
@@ -838,30 +858,30 @@ func doAppleAttestationFormat(_ context.Context, prov Provisioner, _ *Challenge,
 
 	x5c, ok := att.AttStatement["x5c"].([]interface{})
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
 	}
 	if len(x5c) == 0 {
-		return nil, NewError(ErrorRejectedIdentifierType, "x5c is empty")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c is empty").WithAdditionalErrorDetail()
 	}
 
 	der, ok := x5c[0].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
 	}
 	leaf, err := x509.ParseCertificate(der)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
 	}
 
 	intermediates := x509.NewCertPool()
 	for _, v := range x5c[1:] {
 		der, ok = v.([]byte)
 		if !ok {
-			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed")
+			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
 		}
 		cert, err := x509.ParseCertificate(der)
 		if err != nil {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed")
+			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
 		}
 		intermediates.AddCert(cert)
 	}
@@ -872,7 +892,7 @@ func doAppleAttestationFormat(_ context.Context, prov Provisioner, _ *Challenge,
 		CurrentTime:   time.Now().Truncate(time.Second),
 		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageAny},
 	}); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid").WithAdditionalErrorDetail()
 	}
 
 	data := &appleAttestationData{
diff --git a/acme/errors.go b/acme/errors.go
index 44f367a0..e5baf87a 100644
--- a/acme/errors.go
+++ b/acme/errors.go
@@ -293,6 +293,11 @@ type Subproblem struct {
 	Identifier *Identifier `json:"identifier,omitempty"`
 }
 
+// NewError creates a new Error.
+func NewError(pt ProblemType, msg string, args ...interface{}) *Error {
+	return newError(pt, errors.Errorf(msg, args...))
+}
+
 // AddSubproblems adds the Subproblems to Error. It
 // returns the Error, allowing for fluent addition.
 func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error {
@@ -300,9 +305,12 @@ func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error {
 	return e
 }
 
-// NewError creates a new Error type.
-func NewError(pt ProblemType, msg string, args ...interface{}) *Error {
-	return newError(pt, errors.Errorf(msg, args...))
+// WithAdditionalErrorDetail adds the underlying error
+// to the existing (default) ACME error detail, providing
+// more information to the ACME client.
+func (e *Error) WithAdditionalErrorDetail() *Error {
+	e.Detail = fmt.Sprintf("%s: %s", e.Detail, e.Err)
+	return e
 }
 
 // NewSubproblem creates a new Subproblem. The msg and args

From d5dd8feccd81da0b9e69aee87614e0bdbd760bf5 Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Fri, 28 Jul 2023 14:39:35 +0200
Subject: [PATCH 02/11] Prevent internal errors from being returned to ACME
 clients

---
 acme/errors.go | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/acme/errors.go b/acme/errors.go
index e5baf87a..59bd2e11 100644
--- a/acme/errors.go
+++ b/acme/errors.go
@@ -309,6 +309,12 @@ func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error {
 // to the existing (default) ACME error detail, providing
 // more information to the ACME client.
 func (e *Error) WithAdditionalErrorDetail() *Error {
+	// prevent internal server errors from disclosing
+	// the internal error to the client.
+	if e.Status >= 500 {
+		return e
+	}
+
 	e.Detail = fmt.Sprintf("%s: %s", e.Detail, e.Err)
 	return e
 }

From 9cbbd1d575f017758a94e0a5cad34ed745ead123 Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Fri, 28 Jul 2023 16:28:31 +0200
Subject: [PATCH 03/11] Add error details to ACME `tpm` format validation
 errors

---
 acme/challenge.go                   | 76 ++++++++++++-----------------
 acme/challenge_test.go              | 10 +++-
 acme/challenge_tpmsimulator_test.go |  5 +-
 3 files changed, 43 insertions(+), 48 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index 843bdbb4..e8870077 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -412,26 +412,13 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		// identifiers.
 		//
 		// Note: We might want to use an external service for this.
-		var subproblem *Subproblem
-		switch {
-		case data.UDID != ch.Value:
-			s := NewSubproblemWithIdentifier(
-				ErrorMalformedType,
-				Identifier{Type: "permanent-identifier", Value: ch.Value},
-				"challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.UDID,
-			)
-			subproblem = &s
-		case data.SerialNumber != ch.Value:
-			s := NewSubproblemWithIdentifier(
+		if data.UDID != ch.Value && data.SerialNumber != ch.Value {
+			subproblem := NewSubproblemWithIdentifier(
 				ErrorMalformedType,
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
-				"challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.SerialNumber,
+				"challenge identifier %q doesn't match any of the attested hardware identifiers %s", ch.Value, []string{data.UDID, data.SerialNumber},
 			)
-			subproblem = &s
-		}
-
-		if data.UDID != ch.Value && data.SerialNumber != ch.Value {
-			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(*subproblem))
+			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
@@ -459,7 +446,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
 				"challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.SerialNumber,
 			)
-			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(subproblem))
+			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
@@ -491,7 +478,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
 				"challenge identifier %q doesn't match any of the attested hardware identifiers %q", ch.Value, data.PermanentIdentifiers,
 			)
-			return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "permanent identifier does not match").AddSubproblems(subproblem))
+			return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
@@ -543,38 +530,38 @@ const (
 func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge, jwk *jose.JSONWebKey, att *attestationObject) (*tpmAttestationData, error) {
 	ver, ok := att.AttStatement["ver"].(string)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "ver not present")
+		return nil, NewError(ErrorBadAttestationStatementType, "ver not present").WithAdditionalErrorDetail()
 	}
 	if ver != "2.0" {
-		return nil, NewError(ErrorBadAttestationStatementType, "version %q is not supported", ver)
+		return nil, NewError(ErrorBadAttestationStatementType, "version %q is not supported", ver).WithAdditionalErrorDetail()
 	}
 
 	x5c, ok := att.AttStatement["x5c"].([]interface{})
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
 	}
 	if len(x5c) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is empty")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c is empty").WithAdditionalErrorDetail()
 	}
 
 	akCertBytes, ok := x5c[0].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
 	}
 	akCert, err := x509.ParseCertificate(akCertBytes)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
 	}
 
 	intermediates := x509.NewCertPool()
 	for _, v := range x5c[1:] {
 		intCertBytes, vok := v.([]byte)
 		if !vok {
-			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed")
+			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
 		}
 		intCert, err := x509.ParseCertificate(intCertBytes)
 		if err != nil {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed")
+			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
 		}
 		intermediates.AddCert(intCert)
 	}
@@ -612,19 +599,19 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageAny},
 	})
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid").WithAdditionalErrorDetail()
 	}
 
 	// validate additional AK certificate requirements
 	if err := validateAKCertificate(akCert); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "AK certificate is not valid")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "AK certificate is not valid").WithAdditionalErrorDetail()
 	}
 
 	// TODO(hs): implement revocation check; Verify() doesn't perform CRL check nor OCSP lookup.
 
 	sans, err := x509util.ParseSubjectAlternativeNames(akCert)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed parsing AK certificate Subject Alternative Names")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed parsing AK certificate Subject Alternative Names").WithAdditionalErrorDetail()
 	}
 
 	permanentIdentifiers := make([]string, len(sans.PermanentIdentifiers))
@@ -635,37 +622,37 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	// extract and validate pubArea, sig, certInfo and alg properties from the request body
 	pubArea, ok := att.AttStatement["pubArea"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid pubArea in attestation statement")
+		return nil, NewError(ErrorBadAttestationStatementType, "invalid pubArea in attestation statement").WithAdditionalErrorDetail()
 	}
 	if len(pubArea) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "pubArea is empty")
+		return nil, NewError(ErrorBadAttestationStatementType, "pubArea is empty").WithAdditionalErrorDetail()
 	}
 
 	sig, ok := att.AttStatement["sig"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid sig in attestation statement")
+		return nil, NewError(ErrorBadAttestationStatementType, "invalid sig in attestation statement").WithAdditionalErrorDetail()
 	}
 	if len(sig) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "sig is empty")
+		return nil, NewError(ErrorBadAttestationStatementType, "sig is empty").WithAdditionalErrorDetail()
 	}
 
 	certInfo, ok := att.AttStatement["certInfo"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid certInfo in attestation statement")
+		return nil, NewError(ErrorBadAttestationStatementType, "invalid certInfo in attestation statement").WithAdditionalErrorDetail()
 	}
 	if len(certInfo) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "certInfo is empty")
+		return nil, NewError(ErrorBadAttestationStatementType, "certInfo is empty").WithAdditionalErrorDetail()
 	}
 
 	alg, ok := att.AttStatement["alg"].(int64)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid alg in attestation statement")
+		return nil, NewError(ErrorBadAttestationStatementType, "invalid alg in attestation statement").WithAdditionalErrorDetail()
 	}
 
 	// only RS256 and ES256 are allowed
 	coseAlg := coseAlgorithmIdentifier(alg)
 	if coseAlg != coseAlgRS256 && coseAlg != coseAlgES256 {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid alg %d in attestation statement", alg)
+		return nil, NewError(ErrorBadAttestationStatementType, "invalid alg %d in attestation statement", alg).WithAdditionalErrorDetail()
 	}
 
 	// set the hash algorithm to use to SHA256
@@ -683,36 +670,37 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 		Hash:   hash,
 	}
 	if err = certificationParameters.Verify(verifyOpts); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "invalid certification parameters")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "invalid certification parameters").WithAdditionalErrorDetail()
 	}
 
 	// decode the "certInfo" data. This won't fail, as it's also done as part of Verify().
 	tpmCertInfo, err := tpm2.DecodeAttestationData(certInfo)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed decoding attestation data")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed decoding attestation data").WithAdditionalErrorDetail()
 	}
 
 	keyAuth, err := KeyAuthorization(ch.Token, jwk)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed creating key auth digest")
+		return nil, WrapErrorISE(err, "failed creating key auth digest")
 	}
 	hashedKeyAuth := sha256.Sum256([]byte(keyAuth))
 
 	// verify the WebAuthn object contains the expect key authorization digest, which is carried
 	// within the encoded `certInfo` property of the attestation statement.
 	if subtle.ConstantTimeCompare(hashedKeyAuth[:], []byte(tpmCertInfo.ExtraData)) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "key authorization does not match")
+		return nil, NewError(ErrorBadAttestationStatementType, "key authorization invalid").WithAdditionalErrorDetail()
 	}
 
 	// decode the (attested) public key and determine its fingerprint.  This won't fail, as it's also done as part of Verify().
 	pub, err := tpm2.DecodePublic(pubArea)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed decoding pubArea")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed decoding pubArea").WithAdditionalErrorDetail()
 	}
 
 	publicKey, err := pub.Key()
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed getting public key")
+		// TODO(hs): to return the detail or not? Is it just internal at this point?
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed getting public key").WithAdditionalErrorDetail()
 	}
 
 	data := &tpmAttestationData{
diff --git a/acme/challenge_test.go b/acme/challenge_test.go
index 74ff363c..1f5135ca 100644
--- a/acme/challenge_test.go
+++ b/acme/challenge_test.go
@@ -3532,7 +3532,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "x5c not present")
+							err := NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -3625,7 +3625,12 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "non-matching-value", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "permanent identifier does not match")
+							subproblem := NewSubproblemWithIdentifier(
+								ErrorMalformedType,
+								Identifier{Type: "permanent-identifier", Value: "non-matching-value"},
+								`challenge identifier "non-matching-value" doesn't match any of the attested hardware identifiers [udid serial-number]`,
+							)
+							err := NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem)
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -3753,6 +3758,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, "12345678", updch.Value)
 
 							err := NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").
+								WithAdditionalErrorDetail().
 								AddSubproblems(NewSubproblemWithIdentifier(
 									ErrorMalformedType,
 									Identifier{Type: "permanent-identifier", Value: "12345678"},
diff --git a/acme/challenge_tpmsimulator_test.go b/acme/challenge_tpmsimulator_test.go
index cb893b14..18a87e2a 100644
--- a/acme/challenge_tpmsimulator_test.go
+++ b/acme/challenge_tpmsimulator_test.go
@@ -237,7 +237,7 @@ func Test_deviceAttest01ValidateWithTPMSimulator(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "device.id.12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, `version "bogus" is not supported`)
+							err := NewError(ErrorBadAttestationStatementType, `version "bogus" is not supported`).WithAdditionalErrorDetail()
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -283,6 +283,7 @@ func Test_deviceAttest01ValidateWithTPMSimulator(t *testing.T) {
 							assert.Equal(t, "device.id.99999999", updch.Value)
 
 							err := NewError(ErrorRejectedIdentifierType, `permanent identifier does not match`).
+								WithAdditionalErrorDetail().
 								AddSubproblems(NewSubproblemWithIdentifier(
 									ErrorMalformedType,
 									Identifier{Type: "permanent-identifier", Value: "device.id.99999999"},
@@ -828,7 +829,7 @@ func Test_doTPMAttestationFormat(t *testing.T) {
 				"certInfo": params.CreateAttestation,
 				"pubArea":  params.Public,
 			},
-		}}, nil, newBadAttestationStatementError("key authorization does not match")},
+		}}, nil, newBadAttestationStatementError("key authorization invalid")},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

From dd9bf1e91533527891b2d2e373697a1b42bf9ff1 Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Fri, 28 Jul 2023 16:59:34 +0200
Subject: [PATCH 04/11] Add error details for the `step` format

---
 acme/challenge.go      | 28 ++++++++++++++--------------
 acme/challenge_test.go |  2 +-
 acme/errors.go         |  4 ++--
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index e8870077..d65c262c 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -952,28 +952,28 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	// Extract x5c and verify certificate
 	x5c, ok := att.AttStatement["x5c"].([]interface{})
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
 	}
 	if len(x5c) == 0 {
-		return nil, NewError(ErrorRejectedIdentifierType, "x5c is empty")
+		return nil, NewError(ErrorRejectedIdentifierType, "x5c is empty").WithAdditionalErrorDetail()
 	}
 	der, ok := x5c[0].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed")
+		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
 	}
 	leaf, err := x509.ParseCertificate(der)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
 	}
 	intermediates := x509.NewCertPool()
 	for _, v := range x5c[1:] {
 		der, ok = v.([]byte)
 		if !ok {
-			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed")
+			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
 		}
 		cert, err := x509.ParseCertificate(der)
 		if err != nil {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed")
+			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
 		}
 		intermediates.AddCert(cert)
 	}
@@ -983,7 +983,7 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 		CurrentTime:   time.Now().Truncate(time.Second),
 		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageAny},
 	}); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid")
+		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid").WithAdditionalErrorDetail()
 	}
 
 	// Verify proof of possession of private key validating the key
@@ -993,10 +993,10 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	var sig []byte
 	csig, ok := att.AttStatement["sig"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "sig not present")
+		return nil, NewError(ErrorBadAttestationStatementType, "sig not present").WithAdditionalErrorDetail()
 	}
 	if err := cbor.Unmarshal(csig, &sig); err != nil {
-		return nil, NewError(ErrorBadAttestationStatementType, "sig is malformed")
+		return nil, NewError(ErrorBadAttestationStatementType, "sig is malformed").WithAdditionalErrorDetail()
 	}
 	keyAuth, err := KeyAuthorization(ch.Token, jwk)
 	if err != nil {
@@ -1006,23 +1006,23 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	switch pub := leaf.PublicKey.(type) {
 	case *ecdsa.PublicKey:
 		if pub.Curve != elliptic.P256() {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "unsupported elliptic curve %s", pub.Curve)
+			return nil, WrapError(ErrorBadAttestationStatementType, err, "unsupported elliptic curve %s", pub.Curve).WithAdditionalErrorDetail()
 		}
 		sum := sha256.Sum256([]byte(keyAuth))
 		if !ecdsa.VerifyASN1(pub, sum[:], sig) {
-			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature")
+			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature").WithAdditionalErrorDetail()
 		}
 	case *rsa.PublicKey:
 		sum := sha256.Sum256([]byte(keyAuth))
 		if err := rsa.VerifyPKCS1v15(pub, crypto.SHA256, sum[:], sig); err != nil {
-			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature")
+			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature").WithAdditionalErrorDetail()
 		}
 	case ed25519.PublicKey:
 		if !ed25519.Verify(pub, []byte(keyAuth), sig) {
-			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature")
+			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature").WithAdditionalErrorDetail()
 		}
 	default:
-		return nil, NewError(ErrorBadAttestationStatementType, "unsupported public key type %T", pub)
+		return nil, NewError(ErrorBadAttestationStatementType, "unsupported public key type %T", pub).WithAdditionalErrorDetail()
 	}
 
 	// Parse attestation data:
diff --git a/acme/challenge_test.go b/acme/challenge_test.go
index 1f5135ca..e489aac7 100644
--- a/acme/challenge_test.go
+++ b/acme/challenge_test.go
@@ -3703,7 +3703,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "x5c not present")
+							err := NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
diff --git a/acme/errors.go b/acme/errors.go
index 59bd2e11..06b45114 100644
--- a/acme/errors.go
+++ b/acme/errors.go
@@ -310,8 +310,8 @@ func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error {
 // more information to the ACME client.
 func (e *Error) WithAdditionalErrorDetail() *Error {
 	// prevent internal server errors from disclosing
-	// the internal error to the client.
-	if e.Status >= 500 {
+	// the internal error to the client at all times.
+	if e == nil || e.Status >= 500 {
 		return e
 	}
 

From df22b8a30338ee7a7b2209838fc5598464249a5f Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Mon, 31 Jul 2023 11:59:26 +0200
Subject: [PATCH 05/11] Cleanup some leftover TODOs

---
 acme/challenge.go | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index d65c262c..74c92ed3 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -455,8 +455,6 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 	case "tpm":
 		data, err := doTPMAttestationFormat(ctx, prov, ch, jwk, &att)
 		if err != nil {
-			// TODO(hs): we should provide more details in the error reported to the client;
-			// "Attestation statement cannot be verified" is VERY generic. Also holds true for the other formats.
 			var acmeError *Error
 			if errors.As(err, &acmeError) {
 				if acmeError.Status == 500 {
@@ -699,7 +697,6 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 
 	publicKey, err := pub.Key()
 	if err != nil {
-		// TODO(hs): to return the detail or not? Is it just internal at this point?
 		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed getting public key").WithAdditionalErrorDetail()
 	}
 

From 0d3338ff3aa52242a610c69b4498036204614d6f Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Mon, 31 Jul 2023 12:11:50 +0200
Subject: [PATCH 06/11] Return consistent ACME error types for specific cases

---
 acme/challenge.go                   | 10 +++++-----
 acme/challenge_test.go              |  4 ++--
 acme/challenge_tpmsimulator_test.go |  6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index 74c92ed3..f0ed726a 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -414,7 +414,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		// Note: We might want to use an external service for this.
 		if data.UDID != ch.Value && data.SerialNumber != ch.Value {
 			subproblem := NewSubproblemWithIdentifier(
-				ErrorMalformedType,
+				ErrorRejectedIdentifierType,
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
 				"challenge identifier %q doesn't match any of the attested hardware identifiers %s", ch.Value, []string{data.UDID, data.SerialNumber},
 			)
@@ -442,7 +442,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		// Note: We might want to use an external service for this.
 		if data.SerialNumber != ch.Value {
 			subproblem := NewSubproblemWithIdentifier(
-				ErrorMalformedType,
+				ErrorRejectedIdentifierType,
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
 				"challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.SerialNumber,
 			)
@@ -472,11 +472,11 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		// still fail if the challenge value isn't equal to the CSR subject.
 		if len(data.PermanentIdentifiers) > 0 && !slices.Contains(data.PermanentIdentifiers, ch.Value) { // TODO(hs): add support for HardwareModuleName
 			subproblem := NewSubproblemWithIdentifier(
-				ErrorMalformedType,
+				ErrorRejectedIdentifierType,
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
-				"challenge identifier %q doesn't match any of the attested hardware identifiers %q", ch.Value, data.PermanentIdentifiers,
+				"challenge identifier %q doesn't match any of the attested hardware identifiers %s", ch.Value, data.PermanentIdentifiers,
 			)
-			return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
+			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
diff --git a/acme/challenge_test.go b/acme/challenge_test.go
index e489aac7..2fe3653e 100644
--- a/acme/challenge_test.go
+++ b/acme/challenge_test.go
@@ -3626,7 +3626,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, "non-matching-value", updch.Value)
 
 							subproblem := NewSubproblemWithIdentifier(
-								ErrorMalformedType,
+								ErrorRejectedIdentifierType,
 								Identifier{Type: "permanent-identifier", Value: "non-matching-value"},
 								`challenge identifier "non-matching-value" doesn't match any of the attested hardware identifiers [udid serial-number]`,
 							)
@@ -3760,7 +3760,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							err := NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").
 								WithAdditionalErrorDetail().
 								AddSubproblems(NewSubproblemWithIdentifier(
-									ErrorMalformedType,
+									ErrorRejectedIdentifierType,
 									Identifier{Type: "permanent-identifier", Value: "12345678"},
 									"challenge identifier \"12345678\" doesn't match the attested hardware identifier \"87654321\"",
 								))
diff --git a/acme/challenge_tpmsimulator_test.go b/acme/challenge_tpmsimulator_test.go
index 18a87e2a..96381b80 100644
--- a/acme/challenge_tpmsimulator_test.go
+++ b/acme/challenge_tpmsimulator_test.go
@@ -282,12 +282,12 @@ func Test_deviceAttest01ValidateWithTPMSimulator(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "device.id.99999999", updch.Value)
 
-							err := NewError(ErrorRejectedIdentifierType, `permanent identifier does not match`).
+							err := NewError(ErrorBadAttestationStatementType, `permanent identifier does not match`).
 								WithAdditionalErrorDetail().
 								AddSubproblems(NewSubproblemWithIdentifier(
-									ErrorMalformedType,
+									ErrorRejectedIdentifierType,
 									Identifier{Type: "permanent-identifier", Value: "device.id.99999999"},
-									`challenge identifier "device.id.99999999" doesn't match any of the attested hardware identifiers ["device.id.12345678"]`,
+									`challenge identifier "device.id.99999999" doesn't match any of the attested hardware identifiers [device.id.12345678]`,
 								))
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())

From 9a52675865b537d642e306a7254fd348223a9581 Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Mon, 31 Jul 2023 12:29:07 +0200
Subject: [PATCH 07/11] Return descriptive error when using unsupported format

---
 acme/challenge.go      | 13 +++++++++----
 acme/challenge_test.go |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index f0ed726a..a68b4151 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -380,13 +380,18 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		return WrapErrorISE(err, "error unmarshalling CBOR")
 	}
 
+	format := att.Format
 	prov := MustProvisionerFromContext(ctx)
-	if !prov.IsAttestationFormatEnabled(ctx, provisioner.ACMEAttestationFormat(att.Format)) {
+	if !prov.IsAttestationFormatEnabled(ctx, provisioner.ACMEAttestationFormat(format)) {
+		if format != "apple" && format != "step" && format != "tpm" {
+			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "unsupported attestation object format %q", format).WithAdditionalErrorDetail())
+		}
+
 		return storeError(ctx, db, ch, true,
-			NewError(ErrorBadAttestationStatementType, "attestation format %q is not enabled", att.Format))
+			NewError(ErrorBadAttestationStatementType, "attestation format %q is not enabled", format))
 	}
 
-	switch att.Format {
+	switch format {
 	case "apple":
 		data, err := doAppleAttestationFormat(ctx, prov, ch, &att)
 		if err != nil {
@@ -482,7 +487,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		// Update attestation key fingerprint to compare against the CSR
 		az.Fingerprint = data.Fingerprint
 	default:
-		return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "unexpected attestation object format"))
+		return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "unsupported attestation object format %q", format).WithAdditionalErrorDetail())
 	}
 
 	// Update and store the challenge.
diff --git a/acme/challenge_test.go b/acme/challenge_test.go
index 2fe3653e..0853943e 100644
--- a/acme/challenge_test.go
+++ b/acme/challenge_test.go
@@ -3853,7 +3853,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "unexpected attestation object format")
+							err := NewError(ErrorBadAttestationStatementType, `unsupported attestation object format "bogus-format"`).WithAdditionalErrorDetail()
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)

From a0cdad335dbf84cc38c1df8f19f5462598c9016c Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Mon, 31 Jul 2023 13:22:00 +0200
Subject: [PATCH 08/11] Add test for `WithAdditionalErrorDetail`

---
 acme/errors.go      |  5 +++--
 acme/errors_test.go | 55 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 acme/errors_test.go

diff --git a/acme/errors.go b/acme/errors.go
index 06b45114..bd37e7bd 100644
--- a/acme/errors.go
+++ b/acme/errors.go
@@ -310,8 +310,9 @@ func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error {
 // more information to the ACME client.
 func (e *Error) WithAdditionalErrorDetail() *Error {
 	// prevent internal server errors from disclosing
-	// the internal error to the client at all times.
-	if e == nil || e.Status >= 500 {
+	// the internal error to the client at all times and
+	// prevent nil pointers.
+	if e == nil || e.Status >= 500 || e.Err == nil {
 		return e
 	}
 
diff --git a/acme/errors_test.go b/acme/errors_test.go
new file mode 100644
index 00000000..98040739
--- /dev/null
+++ b/acme/errors_test.go
@@ -0,0 +1,55 @@
+package acme
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func mustJSON(t *testing.T, m map[string]interface{}) string {
+	t.Helper()
+
+	b, err := json.Marshal(m)
+	require.NoError(t, err)
+
+	return string(b)
+}
+
+func TestError_WithAdditionalErrorDetail(t *testing.T) {
+	internalJSON := mustJSON(t, map[string]interface{}{
+		"detail": "The server experienced an internal error",
+		"type":   "urn:ietf:params:acme:error:serverInternal",
+	})
+	malformedErr := NewError(ErrorMalformedType, "malformed error")
+	malformedErr.Err = nil
+	malformedJSON := mustJSON(t, map[string]interface{}{
+		"detail": "The request message was malformed",
+		"type":   "urn:ietf:params:acme:error:malformed",
+	})
+	withDetailJSON := mustJSON(t, map[string]interface{}{
+		"detail": "Attestation statement cannot be verified: invalid property",
+		"type":   "urn:ietf:params:acme:error:badAttestationStatement",
+	})
+	tests := []struct {
+		name string
+		err  *Error
+		want string
+	}{
+		{"internal", NewError(ErrorServerInternalType, "").WithAdditionalErrorDetail(), internalJSON},
+		{"nil err", malformedErr.WithAdditionalErrorDetail(), malformedJSON},
+		{"detailed", NewError(ErrorBadAttestationStatementType, "invalid property").WithAdditionalErrorDetail(), withDetailJSON},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			b, err := json.Marshal(tt.err)
+			require.NoError(t, err)
+
+			// tests if the additional error detail is included in the JSON representation
+			// of the ACME error. This is what is returned to ACME clients and being logged
+			// by the CA.
+			assert.JSONEq(t, tt.want, string(b))
+		})
+	}
+}

From f3c24fe875b20269f95f2a9e28b1fd1bd12ffa8c Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Thu, 3 Aug 2023 14:45:00 +0200
Subject: [PATCH 09/11] Change how multiple identifiers are printed in errors

---
 acme/challenge.go                   | 4 ++--
 acme/challenge_test.go              | 2 +-
 acme/challenge_tpmsimulator_test.go | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index a68b4151..72a6dc46 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -421,7 +421,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 			subproblem := NewSubproblemWithIdentifier(
 				ErrorRejectedIdentifierType,
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
-				"challenge identifier %q doesn't match any of the attested hardware identifiers %s", ch.Value, []string{data.UDID, data.SerialNumber},
+				"challenge identifier %q doesn't match any of the attested hardware identifiers %q", ch.Value, []string{data.UDID, data.SerialNumber},
 			)
 			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
 		}
@@ -479,7 +479,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 			subproblem := NewSubproblemWithIdentifier(
 				ErrorRejectedIdentifierType,
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
-				"challenge identifier %q doesn't match any of the attested hardware identifiers %s", ch.Value, data.PermanentIdentifiers,
+				"challenge identifier %q doesn't match any of the attested hardware identifiers %q", ch.Value, data.PermanentIdentifiers,
 			)
 			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
 		}
diff --git a/acme/challenge_test.go b/acme/challenge_test.go
index 0853943e..aa879726 100644
--- a/acme/challenge_test.go
+++ b/acme/challenge_test.go
@@ -3628,7 +3628,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							subproblem := NewSubproblemWithIdentifier(
 								ErrorRejectedIdentifierType,
 								Identifier{Type: "permanent-identifier", Value: "non-matching-value"},
-								`challenge identifier "non-matching-value" doesn't match any of the attested hardware identifiers [udid serial-number]`,
+								`challenge identifier "non-matching-value" doesn't match any of the attested hardware identifiers ["udid" "serial-number"]`,
 							)
 							err := NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem)
 
diff --git a/acme/challenge_tpmsimulator_test.go b/acme/challenge_tpmsimulator_test.go
index 96381b80..36876638 100644
--- a/acme/challenge_tpmsimulator_test.go
+++ b/acme/challenge_tpmsimulator_test.go
@@ -287,7 +287,7 @@ func Test_deviceAttest01ValidateWithTPMSimulator(t *testing.T) {
 								AddSubproblems(NewSubproblemWithIdentifier(
 									ErrorRejectedIdentifierType,
 									Identifier{Type: "permanent-identifier", Value: "device.id.99999999"},
-									`challenge identifier "device.id.99999999" doesn't match any of the attested hardware identifiers [device.id.12345678]`,
+									`challenge identifier "device.id.99999999" doesn't match any of the attested hardware identifiers ["device.id.12345678"]`,
 								))
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())

From afdd8d3786e8a3aae4ee4b06420d7eb450d6ab3a Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Thu, 3 Aug 2023 14:48:26 +0200
Subject: [PATCH 10/11] Upgrade `golang.org/x/net` to `v0.13.0`

---
 go.mod | 2 +-
 go.sum | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/go.mod b/go.mod
index 4687a1e7..90a147ac 100644
--- a/go.mod
+++ b/go.mod
@@ -35,7 +35,7 @@ require (
 	go.step.sm/linkedca v0.20.0
 	golang.org/x/crypto v0.11.0
 	golang.org/x/exp v0.0.0-20230310171629-522b1b587ee0
-	golang.org/x/net v0.12.0
+	golang.org/x/net v0.13.0
 	google.golang.org/api v0.134.0
 	google.golang.org/grpc v1.57.0
 	google.golang.org/protobuf v1.31.0
diff --git a/go.sum b/go.sum
index 5e891c58..5ac6b640 100644
--- a/go.sum
+++ b/go.sum
@@ -352,6 +352,7 @@ github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zV
 github.com/gogo/protobuf v1.3.0/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o=
 github.com/gogo/protobuf v1.3.1/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o=
 github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
+github.com/golang-jwt/jwt v3.2.1+incompatible/go.mod h1:8pz2t5EyA70fFQQSrl6XZXzqecmYZeUEB8OUGHkxJ+I=
 github.com/golang-jwt/jwt/v4 v4.5.0 h1:7cYmW1XlMY7h7ii7UhUyChSgS5wUJEnm9uZVTGqOWzg=
 github.com/golang-jwt/jwt/v4 v4.5.0/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
@@ -1214,6 +1215,8 @@ golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
 golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
 golang.org/x/net v0.12.0 h1:cfawfvKITfUsFCeJIHJrbSxpeu/E81khclypR0GVT50=
 golang.org/x/net v0.12.0/go.mod h1:zEVYFnQC7m/vmpQFELhcD1EWkZlX69l4oqgmer6hfKA=
+golang.org/x/net v0.13.0 h1:Nvo8UFsZ8X3BhAC9699Z1j7XQ3rsZnUUm7jfBEk1ueY=
+golang.org/x/net v0.13.0/go.mod h1:zEVYFnQC7m/vmpQFELhcD1EWkZlX69l4oqgmer6hfKA=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20181106182150-f42d05182288/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=

From c952e9fc9de1f2d2f172871bb89cf567d733fde8 Mon Sep 17 00:00:00 2001
From: Herman Slatman <hermanslatman@hotmail.com>
Date: Fri, 4 Aug 2023 11:24:22 +0200
Subject: [PATCH 11/11] Use `NewDetailedError` instead

---
 acme/challenge.go                   | 102 ++++++++++++++--------------
 acme/challenge_test.go              |  13 ++--
 acme/challenge_tpmsimulator_test.go |   5 +-
 acme/errors.go                      |  29 ++++----
 acme/errors_test.go                 |   9 ++-
 5 files changed, 80 insertions(+), 78 deletions(-)

diff --git a/acme/challenge.go b/acme/challenge.go
index 72a6dc46..687cc680 100644
--- a/acme/challenge.go
+++ b/acme/challenge.go
@@ -384,7 +384,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 	prov := MustProvisionerFromContext(ctx)
 	if !prov.IsAttestationFormatEnabled(ctx, provisioner.ACMEAttestationFormat(format)) {
 		if format != "apple" && format != "step" && format != "tpm" {
-			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "unsupported attestation object format %q", format).WithAdditionalErrorDetail())
+			return storeError(ctx, db, ch, true, NewDetailedError(ErrorBadAttestationStatementType, "unsupported attestation object format %q", format))
 		}
 
 		return storeError(ctx, db, ch, true,
@@ -409,7 +409,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 		if len(data.Nonce) != 0 {
 			sum := sha256.Sum256([]byte(ch.Token))
 			if subtle.ConstantTimeCompare(data.Nonce, sum[:]) != 1 {
-				return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "challenge token does not match"))
+				return storeError(ctx, db, ch, true, NewDetailedError(ErrorBadAttestationStatementType, "challenge token does not match"))
 			}
 		}
 
@@ -423,7 +423,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
 				"challenge identifier %q doesn't match any of the attested hardware identifiers %q", ch.Value, []string{data.UDID, data.SerialNumber},
 			)
-			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
+			return storeError(ctx, db, ch, true, NewDetailedError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
@@ -451,7 +451,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
 				"challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.SerialNumber,
 			)
-			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
+			return storeError(ctx, db, ch, true, NewDetailedError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
@@ -481,13 +481,13 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
 				Identifier{Type: "permanent-identifier", Value: ch.Value},
 				"challenge identifier %q doesn't match any of the attested hardware identifiers %q", ch.Value, data.PermanentIdentifiers,
 			)
-			return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem))
+			return storeError(ctx, db, ch, true, NewDetailedError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(subproblem))
 		}
 
 		// Update attestation key fingerprint to compare against the CSR
 		az.Fingerprint = data.Fingerprint
 	default:
-		return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "unsupported attestation object format %q", format).WithAdditionalErrorDetail())
+		return storeError(ctx, db, ch, true, NewDetailedError(ErrorBadAttestationStatementType, "unsupported attestation object format %q", format))
 	}
 
 	// Update and store the challenge.
@@ -533,38 +533,38 @@ const (
 func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge, jwk *jose.JSONWebKey, att *attestationObject) (*tpmAttestationData, error) {
 	ver, ok := att.AttStatement["ver"].(string)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "ver not present").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "ver not present")
 	}
 	if ver != "2.0" {
-		return nil, NewError(ErrorBadAttestationStatementType, "version %q is not supported", ver).WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "version %q is not supported", ver)
 	}
 
 	x5c, ok := att.AttStatement["x5c"].([]interface{})
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c not present")
 	}
 	if len(x5c) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is empty").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is empty")
 	}
 
 	akCertBytes, ok := x5c[0].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is malformed")
 	}
 	akCert, err := x509.ParseCertificate(akCertBytes)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is malformed")
 	}
 
 	intermediates := x509.NewCertPool()
 	for _, v := range x5c[1:] {
 		intCertBytes, vok := v.([]byte)
 		if !vok {
-			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
+			return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is malformed")
 		}
 		intCert, err := x509.ParseCertificate(intCertBytes)
 		if err != nil {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
+			return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is malformed")
 		}
 		intermediates.AddCert(intCert)
 	}
@@ -602,19 +602,19 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageAny},
 	})
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is not valid")
 	}
 
 	// validate additional AK certificate requirements
 	if err := validateAKCertificate(akCert); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "AK certificate is not valid").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "AK certificate is not valid")
 	}
 
 	// TODO(hs): implement revocation check; Verify() doesn't perform CRL check nor OCSP lookup.
 
 	sans, err := x509util.ParseSubjectAlternativeNames(akCert)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed parsing AK certificate Subject Alternative Names").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "failed parsing AK certificate Subject Alternative Names")
 	}
 
 	permanentIdentifiers := make([]string, len(sans.PermanentIdentifiers))
@@ -625,37 +625,37 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	// extract and validate pubArea, sig, certInfo and alg properties from the request body
 	pubArea, ok := att.AttStatement["pubArea"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid pubArea in attestation statement").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "invalid pubArea in attestation statement")
 	}
 	if len(pubArea) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "pubArea is empty").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "pubArea is empty")
 	}
 
 	sig, ok := att.AttStatement["sig"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid sig in attestation statement").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "invalid sig in attestation statement")
 	}
 	if len(sig) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "sig is empty").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "sig is empty")
 	}
 
 	certInfo, ok := att.AttStatement["certInfo"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid certInfo in attestation statement").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "invalid certInfo in attestation statement")
 	}
 	if len(certInfo) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "certInfo is empty").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "certInfo is empty")
 	}
 
 	alg, ok := att.AttStatement["alg"].(int64)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid alg in attestation statement").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "invalid alg in attestation statement")
 	}
 
 	// only RS256 and ES256 are allowed
 	coseAlg := coseAlgorithmIdentifier(alg)
 	if coseAlg != coseAlgRS256 && coseAlg != coseAlgES256 {
-		return nil, NewError(ErrorBadAttestationStatementType, "invalid alg %d in attestation statement", alg).WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "invalid alg %d in attestation statement", alg)
 	}
 
 	// set the hash algorithm to use to SHA256
@@ -673,13 +673,13 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 		Hash:   hash,
 	}
 	if err = certificationParameters.Verify(verifyOpts); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "invalid certification parameters").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "invalid certification parameters")
 	}
 
 	// decode the "certInfo" data. This won't fail, as it's also done as part of Verify().
 	tpmCertInfo, err := tpm2.DecodeAttestationData(certInfo)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed decoding attestation data").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "failed decoding attestation data")
 	}
 
 	keyAuth, err := KeyAuthorization(ch.Token, jwk)
@@ -691,18 +691,18 @@ func doTPMAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	// verify the WebAuthn object contains the expect key authorization digest, which is carried
 	// within the encoded `certInfo` property of the attestation statement.
 	if subtle.ConstantTimeCompare(hashedKeyAuth[:], []byte(tpmCertInfo.ExtraData)) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "key authorization invalid").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "key authorization invalid")
 	}
 
 	// decode the (attested) public key and determine its fingerprint.  This won't fail, as it's also done as part of Verify().
 	pub, err := tpm2.DecodePublic(pubArea)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed decoding pubArea").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "failed decoding pubArea")
 	}
 
 	publicKey, err := pub.Key()
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "failed getting public key").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "failed getting public key")
 	}
 
 	data := &tpmAttestationData{
@@ -848,30 +848,30 @@ func doAppleAttestationFormat(_ context.Context, prov Provisioner, _ *Challenge,
 
 	x5c, ok := att.AttStatement["x5c"].([]interface{})
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c not present")
 	}
 	if len(x5c) == 0 {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is empty").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is empty")
 	}
 
 	der, ok := x5c[0].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is malformed")
 	}
 	leaf, err := x509.ParseCertificate(der)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is malformed")
 	}
 
 	intermediates := x509.NewCertPool()
 	for _, v := range x5c[1:] {
 		der, ok = v.([]byte)
 		if !ok {
-			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
+			return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is malformed")
 		}
 		cert, err := x509.ParseCertificate(der)
 		if err != nil {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
+			return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is malformed")
 		}
 		intermediates.AddCert(cert)
 	}
@@ -882,7 +882,7 @@ func doAppleAttestationFormat(_ context.Context, prov Provisioner, _ *Challenge,
 		CurrentTime:   time.Now().Truncate(time.Second),
 		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageAny},
 	}); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is not valid")
 	}
 
 	data := &appleAttestationData{
@@ -954,28 +954,28 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	// Extract x5c and verify certificate
 	x5c, ok := att.AttStatement["x5c"].([]interface{})
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c not present")
 	}
 	if len(x5c) == 0 {
-		return nil, NewError(ErrorRejectedIdentifierType, "x5c is empty").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorRejectedIdentifierType, "x5c is empty")
 	}
 	der, ok := x5c[0].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is malformed")
 	}
 	leaf, err := x509.ParseCertificate(der)
 	if err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is malformed")
 	}
 	intermediates := x509.NewCertPool()
 	for _, v := range x5c[1:] {
 		der, ok = v.([]byte)
 		if !ok {
-			return nil, NewError(ErrorBadAttestationStatementType, "x5c is malformed").WithAdditionalErrorDetail()
+			return nil, NewDetailedError(ErrorBadAttestationStatementType, "x5c is malformed")
 		}
 		cert, err := x509.ParseCertificate(der)
 		if err != nil {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is malformed").WithAdditionalErrorDetail()
+			return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is malformed")
 		}
 		intermediates.AddCert(cert)
 	}
@@ -985,7 +985,7 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 		CurrentTime:   time.Now().Truncate(time.Second),
 		KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageAny},
 	}); err != nil {
-		return nil, WrapError(ErrorBadAttestationStatementType, err, "x5c is not valid").WithAdditionalErrorDetail()
+		return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "x5c is not valid")
 	}
 
 	// Verify proof of possession of private key validating the key
@@ -995,10 +995,10 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	var sig []byte
 	csig, ok := att.AttStatement["sig"].([]byte)
 	if !ok {
-		return nil, NewError(ErrorBadAttestationStatementType, "sig not present").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "sig not present")
 	}
 	if err := cbor.Unmarshal(csig, &sig); err != nil {
-		return nil, NewError(ErrorBadAttestationStatementType, "sig is malformed").WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "sig is malformed")
 	}
 	keyAuth, err := KeyAuthorization(ch.Token, jwk)
 	if err != nil {
@@ -1008,23 +1008,23 @@ func doStepAttestationFormat(_ context.Context, prov Provisioner, ch *Challenge,
 	switch pub := leaf.PublicKey.(type) {
 	case *ecdsa.PublicKey:
 		if pub.Curve != elliptic.P256() {
-			return nil, WrapError(ErrorBadAttestationStatementType, err, "unsupported elliptic curve %s", pub.Curve).WithAdditionalErrorDetail()
+			return nil, WrapDetailedError(ErrorBadAttestationStatementType, err, "unsupported elliptic curve %s", pub.Curve)
 		}
 		sum := sha256.Sum256([]byte(keyAuth))
 		if !ecdsa.VerifyASN1(pub, sum[:], sig) {
-			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature").WithAdditionalErrorDetail()
+			return nil, NewDetailedError(ErrorBadAttestationStatementType, "failed to validate signature")
 		}
 	case *rsa.PublicKey:
 		sum := sha256.Sum256([]byte(keyAuth))
 		if err := rsa.VerifyPKCS1v15(pub, crypto.SHA256, sum[:], sig); err != nil {
-			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature").WithAdditionalErrorDetail()
+			return nil, NewDetailedError(ErrorBadAttestationStatementType, "failed to validate signature")
 		}
 	case ed25519.PublicKey:
 		if !ed25519.Verify(pub, []byte(keyAuth), sig) {
-			return nil, NewError(ErrorBadAttestationStatementType, "failed to validate signature").WithAdditionalErrorDetail()
+			return nil, NewDetailedError(ErrorBadAttestationStatementType, "failed to validate signature")
 		}
 	default:
-		return nil, NewError(ErrorBadAttestationStatementType, "unsupported public key type %T", pub).WithAdditionalErrorDetail()
+		return nil, NewDetailedError(ErrorBadAttestationStatementType, "unsupported public key type %T", pub)
 	}
 
 	// Parse attestation data:
diff --git a/acme/challenge_test.go b/acme/challenge_test.go
index aa879726..f14249d2 100644
--- a/acme/challenge_test.go
+++ b/acme/challenge_test.go
@@ -3532,7 +3532,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
+							err := NewDetailedError(ErrorBadAttestationStatementType, "x5c not present")
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -3579,7 +3579,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "serial-number", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "challenge token does not match")
+							err := NewDetailedError(ErrorBadAttestationStatementType, "challenge token does not match")
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -3630,7 +3630,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 								Identifier{Type: "permanent-identifier", Value: "non-matching-value"},
 								`challenge identifier "non-matching-value" doesn't match any of the attested hardware identifiers ["udid" "serial-number"]`,
 							)
-							err := NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").WithAdditionalErrorDetail().AddSubproblems(subproblem)
+							err := NewDetailedError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(subproblem)
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -3703,7 +3703,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "x5c not present").WithAdditionalErrorDetail()
+							err := NewDetailedError(ErrorBadAttestationStatementType, "x5c not present")
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -3757,8 +3757,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").
-								WithAdditionalErrorDetail().
+							err := NewDetailedError(ErrorBadAttestationStatementType, "permanent identifier does not match").
 								AddSubproblems(NewSubproblemWithIdentifier(
 									ErrorRejectedIdentifierType,
 									Identifier{Type: "permanent-identifier", Value: "12345678"},
@@ -3853,7 +3852,7 @@ func Test_deviceAttest01Validate(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, `unsupported attestation object format "bogus-format"`).WithAdditionalErrorDetail()
+							err := NewDetailedError(ErrorBadAttestationStatementType, `unsupported attestation object format "bogus-format"`)
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
diff --git a/acme/challenge_tpmsimulator_test.go b/acme/challenge_tpmsimulator_test.go
index 36876638..87db8631 100644
--- a/acme/challenge_tpmsimulator_test.go
+++ b/acme/challenge_tpmsimulator_test.go
@@ -237,7 +237,7 @@ func Test_deviceAttest01ValidateWithTPMSimulator(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "device.id.12345678", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, `version "bogus" is not supported`).WithAdditionalErrorDetail()
+							err := NewDetailedError(ErrorBadAttestationStatementType, `version "bogus" is not supported`)
 
 							assert.EqualError(t, updch.Error.Err, err.Err.Error())
 							assert.Equal(t, err.Type, updch.Error.Type)
@@ -282,8 +282,7 @@ func Test_deviceAttest01ValidateWithTPMSimulator(t *testing.T) {
 							assert.Equal(t, ChallengeType("device-attest-01"), updch.Type)
 							assert.Equal(t, "device.id.99999999", updch.Value)
 
-							err := NewError(ErrorBadAttestationStatementType, `permanent identifier does not match`).
-								WithAdditionalErrorDetail().
+							err := NewDetailedError(ErrorBadAttestationStatementType, `permanent identifier does not match`).
 								AddSubproblems(NewSubproblemWithIdentifier(
 									ErrorRejectedIdentifierType,
 									Identifier{Type: "permanent-identifier", Value: "device.id.99999999"},
diff --git a/acme/errors.go b/acme/errors.go
index bd37e7bd..658ec6e0 100644
--- a/acme/errors.go
+++ b/acme/errors.go
@@ -298,20 +298,14 @@ func NewError(pt ProblemType, msg string, args ...interface{}) *Error {
 	return newError(pt, errors.Errorf(msg, args...))
 }
 
-// AddSubproblems adds the Subproblems to Error. It
-// returns the Error, allowing for fluent addition.
-func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error {
-	e.Subproblems = append(e.Subproblems, subproblems...)
-	return e
+// NewDetailedError creates a new Error that includes the error
+// message in the details, providing more information to the
+// ACME client.
+func NewDetailedError(pt ProblemType, msg string, args ...interface{}) *Error {
+	return NewError(pt, msg, args...).withDetail()
 }
 
-// WithAdditionalErrorDetail adds the underlying error
-// to the existing (default) ACME error detail, providing
-// more information to the ACME client.
-func (e *Error) WithAdditionalErrorDetail() *Error {
-	// prevent internal server errors from disclosing
-	// the internal error to the client at all times and
-	// prevent nil pointers.
+func (e *Error) withDetail() *Error {
 	if e == nil || e.Status >= 500 || e.Err == nil {
 		return e
 	}
@@ -320,6 +314,13 @@ func (e *Error) WithAdditionalErrorDetail() *Error {
 	return e
 }
 
+// AddSubproblems adds the Subproblems to Error. It
+// returns the Error, allowing for fluent addition.
+func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error {
+	e.Subproblems = append(e.Subproblems, subproblems...)
+	return e
+}
+
 // NewSubproblem creates a new Subproblem. The msg and args
 // are used to create a new error, which is set as the Detail, allowing
 // for more detailed error messages to be returned to the ACME client.
@@ -383,6 +384,10 @@ func WrapError(typ ProblemType, err error, msg string, args ...interface{}) *Err
 	}
 }
 
+func WrapDetailedError(typ ProblemType, err error, msg string, args ...interface{}) *Error {
+	return WrapError(typ, err, msg, args...).withDetail()
+}
+
 // WrapErrorISE shortcut to wrap an internal server error type.
 func WrapErrorISE(err error, msg string, args ...interface{}) *Error {
 	return WrapError(ErrorServerInternalType, err, msg, args...)
diff --git a/acme/errors_test.go b/acme/errors_test.go
index 98040739..8e586a12 100644
--- a/acme/errors_test.go
+++ b/acme/errors_test.go
@@ -22,8 +22,7 @@ func TestError_WithAdditionalErrorDetail(t *testing.T) {
 		"detail": "The server experienced an internal error",
 		"type":   "urn:ietf:params:acme:error:serverInternal",
 	})
-	malformedErr := NewError(ErrorMalformedType, "malformed error")
-	malformedErr.Err = nil
+	malformedErr := NewError(ErrorMalformedType, "malformed error") // will result in Err == nil behavior
 	malformedJSON := mustJSON(t, map[string]interface{}{
 		"detail": "The request message was malformed",
 		"type":   "urn:ietf:params:acme:error:malformed",
@@ -37,9 +36,9 @@ func TestError_WithAdditionalErrorDetail(t *testing.T) {
 		err  *Error
 		want string
 	}{
-		{"internal", NewError(ErrorServerInternalType, "").WithAdditionalErrorDetail(), internalJSON},
-		{"nil err", malformedErr.WithAdditionalErrorDetail(), malformedJSON},
-		{"detailed", NewError(ErrorBadAttestationStatementType, "invalid property").WithAdditionalErrorDetail(), withDetailJSON},
+		{"internal", NewDetailedError(ErrorServerInternalType, ""), internalJSON},
+		{"nil err", malformedErr, malformedJSON},
+		{"detailed", NewDetailedError(ErrorBadAttestationStatementType, "invalid property"), withDetailJSON},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {