Address review remarks

acme/api/order.go

@@ -10,18 +10,16 @@ import (
 	"strings"
 	"time"
 
-	"go.step.sm/crypto/kms/uri"
-
 	"github.com/go-chi/chi/v5"
 
 	"go.step.sm/crypto/randutil"
 	"go.step.sm/crypto/x509util"
 
 	"github.com/smallstep/certificates/acme"
+	"github.com/smallstep/certificates/acme/wire"
 	"github.com/smallstep/certificates/api/render"
 	"github.com/smallstep/certificates/authority/policy"
 	"github.com/smallstep/certificates/authority/provisioner"
-	"github.com/smallstep/certificates/wire"
 )
 
 // NewOrderRequest represents the body for a NewOrder request.
@@ -52,16 +50,12 @@ func (n *NewOrderRequest) Validate() error {
 				return acme.NewError(acme.ErrorMalformedType, "permanent identifier cannot be empty")
 			}
 		case acme.WireID:
-			orderValue, err := wire.ParseID([]byte(id.Value))
+			wireID, err := wire.ParseID([]byte(id.Value))
-			if err != nil {
-				return acme.NewError(acme.ErrorMalformedType, "ID cannot be parsed")
-			}
-			clientIDURI, err := uri.Parse(orderValue.ClientID)
 			if err != nil {
-				return acme.NewError(acme.ErrorMalformedType, "invalid client ID, it's supposed to be a valid URI")
+				return acme.WrapError(acme.ErrorMalformedType, err, "failed parsing Wire ID")
 			}
-			if clientIDURI.Scheme != "wireapp" {
+			if _, err = wire.ParseClientID(wireID.ClientID); err != nil {
-				return acme.NewError(acme.ErrorMalformedType, "invalid client ID scheme")
+				return acme.WrapError(acme.ErrorMalformedType, err, "invalid Wire client ID %q", wireID.ClientID)
 			}
 		default:
 			return acme.NewError(acme.ErrorMalformedType, "identifier type unsupported: %s", id.Type)

acme/challenge.go

@@ -36,10 +36,9 @@ import (
 	"go.step.sm/crypto/pemutil"
 	"go.step.sm/crypto/x509util"
 	"golang.org/x/exp/slices"
-	"gopkg.in/square/go-jose.v2/jwt"
 
+	"github.com/smallstep/certificates/acme/wire"
 	"github.com/smallstep/certificates/authority/provisioner"
-	"github.com/smallstep/certificates/wire"
 )
 
 type ChallengeType string
@@ -390,8 +389,7 @@ func wireOIDC01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO
 		GivenName string `json:"given_name,omitempty"`
 		KeyAuth   string `json:"keyauth"` // TODO(hs): use this property instead of the one in the payload after https://github.com/wireapp/rusty-jwt-tools/tree/fix/keyauth is done
 	}
-	err = idToken.Claims(&claims)
+	if err = idToken.Claims(&claims); err != nil {
-	if err != nil {
 		return storeError(ctx, db, ch, false, WrapError(ErrorRejectedIdentifierType, err,
 			"error retrieving claims from ID token"))
 	}
@@ -423,28 +421,26 @@ func wireOIDC01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO
 		return WrapErrorISE(err, "error updating challenge")
 	}
 
-	parsedIDToken, err := jwt.ParseSigned(wireChallengePayload.IDToken)
+	parsedIDToken, err := jose.ParseSigned(wireChallengePayload.IDToken)
 	if err != nil {
-		return WrapErrorISE(err, "Invalid OIDC id token")
+		return WrapErrorISE(err, "invalid OIDC ID token")
 	}
 	oidcToken := make(map[string]interface{})
 	if err := parsedIDToken.UnsafeClaimsWithoutVerification(&oidcToken); err != nil {
-		return WrapErrorISE(err, "Failed parsing OIDC id token")
+		return WrapErrorISE(err, "failed parsing OIDC id token")
 	}
 
 	orders, err := db.GetAllOrdersByAccountID(ctx, ch.AccountID)
 	if err != nil {
-		return WrapErrorISE(err, "Could not find current order by account id")
+		return WrapErrorISE(err, "could not find current order by account id")
 	}
-
 	if len(orders) == 0 {
-		return WrapErrorISE(err, "There are not enough orders for this account for this custom OIDC challenge")
+		return NewErrorISE("there are not enough orders for this account for this custom OIDC challenge")
 	}
 
 	order := orders[len(orders)-1]
-
 	if err := db.CreateOidcToken(ctx, order, oidcToken); err != nil {
-		return WrapErrorISE(err, "Failed storing OIDC id token")
+		return WrapErrorISE(err, "failed storing OIDC id token")
 	}
 
 	return nil
@@ -456,19 +452,17 @@ func wireDPOP01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO
 		return NewErrorISE("missing provisioner")
 	}
 
-	rawKid, thumbprintErr := jwk.Thumbprint(crypto.SHA256)
+	rawKid, err := jwk.Thumbprint(crypto.SHA256)
-	if thumbprintErr != nil {
+	if err != nil {
-		return storeError(ctx, db, ch, false, WrapError(ErrorServerInternalType, thumbprintErr, "failed to compute JWK thumbprint"))
+		return storeError(ctx, db, ch, false, WrapError(ErrorServerInternalType, err, "failed to compute JWK thumbprint"))
 	}
-
 	kid := base64.RawURLEncoding.EncodeToString(rawKid)
 
 	dpopOptions := prov.GetOptions().GetDPOPOptions()
 	key := dpopOptions.GetSigningKey()
 
 	var wireChallengePayload WireChallengePayload
-	err := json.Unmarshal(payload, &wireChallengePayload)
+	if err := json.Unmarshal(payload, &wireChallengePayload); err != nil {
-	if err != nil {
 		return storeError(ctx, db, ch, false, WrapError(ErrorRejectedIdentifierType, err,
 			"error unmarshalling Wire challenge payload"))
 	}
@@ -485,7 +479,7 @@ func wireDPOP01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO
 
 	n, err := file.Write(buf.Bytes())
 	if err != nil {
-		return WrapErrorISE(err, "Failed writing signature key to temp file")
+		return WrapErrorISE(err, "failed writing signature key to temp file")
 	}
 	if n != buf.Len() {
 		return WrapErrorISE(err, "expected to write %d characters to the key file, got %d", buf.Len(), n)
@@ -503,7 +497,7 @@ func wireDPOP01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO
 
 	issuer, err := dpopOptions.GetTarget(clientID.DeviceID)
 	if err != nil {
-		return WrapErrorISE(err, "Invalid Go template registered for 'target'")
+		return WrapErrorISE(err, "invalid Go template registered for 'target'")
 	}
 
 	expiry := strconv.FormatInt(time.Now().Add(time.Hour*24*365).Unix(), 10)
@@ -565,42 +559,42 @@ func wireDPOP01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO
 		return WrapErrorISE(err, "error updating challenge")
 	}
 
-	parsedAccessToken, err := jwt.ParseSigned(wireChallengePayload.AccessToken)
+	parsedAccessToken, err := jose.ParseSigned(wireChallengePayload.AccessToken)
 	if err != nil {
-		return WrapErrorISE(err, "Invalid access token")
+		return WrapErrorISE(err, "invalid access token")
 	}
 	access := make(map[string]interface{})
 	if err := parsedAccessToken.UnsafeClaimsWithoutVerification(&access); err != nil {
-		return WrapErrorISE(err, "Failed parsing access token")
+		return WrapErrorISE(err, "failed parsing access token")
 	}
 
 	rawDpop, ok := access["proof"].(string)
 	if !ok {
-		return WrapErrorISE(err, "Invalid dpop proof format in access token")
+		return WrapErrorISE(err, "invalid dpop proof format in access token")
 	}
 
-	parsedDpopToken, err := jwt.ParseSigned(rawDpop)
+	parsedDpopToken, err := jose.ParseSigned(rawDpop)
 	if err != nil {
-		return WrapErrorISE(err, "Invalid DPoP token")
+		return WrapErrorISE(err, "invalid DPoP token")
 	}
 	dpop := make(map[string]interface{})
 	if err := parsedDpopToken.UnsafeClaimsWithoutVerification(&dpop); err != nil {
-		return WrapErrorISE(err, "Failed parsing dpop token")
+		return WrapErrorISE(err, "failed parsing dpop token")
 	}
 
 	orders, err := db.GetAllOrdersByAccountID(ctx, ch.AccountID)
 	if err != nil {
-		return WrapErrorISE(err, "Could not find current order by account id")
+		return WrapErrorISE(err, "could not find current order by account id")
 	}
 
 	if len(orders) == 0 {
-		return WrapErrorISE(err, "There are not enough orders for this account for this custom OIDC challenge")
+		return WrapErrorISE(err, "there are not enough orders for this account for this custom OIDC challenge")
 	}
 
 	order := orders[len(orders)-1]
 
 	if err := db.CreateDpopToken(ctx, order, dpop); err != nil {
-		return WrapErrorISE(err, "Failed storing DPoP token")
+		return WrapErrorISE(err, "failed storing DPoP token")
 	}
 
 	return nil

acme/db.go

@@ -52,9 +52,10 @@ type DB interface {
 	CreateOrder(ctx context.Context, o *Order) error
 	GetOrder(ctx context.Context, id string) (*Order, error)
 	GetOrdersByAccountID(ctx context.Context, accountID string) ([]string, error)
-	GetAllOrdersByAccountID(ctx context.Context, accountID string) ([]string, error)
 	UpdateOrder(ctx context.Context, o *Order) error
 
+	// TODO(hs): put in a different interface
+	GetAllOrdersByAccountID(ctx context.Context, accountID string) ([]string, error)
 	CreateDpopToken(ctx context.Context, orderID string, dpop map[string]interface{}) error
 	GetDpopToken(ctx context.Context, orderID string) (map[string]interface{}, error)
 	CreateOidcToken(ctx context.Context, orderID string, idToken map[string]interface{}) error

acme/order.go

@@ -17,8 +17,8 @@ import (
 	"go.step.sm/crypto/keyutil"
 	"go.step.sm/crypto/x509util"
 
+	"github.com/smallstep/certificates/acme/wire"
 	"github.com/smallstep/certificates/authority/provisioner"
-	"github.com/smallstep/certificates/wire"
 )
 
 type IdentifierType string

acme/wire/id.go

@@ -21,6 +21,7 @@ func ParseID(data []byte) (wireID ID, err error) {
 }
 
 type ClientID struct {
+	Scheme   string
 	Username string
 	DeviceID string
 	Domain   string
@@ -34,14 +35,18 @@ type ClientID struct {
 func ParseClientID(clientID string) (ClientID, error) {
 	clientIDURI, err := uri.Parse(clientID)
 	if err != nil {
-		return ClientID{}, fmt.Errorf("invalid clientID URI %q: %w", clientID, err)
+		return ClientID{}, fmt.Errorf("invalid Wire client ID URI %q: %w", clientID, err)
+	}
+	if clientIDURI.Scheme != "wireapp" {
+		return ClientID{}, fmt.Errorf("invalid Wire client ID scheme %q; expected \"wireapp\"", clientIDURI.Scheme)
 	}
 	fullUsername := clientIDURI.User.Username()
 	parts := strings.SplitN(fullUsername, "!", 2)
 	if len(parts) != 2 {
-		return ClientID{}, fmt.Errorf("invalid clientID %q", fullUsername)
+		return ClientID{}, fmt.Errorf("invalid Wire client ID username %q", fullUsername)
 	}
 	return ClientID{
+		Scheme:   clientIDURI.Scheme,
 		Username: parts[0],
 		DeviceID: parts[1],
 		Domain:   clientIDURI.Host,

authority/provisioner/acme.go

@@ -10,8 +10,9 @@ import (
 	"time"
 
 	"github.com/pkg/errors"
-	"github.com/smallstep/certificates/wire"
 	"go.step.sm/linkedca"
+
+	"github.com/smallstep/certificates/acme/wire"
 )
 
 // ACMEChallenge represents the supported acme challenges.

go.mod

@@ -148,6 +148,6 @@ require (
 	google.golang.org/genproto/googleapis/api v0.0.0-20231211222908-989df2bf70f3 // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20231212172506-995d672761c0 // indirect
 	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
-	gopkg.in/square/go-jose.v2 v2.6.0
+	gopkg.in/square/go-jose.v2 v2.6.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect; indirects
 )