diff --git a/builtin/logical/transit/backend.go b/builtin/logical/transit/backend.go index 62703e6af0af..e30d7660496b 100644 --- a/builtin/logical/transit/backend.go +++ b/builtin/logical/transit/backend.go @@ -248,6 +248,7 @@ func (b *backend) autoRotateKeys(ctx context.Context, req *logical.Request) erro continue } + // rotateIfRequired properly acquires/releases the lock on p err = b.rotateIfRequired(ctx, req, key, p) if err != nil { errs = multierror.Append(errs, err) diff --git a/builtin/logical/transit/path_decrypt.go b/builtin/logical/transit/path_decrypt.go index 72c055bb67f6..1daf74daf5d1 100644 --- a/builtin/logical/transit/path_decrypt.go +++ b/builtin/logical/transit/path_decrypt.go @@ -184,6 +184,7 @@ func (b *backend) pathDecryptWrite(ctx context.Context, req *logical.Request, d if !b.System().CachingDisabled() { p.Lock(false) } + defer p.Unlock() successesInBatch := false for i, item := range batchInputItems { @@ -243,8 +244,6 @@ func (b *backend) pathDecryptWrite(ctx context.Context, req *logical.Request, d } } else { if batchResponseItems[0].Error != "" { - p.Unlock() - if internalErrorInBatch { return nil, errutil.InternalError{Err: batchResponseItems[0].Error} } @@ -256,8 +255,6 @@ func (b *backend) pathDecryptWrite(ctx context.Context, req *logical.Request, d } } - p.Unlock() - return batchRequestResponse(d, resp, req, successesInBatch, userErrorInBatch, internalErrorInBatch) } diff --git a/builtin/logical/transit/path_encrypt.go b/builtin/logical/transit/path_encrypt.go index 0b6c98e3aa63..05199cae09b1 100644 --- a/builtin/logical/transit/path_encrypt.go +++ b/builtin/logical/transit/path_encrypt.go @@ -460,6 +460,7 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d if !b.System().CachingDisabled() { p.Lock(false) } + defer p.Unlock() // Process batch request items. If encryption of any request // item fails, respectively mark the error in the response @@ -547,8 +548,6 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d } } else { if batchResponseItems[0].Error != "" { - p.Unlock() - if internalErrorInBatch { return nil, errutil.InternalError{Err: batchResponseItems[0].Error} } @@ -570,8 +569,6 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d resp.AddWarning("Attempted creation of the key during the encrypt operation, but it was created beforehand") } - p.Unlock() - return batchRequestResponse(d, resp, req, successesInBatch, userErrorInBatch, internalErrorInBatch) } diff --git a/builtin/logical/transit/path_hmac.go b/builtin/logical/transit/path_hmac.go index b589122550f7..0465b8dfa2be 100644 --- a/builtin/logical/transit/path_hmac.go +++ b/builtin/logical/transit/path_hmac.go @@ -136,6 +136,7 @@ func (b *backend) pathHMACWrite(ctx context.Context, req *logical.Request, d *fr if !b.System().CachingDisabled() { p.Lock(false) } + defer p.Unlock() switch { case ver == 0: @@ -145,23 +146,19 @@ func (b *backend) pathHMACWrite(ctx context.Context, req *logical.Request, d *fr case ver == p.LatestVersion: // Allowed case p.MinEncryptionVersion > 0 && ver < p.MinEncryptionVersion: - p.Unlock() return logical.ErrorResponse("cannot generate HMAC: version is too old (disallowed by policy)"), logical.ErrInvalidRequest } key, err := p.HMACKey(ver) if err != nil { - p.Unlock() return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } if key == nil && p.Type != keysutil.KeyType_MANAGED_KEY { - p.Unlock() return nil, fmt.Errorf("HMAC key value could not be computed") } hashAlgorithm, ok := keysutil.HashTypeMap[algorithm] if !ok { - p.Unlock() return logical.ErrorResponse("unsupported algorithm %q", hashAlgorithm), nil } @@ -172,18 +169,15 @@ func (b *backend) pathHMACWrite(ctx context.Context, req *logical.Request, d *fr if batchInputRaw != nil { err = mapstructure.Decode(batchInputRaw, &batchInputItems) if err != nil { - p.Unlock() return nil, fmt.Errorf("failed to parse batch input: %w", err) } if len(batchInputItems) == 0 { - p.Unlock() return logical.ErrorResponse("missing batch input to process"), logical.ErrInvalidRequest } } else { valueRaw, ok := d.GetOk("input") if !ok { - p.Unlock() return logical.ErrorResponse("missing input for HMAC"), logical.ErrInvalidRequest } @@ -233,8 +227,6 @@ func (b *backend) pathHMACWrite(ctx context.Context, req *logical.Request, d *fr response[i].HMAC = retStr } - p.Unlock() - // Generate the response resp := &logical.Response{} if batchInputRaw != nil { @@ -282,10 +274,10 @@ func (b *backend) pathHMACVerify(ctx context.Context, req *logical.Request, d *f if !b.System().CachingDisabled() { p.Lock(false) } + defer p.Unlock() hashAlgorithm, ok := keysutil.HashTypeMap[algorithm] if !ok { - p.Unlock() return logical.ErrorResponse("unsupported algorithm %q", hashAlgorithm), nil } @@ -296,12 +288,10 @@ func (b *backend) pathHMACVerify(ctx context.Context, req *logical.Request, d *f if batchInputRaw != nil { err := mapstructure.Decode(batchInputRaw, &batchInputItems) if err != nil { - p.Unlock() return nil, fmt.Errorf("failed to parse batch input: %w", err) } if len(batchInputItems) == 0 { - p.Unlock() return logical.ErrorResponse("missing batch input to process"), logical.ErrInvalidRequest } } else { @@ -398,8 +388,6 @@ func (b *backend) pathHMACVerify(ctx context.Context, req *logical.Request, d *f response[i].Valid = hmac.Equal(retBytes, verBytes) } - p.Unlock() - // Generate the response resp := &logical.Response{} if batchInputRaw != nil { diff --git a/builtin/logical/transit/path_rewrap.go b/builtin/logical/transit/path_rewrap.go index 15a213748c4b..49b69c7255e1 100644 --- a/builtin/logical/transit/path_rewrap.go +++ b/builtin/logical/transit/path_rewrap.go @@ -148,6 +148,7 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * if !b.System().CachingDisabled() { p.Lock(false) } + defer p.Unlock() warnAboutNonceUsage := false for i, item := range batchInputItems { @@ -167,7 +168,6 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * batchResponseItems[i].Error = err.Error() continue default: - p.Unlock() return nil, err } } @@ -183,16 +183,13 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * batchResponseItems[i].Error = err.Error() continue case errutil.InternalError: - p.Unlock() return nil, err default: - p.Unlock() return nil, err } } if ciphertext == "" { - p.Unlock() return nil, fmt.Errorf("empty ciphertext returned for input item %d", i) } @@ -216,7 +213,6 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * } } else { if batchResponseItems[0].Error != "" { - p.Unlock() return logical.ErrorResponse(batchResponseItems[0].Error), logical.ErrInvalidRequest } resp.Data = map[string]interface{}{ @@ -229,7 +225,6 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } - p.Unlock() return resp, nil } diff --git a/builtin/logical/transit/path_sign_verify.go b/builtin/logical/transit/path_sign_verify.go index 8e80723d10c9..3307c5ca99b9 100644 --- a/builtin/logical/transit/path_sign_verify.go +++ b/builtin/logical/transit/path_sign_verify.go @@ -367,9 +367,9 @@ func (b *backend) pathSignWrite(ctx context.Context, req *logical.Request, d *fr if !b.System().CachingDisabled() { p.Lock(false) } + defer p.Unlock() if !p.Type.SigningSupported() { - p.Unlock() return logical.ErrorResponse(fmt.Sprintf("key type %v does not support signing", p.Type)), logical.ErrInvalidRequest } @@ -385,12 +385,10 @@ func (b *backend) pathSignWrite(ctx context.Context, req *logical.Request, d *fr if batchInputRaw != nil { err = mapstructure.Decode(batchInputRaw, &batchInputItems) if err != nil { - p.Unlock() return nil, fmt.Errorf("failed to parse batch input: %w", err) } if len(batchInputItems) == 0 { - p.Unlock() return logical.ErrorResponse("missing batch input to process"), logical.ErrInvalidRequest } } else { @@ -403,7 +401,6 @@ func (b *backend) pathSignWrite(ctx context.Context, req *logical.Request, d *fr } response := make([]batchResponseSignItem, len(batchInputItems)) - for i, item := range batchInputItems { rawInput, ok := item["input"] @@ -491,7 +488,6 @@ func (b *backend) pathSignWrite(ctx context.Context, req *logical.Request, d *fr } } else { if response[0].Error != "" || response[0].err != nil { - p.Unlock() if response[0].Error != "" { return logical.ErrorResponse(response[0].Error), response[0].err } @@ -509,7 +505,6 @@ func (b *backend) pathSignWrite(ctx context.Context, req *logical.Request, d *fr } } - p.Unlock() return resp, nil } @@ -625,9 +620,9 @@ func (b *backend) pathVerifyWrite(ctx context.Context, req *logical.Request, d * if !b.System().CachingDisabled() { p.Lock(false) } + defer p.Unlock() if !p.Type.SigningSupported() { - p.Unlock() return logical.ErrorResponse(fmt.Sprintf("key type %v does not support verification", p.Type)), logical.ErrInvalidRequest } @@ -732,7 +727,6 @@ func (b *backend) pathVerifyWrite(ctx context.Context, req *logical.Request, d * } } else { if response[0].Error != "" || response[0].err != nil { - p.Unlock() if response[0].Error != "" { return logical.ErrorResponse(response[0].Error), response[0].err } @@ -743,7 +737,6 @@ func (b *backend) pathVerifyWrite(ctx context.Context, req *logical.Request, d * } } - p.Unlock() return resp, nil } diff --git a/changelog/25336.txt b/changelog/25336.txt new file mode 100644 index 000000000000..a1f32a444a4c --- /dev/null +++ b/changelog/25336.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/transit: When provided an invalid input with hash_algorithm=none, a lock was not released properly before reporting an error leading to deadlocks on a subsequent key configuration update. +```