From b4c3aca7a131db2082b235db14d77f4afb2a593d Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Wed, 29 Mar 2023 17:08:31 -0400 Subject: [PATCH] Merge ACME package back into the PKI package (#19826) * Squash pki/acme package down to pki folder Without refactoring most of PKI to export the storage layer, which we were initially hesitant about, it would be nearly impossible to have the ACME layer handle its own storage while being in the acme/ subpackage under the pki package. Thus, merge the two packages together again. Signed-off-by: Alexander Scheel * Properly format errors for missing parameters When missing required ACME request parameters, don't return Vault-level errors, but drop into the PKI package to return properly-formatted ACME error messages. Signed-off-by: Alexander Scheel * Error type clarifications Signed-off-by: Alexander Scheel * Fix GetOk with type conversion calls Signed-off-by: Alexander Scheel --------- Signed-off-by: Alexander Scheel --- .../pki/{acme/errors.go => acme_errors.go} | 2 +- .../logical/pki/{acme/jws.go => acme_jws.go} | 24 ++++----- .../pki/{acme/state.go => acme_state.go} | 52 ++++++++++++------- .../state_test.go => acme_state_test.go} | 2 +- builtin/logical/pki/backend.go | 6 +-- builtin/logical/pki/path_acme_directory.go | 9 ++-- builtin/logical/pki/path_acme_new_account.go | 27 +++++----- 7 files changed, 66 insertions(+), 56 deletions(-) rename builtin/logical/pki/{acme/errors.go => acme_errors.go} (99%) rename builtin/logical/pki/{acme/jws.go => acme_jws.go} (82%) rename builtin/logical/pki/{acme/state.go => acme_state.go} (68%) rename builtin/logical/pki/{acme/state_test.go => acme_state_test.go} (98%) diff --git a/builtin/logical/pki/acme/errors.go b/builtin/logical/pki/acme_errors.go similarity index 99% rename from builtin/logical/pki/acme/errors.go rename to builtin/logical/pki/acme_errors.go index 69683837e..f75ee88b5 100644 --- a/builtin/logical/pki/acme/errors.go +++ b/builtin/logical/pki/acme_errors.go @@ -1,4 +1,4 @@ -package acme +package pki import ( "encoding/json" diff --git a/builtin/logical/pki/acme/jws.go b/builtin/logical/pki/acme_jws.go similarity index 82% rename from builtin/logical/pki/acme/jws.go rename to builtin/logical/pki/acme_jws.go index 0807b7b5d..034c379c8 100644 --- a/builtin/logical/pki/acme/jws.go +++ b/builtin/logical/pki/acme_jws.go @@ -1,4 +1,4 @@ -package acme +package pki import ( "crypto" @@ -23,7 +23,7 @@ var AllowedOuterJWSTypes = map[string]interface{}{ } // This wraps a JWS message structure. -type JWSCtx struct { +type jwsCtx struct { Algo string `json:"alg"` Kid string `json:"kid"` jwk json.RawMessage `json:"jwk"` @@ -33,7 +33,7 @@ type JWSCtx struct { Existing bool `json:"-"` } -func (c *JWSCtx) UnmarshalJSON(a *ACMEState, jws []byte) error { +func (c *jwsCtx) UnmarshalJSON(a *acmeState, jws []byte) error { var err error if err = json.Unmarshal(jws, c); err != nil { return err @@ -44,7 +44,7 @@ func (c *JWSCtx) UnmarshalJSON(a *ACMEState, jws []byte) error { // // > The "jwk" and "kid" fields are mutually exclusive. Servers MUST // > reject requests that contain both. - return fmt.Errorf("invalid header: got both account 'kid' and 'jwk' in the same message; expected only one") + return fmt.Errorf("invalid header: got both account 'kid' and 'jwk' in the same message; expected only one: %w", ErrMalformed) } if c.Kid == "" && len(c.jwk) == 0 { @@ -52,7 +52,7 @@ func (c *JWSCtx) UnmarshalJSON(a *ACMEState, jws []byte) error { // // > Either "jwk" (JSON Web Key) or "kid" (Key ID) as specified // > below - return fmt.Errorf("invalid header: got neither required fields of 'kid' nor 'jwk'") + return fmt.Errorf("invalid header: got neither required fields of 'kid' nor 'jwk': %w", ErrMalformed) } if _, present := AllowedOuterJWSTypes[c.Algo]; !present { @@ -65,7 +65,7 @@ func (c *JWSCtx) UnmarshalJSON(a *ACMEState, jws []byte) error { // > * This field MUST NOT contain "none" or a Message // > Authentication Code (MAC) algorithm (e.g. one in which the // > algorithm registry description mentions MAC/HMAC). - return fmt.Errorf("invalid header: unexpected value for 'algo'") + return fmt.Errorf("invalid header: unexpected value for 'algo': %w", ErrMalformed) } if c.Kid != "" { @@ -82,7 +82,7 @@ func (c *JWSCtx) UnmarshalJSON(a *ACMEState, jws []byte) error { } if !c.key.Valid() { - return fmt.Errorf("received invalid jwk") + return fmt.Errorf("received invalid jwk: %w", ErrMalformed) } if c.Kid != "" { @@ -103,7 +103,7 @@ func hasValues(h jose.Header) bool { return h.KeyID != "" || h.JSONWebKey != nil || h.Algorithm != "" || h.Nonce != "" || len(h.ExtraHeaders) > 0 } -func (c *JWSCtx) VerifyJWS(signature string) (map[string]interface{}, error) { +func (c *jwsCtx) VerifyJWS(signature string) (map[string]interface{}, error) { // See RFC 8555 Section 6.2. Request Authentication: // // > The JWS Unencoded Payload Option [RFC7797] MUST NOT be used @@ -111,21 +111,21 @@ func (c *JWSCtx) VerifyJWS(signature string) (map[string]interface{}, error) { // This is validated by go-jose. sig, err := jose.ParseSigned(signature) if err != nil { - return nil, fmt.Errorf("error parsing signature: %w", err) + return nil, fmt.Errorf("error parsing signature: %s: %w", err, ErrMalformed) } if len(sig.Signatures) > 1 { // See RFC 8555 Section 6.2. Request Authentication: // // > The JWS MUST NOT have multiple signatures - return nil, fmt.Errorf("request had multiple signatures") + return nil, fmt.Errorf("request had multiple signatures: %w", ErrMalformed) } if hasValues(sig.Signatures[0].Unprotected) { // See RFC 8555 Section 6.2. Request Authentication: // // > The JWS Unprotected Header [RFC7515] MUST NOT be used - return nil, fmt.Errorf("request had unprotected headers") + return nil, fmt.Errorf("request had unprotected headers: %w", ErrMalformed) } payload, err := sig.Verify(c.key) @@ -135,7 +135,7 @@ func (c *JWSCtx) VerifyJWS(signature string) (map[string]interface{}, error) { var m map[string]interface{} if err := json.Unmarshal(payload, &m); err != nil { - return nil, fmt.Errorf("failed to json unmarshal 'payload': %w", err) + return nil, fmt.Errorf("failed to json unmarshal 'payload': %s: %w", err, ErrMalformed) } return m, nil diff --git a/builtin/logical/pki/acme/state.go b/builtin/logical/pki/acme_state.go similarity index 68% rename from builtin/logical/pki/acme/state.go rename to builtin/logical/pki/acme_state.go index 2e341f1e8..36715e168 100644 --- a/builtin/logical/pki/acme/state.go +++ b/builtin/logical/pki/acme_state.go @@ -1,4 +1,4 @@ -package acme +package pki import ( "crypto/rand" @@ -15,13 +15,13 @@ import ( // How long nonces are considered valid. const nonceExpiry = 15 * time.Minute -type ACMEState struct { +type acmeState struct { nextExpiry *atomic.Int64 nonces *sync.Map // map[string]time.Time } -func NewACMEState() *ACMEState { - return &ACMEState{ +func NewACMEState() *acmeState { + return &acmeState{ nextExpiry: new(atomic.Int64), nonces: new(sync.Map), } @@ -36,7 +36,7 @@ func generateNonce() (string, error) { return base64.RawURLEncoding.EncodeToString(data), nil } -func (a *ACMEState) GetNonce() (string, time.Time, error) { +func (a *acmeState) GetNonce() (string, time.Time, error) { now := time.Now() nonce, err := generateNonce() if err != nil { @@ -55,7 +55,7 @@ func (a *ACMEState) GetNonce() (string, time.Time, error) { return nonce, then, nil } -func (a *ACMEState) RedeemNonce(nonce string) bool { +func (a *acmeState) RedeemNonce(nonce string) bool { rawTimeout, present := a.nonces.LoadAndDelete(nonce) if !present { return false @@ -69,7 +69,7 @@ func (a *ACMEState) RedeemNonce(nonce string) bool { return true } -func (a *ACMEState) DoTidyNonces() { +func (a *acmeState) DoTidyNonces() { now := time.Now() expiry := a.nextExpiry.Load() then := time.Unix(expiry, 0) @@ -79,7 +79,7 @@ func (a *ACMEState) DoTidyNonces() { } } -func (a *ACMEState) TidyNonces() { +func (a *acmeState) TidyNonces() { now := time.Now() nextRun := now.Add(nonceExpiry) @@ -99,22 +99,22 @@ func (a *ACMEState) TidyNonces() { a.nextExpiry.Store(nextRun.Unix()) } -func (a *ACMEState) CreateAccount(c *JWSCtx, contact []string, termsOfServiceAgreed bool) (map[string]interface{}, error) { +func (a *acmeState) CreateAccount(c *jwsCtx, contact []string, termsOfServiceAgreed bool) (map[string]interface{}, error) { // TODO return nil, nil } -func (a *ACMEState) LoadAccount(keyID string) (map[string]interface{}, error) { +func (a *acmeState) LoadAccount(keyID string) (map[string]interface{}, error) { // TODO return nil, nil } -func (a *ACMEState) DoesAccountExist(keyId string) bool { +func (a *acmeState) DoesAccountExist(keyId string) bool { account, err := a.LoadAccount(keyId) return err == nil && len(account) > 0 } -func (a *ACMEState) LoadJWK(keyID string) ([]byte, error) { +func (a *acmeState) LoadJWK(keyID string) ([]byte, error) { key, err := a.LoadAccount(keyID) if err != nil { return nil, err @@ -128,15 +128,20 @@ func (a *ACMEState) LoadJWK(keyID string) ([]byte, error) { return jwk.([]byte), nil } -func (a *ACMEState) ParseRequestParams(data *framework.FieldData) (*JWSCtx, map[string]interface{}, error) { - var c JWSCtx +func (a *acmeState) ParseRequestParams(data *framework.FieldData) (*jwsCtx, map[string]interface{}, error) { + var c jwsCtx var m map[string]interface{} // Parse the key out. - jwkBase64 := data.Get("protected").(string) + rawJWKBase64, ok := data.GetOk("protected") + if !ok { + return nil, nil, fmt.Errorf("missing required field 'protected': %w", ErrMalformed) + } + jwkBase64 := rawJWKBase64.(string) + jwkBytes, err := base64.RawURLEncoding.DecodeString(jwkBase64) if err != nil { - return nil, nil, fmt.Errorf("failed to base64 parse 'protected': %w", err) + return nil, nil, fmt.Errorf("failed to base64 parse 'protected': %s: %w", err, ErrMalformed) } if err = c.UnmarshalJSON(a, jwkBytes); err != nil { return nil, nil, fmt.Errorf("failed to json unmarshal 'protected': %w", err) @@ -146,11 +151,20 @@ func (a *ACMEState) ParseRequestParams(data *framework.FieldData) (*JWSCtx, map[ // should read and redeem the nonce here too, to avoid doing any extra // work if it is invalid. if !a.RedeemNonce(c.Nonce) { - return nil, nil, fmt.Errorf("invalid or reused nonce") + return nil, nil, fmt.Errorf("invalid or reused nonce: %w", ErrBadNonce) } - payloadBase64 := data.Get("payload").(string) - signatureBase64 := data.Get("signature").(string) + rawPayloadBase64, ok := data.GetOk("payload") + if !ok { + return nil, nil, fmt.Errorf("missing required field 'payload': %w", ErrMalformed) + } + payloadBase64 := rawPayloadBase64.(string) + + rawSignatureBase64, ok := data.GetOk("signature") + if !ok { + return nil, nil, fmt.Errorf("missing required field 'signature': %w", ErrMalformed) + } + signatureBase64 := rawSignatureBase64.(string) // go-jose only seems to support compact signature encodings. compactSig := fmt.Sprintf("%v.%v.%v", jwkBase64, payloadBase64, signatureBase64) diff --git a/builtin/logical/pki/acme/state_test.go b/builtin/logical/pki/acme_state_test.go similarity index 98% rename from builtin/logical/pki/acme/state_test.go rename to builtin/logical/pki/acme_state_test.go index 55bde1fd7..edce657f5 100644 --- a/builtin/logical/pki/acme/state_test.go +++ b/builtin/logical/pki/acme_state_test.go @@ -1,4 +1,4 @@ -package acme +package pki import ( "testing" diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index de937ba73..7d2a8ea1e 100644 --- a/builtin/logical/pki/backend.go +++ b/builtin/logical/pki/backend.go @@ -14,8 +14,6 @@ import ( atomic2 "go.uber.org/atomic" - "github.com/hashicorp/vault/builtin/logical/pki/acme" - "github.com/armon/go-metrics" "github.com/hashicorp/go-multierror" "github.com/hashicorp/vault/helper/constants" @@ -290,7 +288,7 @@ func Backend(conf *logical.BackendConfig) *backend { b.unifiedTransferStatus = newUnifiedTransferStatus() - b.acmeState = acme.NewACMEState() + b.acmeState = NewACMEState() return &b } @@ -325,7 +323,7 @@ type backend struct { issuersLock sync.RWMutex // Context around ACME operations - acmeState *acme.ACMEState + acmeState *acmeState } type roleOperation func(ctx context.Context, req *logical.Request, data *framework.FieldData, role *roleEntry) (*logical.Response, error) diff --git a/builtin/logical/pki/path_acme_directory.go b/builtin/logical/pki/path_acme_directory.go index 4770ff7ab..bd9f2e210 100644 --- a/builtin/logical/pki/path_acme_directory.go +++ b/builtin/logical/pki/path_acme_directory.go @@ -8,7 +8,6 @@ import ( "net/url" "strings" - "github.com/hashicorp/vault/builtin/logical/pki/acme" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" ) @@ -69,7 +68,7 @@ func (b *backend) acmeWrapper(op acmeOperation) framework.OperationFunc { if false { // TODO sclark: Check if ACME is enable here - return nil, fmt.Errorf("ACME is disabled in configuration: %w", acme.ErrServerInternal) + return nil, fmt.Errorf("ACME is disabled in configuration: %w", ErrServerInternal) } baseUrl, err := getAcmeBaseUrl(sc, r.Path) @@ -93,12 +92,12 @@ func getAcmeBaseUrl(sc *storageContext, path string) (*url.URL, error) { } if cfg.Path == "" { - return nil, fmt.Errorf("ACME feature requires local cluster path configuration to be set: %w", acme.ErrServerInternal) + return nil, fmt.Errorf("ACME feature requires local cluster path configuration to be set: %w", ErrServerInternal) } baseUrl, err := url.Parse(cfg.Path) if err != nil { - return nil, fmt.Errorf("ACME feature a proper URL configured in local cluster path: %w", acme.ErrServerInternal) + return nil, fmt.Errorf("ACME feature a proper URL configured in local cluster path: %w", ErrServerInternal) } directoryPrefix := "" @@ -114,7 +113,7 @@ func acmeErrorWrapper(op framework.OperationFunc) framework.OperationFunc { return func(ctx context.Context, r *logical.Request, data *framework.FieldData) (*logical.Response, error) { resp, err := op(ctx, r, data) if err != nil { - return acme.TranslateError(err) + return TranslateError(err) } return resp, nil diff --git a/builtin/logical/pki/path_acme_new_account.go b/builtin/logical/pki/path_acme_new_account.go index 7095e67a7..46b49ab39 100644 --- a/builtin/logical/pki/path_acme_new_account.go +++ b/builtin/logical/pki/path_acme_new_account.go @@ -4,7 +4,6 @@ import ( "fmt" "strings" - "github.com/hashicorp/vault/builtin/logical/pki/acme" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" ) @@ -50,19 +49,19 @@ func addFieldsForACMERequest(fields map[string]*framework.FieldSchema) map[strin fields["protected"] = &framework.FieldSchema{ Type: framework.TypeString, Description: "ACME request 'protected' value", - Required: true, + Required: false, } fields["payload"] = &framework.FieldSchema{ Type: framework.TypeString, Description: "ACME request 'payload' value", - Required: true, + Required: false, } fields["signature"] = &framework.FieldSchema{ Type: framework.TypeString, Description: "ACME request 'signature' value", - Required: true, + Required: false, } return fields @@ -89,7 +88,7 @@ func patternAcmeNewAccount(b *backend, pattern string) *framework.Path { } } -type acmeParsedOperation func(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *acme.JWSCtx, data map[string]interface{}) (*logical.Response, error) +type acmeParsedOperation func(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}) (*logical.Response, error) func (b *backend) acmeParsedWrapper(op acmeParsedOperation) framework.OperationFunc { return b.acmeWrapper(func(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData) (*logical.Response, error) { @@ -102,7 +101,7 @@ func (b *backend) acmeParsedWrapper(op acmeParsedOperation) framework.OperationF }) } -func (b *backend) acmeNewAccountHandler(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *acme.JWSCtx, data map[string]interface{}) (*logical.Response, error) { +func (b *backend) acmeNewAccountHandler(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}) (*logical.Response, error) { // Parameters var ok bool var onlyReturnExisting bool @@ -113,7 +112,7 @@ func (b *backend) acmeNewAccountHandler(acmeCtx acmeContext, r *logical.Request, if present { contact, ok = rawContact.([]string) if !ok { - return nil, fmt.Errorf("invalid type for field 'contact': %w", acme.ErrMalformed) + return nil, fmt.Errorf("invalid type for field 'contact': %w", ErrMalformed) } } @@ -121,7 +120,7 @@ func (b *backend) acmeNewAccountHandler(acmeCtx acmeContext, r *logical.Request, if present { termsOfServiceAgreed, ok = rawTermsOfServiceAgreed.(bool) if !ok { - return nil, fmt.Errorf("invalid type for field 'termsOfServiceAgreed': %w", acme.ErrMalformed) + return nil, fmt.Errorf("invalid type for field 'termsOfServiceAgreed': %w", ErrMalformed) } } @@ -129,7 +128,7 @@ func (b *backend) acmeNewAccountHandler(acmeCtx acmeContext, r *logical.Request, if present { onlyReturnExisting, ok = rawOnlyReturnExisting.(bool) if !ok { - return nil, fmt.Errorf("invalid type for field 'onlyReturnExisting': %w", acme.ErrMalformed) + return nil, fmt.Errorf("invalid type for field 'onlyReturnExisting': %w", ErrMalformed) } } @@ -160,7 +159,7 @@ func formatAccountResponse(location string, status string, contact []string) *lo return resp } -func (b *backend) acmeNewAccountSearchHandler(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *acme.JWSCtx, data map[string]interface{}) (*logical.Response, error) { +func (b *backend) acmeNewAccountSearchHandler(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}) (*logical.Response, error) { if userCtx.Existing || b.acmeState.DoesAccountExist(userCtx.Kid) { // This account exists; return its details. It would be slightly // weird to specify a kid in the request (and not use an explicit @@ -179,12 +178,12 @@ func (b *backend) acmeNewAccountSearchHandler(acmeCtx acmeContext, r *logical.Re // > If a client sends such a request and an account does not exist, // > then the server MUST return an error response with status code // > 400 (Bad Request) and type "urn:ietf:params:acme:error:accountDoesNotExist". - return nil, fmt.Errorf("An account with this key does not exist: %w", acme.ErrAccountDoesNotExist) + return nil, fmt.Errorf("An account with this key does not exist: %w", ErrAccountDoesNotExist) } -func (b *backend) acmeNewAccountCreateHandler(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *acme.JWSCtx, data map[string]interface{}, contact []string, termsOfServiceAgreed bool) (*logical.Response, error) { +func (b *backend) acmeNewAccountCreateHandler(acmeCtx acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}, contact []string, termsOfServiceAgreed bool) (*logical.Response, error) { if userCtx.Existing { - return nil, fmt.Errorf("cannot submit to newAccount with 'kid': %w", acme.ErrMalformed) + return nil, fmt.Errorf("cannot submit to newAccount with 'kid': %w", ErrMalformed) } // If the account already exists, return the existing one. @@ -194,7 +193,7 @@ func (b *backend) acmeNewAccountCreateHandler(acmeCtx acmeContext, r *logical.Re // TODO: Limit this only when ToS are required by the operator. if !termsOfServiceAgreed { - return nil, fmt.Errorf("terms of service not agreed to: %w", acme.ErrUserActionRequired) + return nil, fmt.Errorf("terms of service not agreed to: %w", ErrUserActionRequired) } account, err := b.acmeState.CreateAccount(userCtx, contact, termsOfServiceAgreed)