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] 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