Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix transit import/export of hmac-only keys #20864

Merged
merged 4 commits into from
May 31, 2023
Merged

Fix transit import/export of hmac-only keys #20864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
539796e
Fix export of HMAC typed keys
cipherboy May 30, 2023
deb37ba
Consistently handle HMAC keys in keysutil
cipherboy May 30, 2023
1a89040
Add test proving export of HMAC keys work
cipherboy May 31, 2023
513eef5
Add changelog entry
cipherboy May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions builtin/logical/transit/path_backup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func TestTransit_BackupRestore(t *testing.T) {
testBackupRestore(t, "rsa-2048", "hmac-verify")
testBackupRestore(t, "rsa-3072", "hmac-verify")
testBackupRestore(t, "rsa-4096", "hmac-verify")
testBackupRestore(t, "hmac", "hmac-verify")
}

func testBackupRestore(t *testing.T, keyType, feature string) {
Expand All @@ -57,6 +58,9 @@ func testBackupRestore(t *testing.T, keyType, feature string) {
"exportable": true,
},
}
if keyType == "hmac" {
keyReq.Data["key_size"] = 32
}
resp, err = b.HandleRequest(context.Background(), keyReq)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("resp: %#v\nerr: %v", resp, err)
Expand Down
6 changes: 5 additions & 1 deletion builtin/logical/transit/path_byok_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ func TestTransit_BYOKExportImport(t *testing.T) {
testBYOKExportImport(t, "rsa-3072", "sign-verify")
testBYOKExportImport(t, "rsa-4096", "sign-verify")

// Unlike backup, we don't support importing HMAC keys here.
// Test HMAC sign/verify after a restore for supported keys.
testBYOKExportImport(t, "hmac", "hmac-verify")
}

func testBYOKExportImport(t *testing.T, keyType, feature string) {
Expand All @@ -47,6 +48,9 @@ func testBYOKExportImport(t *testing.T, keyType, feature string) {
"exportable": true,
},
}
if keyType == "hmac" {
keyReq.Data["key_size"] = 32
}
resp, err = b.HandleRequest(context.Background(), keyReq)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("resp: %#v\nerr: %v", resp, err)
Expand Down
6 changes: 5 additions & 1 deletion builtin/logical/transit/path_export.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,11 @@ func getExportKey(policy *keysutil.Policy, key *keysutil.KeyEntry, exportType st

switch exportType {
case exportTypeHMACKey:
return strings.TrimSpace(base64.StdEncoding.EncodeToString(key.HMACKey)), nil
src := key.HMACKey
if policy.Type == keysutil.KeyType_HMAC {
src = key.Key
}
return strings.TrimSpace(base64.StdEncoding.EncodeToString(src)), nil

case exportTypeEncryptionKey:
switch policy.Type {
Expand Down
4 changes: 4 additions & 0 deletions builtin/logical/transit/path_export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func TestTransit_Export_KeyVersion_ExportsCorrectVersion(t *testing.T) {
verifyExportsCorrectVersion(t, "hmac-key", "ecdsa-p384")
verifyExportsCorrectVersion(t, "hmac-key", "ecdsa-p521")
verifyExportsCorrectVersion(t, "hmac-key", "ed25519")
verifyExportsCorrectVersion(t, "hmac-key", "hmac")
}

func verifyExportsCorrectVersion(t *testing.T, exportType, keyType string) {
Expand All @@ -43,6 +44,9 @@ func verifyExportsCorrectVersion(t *testing.T, exportType, keyType string) {
"exportable": true,
"type": keyType,
}
if keyType == "hmac" {
req.Data["key_size"] = 32
}
_, err := b.HandleRequest(context.Background(), req)
if err != nil {
t.Fatal(err)
Expand Down
5 changes: 5 additions & 0 deletions changelog/20864.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
```release-note:bug
secrets/transit: Fix export of HMAC-only key, correctly exporting the key used for sign operations. For consumers of the previously incorrect key, use the plaintext export to retrieve these incorrect keys and import them as new versions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - these didn't format properly in the changelog. I think you might need a different backtick section for each bullet. Can you check the docs? Don't fix this file, just for next time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thank you! I thought I had seen an example like this, but maybe it was also fixed; perhaps we should fix this one, so that we know its not correct?

https://github.com/hashicorp/vault/blob/main/changelog/README.md doesn't really give any indication of how to format changelog files with multiple impacts, probably worth calling out, tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the upstream docs do call it out: https://github.com/hashicorp/go-changelog#change-file-formatting :

Sometimes PRs have multiple changelog entries associated with them. In this case, use multiple blocks.

secrets/transit: Fix bug related to shorter dedicated HMAC key sizing.
sdk/helper/keysutil: New HMAC type policies will have HMACKey equal to Key and be copied over on import.
```
12 changes: 11 additions & 1 deletion sdk/helper/keysutil/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,10 @@ func (p *Policy) Upgrade(ctx context.Context, storage logical.Storage, randReade
entry.HMACKey = hmacKey
p.Keys[strconv.Itoa(p.LatestVersion)] = entry
persistNeeded = true

if p.Type == KeyType_HMAC {
entry.HMACKey = entry.Key
}
}

if persistNeeded {
Expand Down Expand Up @@ -1497,6 +1501,7 @@ func (p *Policy) ImportPublicOrPrivate(ctx context.Context, storage logical.Stor
entry.Key = key
if p.Type == KeyType_HMAC {
p.KeySize = len(key)
entry.HMACKey = key
}
} else {
var parsedKey any
Expand Down Expand Up @@ -1613,7 +1618,7 @@ func (p *Policy) RotateInMemory(randReader io.Reader) (retErr error) {
if p.Type == KeyType_AES128_GCM96 {
numBytes = 16
} else if p.Type == KeyType_HMAC {
numBytes := p.KeySize
numBytes = p.KeySize
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shadowing was causing HMAC keys to be shorter than expected.

if numBytes < HmacMinKeySize || numBytes > HmacMaxKeySize {
return fmt.Errorf("invalid key size for HMAC key, must be between %d and %d bytes", HmacMinKeySize, HmacMaxKeySize)
}
Expand All @@ -1624,6 +1629,11 @@ func (p *Policy) RotateInMemory(randReader io.Reader) (retErr error) {
}
entry.Key = newKey

if p.Type == KeyType_HMAC {
// To avoid causing problems, ensure HMACKey = Key.
entry.HMACKey = newKey
}

case KeyType_ECDSA_P256, KeyType_ECDSA_P384, KeyType_ECDSA_P521:
var curve elliptic.Curve
switch p.Type {
Expand Down