Skip to content

Commit 622be32

Browse files
hslatmanhslatman
authored
Merge pull request #1134 from smallstep/herman/mackms-label-or-hash
Add support for creating signers with (just) `hash` on MacKMS
2 parents 7263a69 + 9ec7bfe commit 622be32

2 files changed

Lines changed: 54 additions & 26 deletions

File tree

‎kms/mackms/mackms.go‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -182,11 +182,13 @@ var signatureAlgorithmMapping = map[apiv1.SignatureAlgorithm]algorithmAttributes
182182
// ones:
183183
// - my-name
184184
// - mackms:label=my-name;tag=com.smallstep.crypto;hash=ccb792f9d9a1262bfb814a339876f825bdba1261
185+
// - mackms:tag=com.smallstep.crypto;hash=ccb792f9d9a1262bfb814a339876f825bdba1261
185186
//
186187
// The above URIs support the following attributes:
187-
// - "label" corresponds with Apple's kSecAttrLabel. It is always required and
188-
// represents the key name. You will be able to see the keys in the Keychain,
189-
// looking for the value.
188+
// - "label" corresponds with Apple's kSecAttrLabel. It represents the key name.
189+
// You will be able to see the keys in the keychain, looking for the value.
190+
// It is required when calling CreateKey and DeleteKey. It is required when
191+
// calling GetPublicKey and CreateSigner, unless "hash" is set.
190192
// - "tag" corresponds with kSecAttrApplicationTag. It defaults to
191193
// com.smallstep.crypto. If tag is an empty string ("tag="), the attribute
192194
// will not be set.
@@ -242,7 +244,7 @@ func (k *MacKMS) GetPublicKey(req *apiv1.GetPublicKeyRequest) (crypto.PublicKey,
242244
return nil, fmt.Errorf("getPublicKeyRequest 'name' cannot be empty")
243245
}
244246

245-
u, err := parseURI(req.Name)
247+
u, err := parseURI(req.Name, false) // lookup key by label and/or hash
246248
if err != nil {
247249
return nil, fmt.Errorf("mackms GetPublicKey failed: %w", err)
248250
}
@@ -268,7 +270,7 @@ func (k *MacKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespons
268270
return nil, fmt.Errorf("createKeyRequest 'name' cannot be empty")
269271
}
270272

271-
u, err := parseURI(req.Name)
273+
u, err := parseURI(req.Name, true) // creation always requires a label
272274
if err != nil {
273275
return nil, fmt.Errorf("mackms CreateKey failed: %w", err)
274276
}
@@ -411,7 +413,7 @@ func (k *MacKMS) CreateSigner(req *apiv1.CreateSignerRequest) (crypto.Signer, er
411413
return nil, fmt.Errorf("createSignerRequest 'signingKey' cannot be empty")
412414
}
413415

414-
u, err := parseURI(req.SigningKey)
416+
u, err := parseURI(req.SigningKey, false) // lookup key by label and/or hash
415417
if err != nil {
416418
return nil, fmt.Errorf("mackms CreateSigner failed: %w", err)
417419
}
@@ -658,7 +660,7 @@ func (*MacKMS) DeleteKey(req *apiv1.DeleteKeyRequest) error {
658660
return fmt.Errorf("deleteKeyRequest 'name' cannot be empty")
659661
}
660662

661-
u, err := parseURI(req.Name)
663+
u, err := parseURI(req.Name, true) // deletion always requires a label
662664
if err != nil {
663665
return fmt.Errorf("mackms DeleteKey failed: %w", err)
664666
}
@@ -1384,7 +1386,7 @@ func storeCertificate(u *certAttributes, cert *x509.Certificate) error {
13841386
return nil
13851387
}
13861388

1387-
func parseURI(rawuri string) (*keyAttributes, error) {
1389+
func parseURI(rawuri string, requireLabel bool) (*keyAttributes, error) {
13881390
// When rawuri is just the key name
13891391
if !strings.HasPrefix(strings.ToLower(rawuri), Scheme) {
13901392
return &keyAttributes{
@@ -1416,17 +1418,24 @@ func parseURI(rawuri string) (*keyAttributes, error) {
14161418
// With regular values, uris look like this:
14171419
// mackms:label=my-key;tag=my-tag;hash=010a...;se=true;bio=true
14181420
label := u.Get("label")
1419-
if label == "" {
1421+
if requireLabel && label == "" {
14201422
return nil, fmt.Errorf("error parsing %q: label is required", rawuri)
14211423
}
1424+
1425+
hash := u.GetEncoded("hash")
1426+
if label == "" && len(hash) == 0 {
1427+
return nil, fmt.Errorf("error parsing %q: one of label or hash is required", rawuri)
1428+
}
1429+
14221430
tag := u.Get("tag")
14231431
if tag == "" && !u.Has("tag") {
14241432
tag = DefaultTag
14251433
}
1434+
14261435
return &keyAttributes{
14271436
label: label,
14281437
tag: tag,
1429-
hash: u.GetEncoded("hash"),
1438+
hash: hash,
14301439
retry: !u.Has("tag"),
14311440
useSecureEnclave: u.GetBool("se"),
14321441
useBiometrics: u.GetBool("bio"),

‎kms/mackms/mackms_test.go‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func mustCreateKey(t *testing.T, name string, signatureAlgorithm apiv1.Signature
7474
func createPrivateKeyOnly(t *testing.T, name string, signatureAlgorithm apiv1.SignatureAlgorithm) *apiv1.CreateKeyResponse {
7575
t.Helper()
7676

77-
u, err := parseURI(name)
77+
u, err := parseURI(name, true)
7878
require.NoError(t, err)
7979
u.sigAlgorithm = signatureAlgorithm
8080
u.keySize = signatureAlgorithmMapping[signatureAlgorithm].Size
@@ -356,6 +356,11 @@ func TestMacKMS_GetPublicKey(t *testing.T) {
356356
}))
357357
})
358358

359+
u, err := uri.ParseWithScheme(Scheme, r1.Name)
360+
require.NoError(t, err)
361+
hash := u.Get("hash")
362+
require.NotEmpty(t, hash)
363+
359364
type args struct {
360365
req *apiv1.GetPublicKeyRequest
361366
}
@@ -375,8 +380,10 @@ func TestMacKMS_GetPublicKey(t *testing.T) {
375380
{"ok uri simple", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:test-p256"}}, r1.PublicKey, assert.NoError},
376381
{"ok uri label", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-p256"}}, r1.PublicKey, assert.NoError},
377382
{"ok uri label + tag", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-p256;tag=com.smallstep.crypto"}}, r1.PublicKey, assert.NoError},
383+
{"ok uri hash", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: fmt.Sprintf("mackms:hash=%s", hash)}}, r1.PublicKey, assert.NoError},
378384
{"fail bad label", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-fail-p256"}}, nil, assert.Error},
379385
{"fail bad tag", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:label=test-p256;tag=com.step.crypto"}}, nil, assert.Error},
386+
{"fail no label nor hash", &MacKMS{}, args{&apiv1.GetPublicKeyRequest{Name: "mackms:tag=com.step.crypto"}}, nil, assert.Error},
380387
}
381388
for _, tt := range tests {
382389
t.Run(tt.name, func(t *testing.T) {
@@ -418,7 +425,7 @@ func TestMacKMS_CreateKey(t *testing.T) {
418425
require.Nil(tt, resp.PrivateKey)
419426
require.NotEmpty(tt, resp.CreateSignerRequest)
420427

421-
u, err := parseURI(resp.Name)
428+
u, err := parseURI(resp.Name, true)
422429
require.NoError(tt, err)
423430
require.NotEmpty(tt, u.label)
424431
require.NotEmpty(tt, u.tag)
@@ -433,7 +440,7 @@ func TestMacKMS_CreateKey(t *testing.T) {
433440
require.Nil(tt, resp.PrivateKey)
434441
require.NotEmpty(tt, resp.CreateSignerRequest)
435442

436-
u, err := parseURI(resp.Name)
443+
u, err := parseURI(resp.Name, true)
437444
require.NoError(tt, err)
438445
require.NotEmpty(tt, u.label)
439446
require.Empty(tt, u.tag)
@@ -462,7 +469,7 @@ func TestMacKMS_CreateSigner(t *testing.T) {
462469
kms := &MacKMS{}
463470
resp, err := kms.CreateKey(&apiv1.CreateKeyRequest{
464471
Name: "mackms:label=test-p256",
465-
SignatureAlgorithm: apiv1.SHA256WithRSA,
472+
SignatureAlgorithm: apiv1.ECDSAWithSHA256,
466473
})
467474
require.NoError(t, err)
468475

@@ -472,6 +479,11 @@ func TestMacKMS_CreateSigner(t *testing.T) {
472479
}))
473480
})
474481

482+
u, err := uri.ParseWithScheme(Scheme, resp.Name)
483+
require.NoError(t, err)
484+
hash := u.Get("hash")
485+
require.NotEmpty(t, hash)
486+
475487
assertSigner := func(tt require.TestingT, i1 any, i2 ...any) {
476488
require.IsType(tt, &Signer{}, i1)
477489
signer := i1.(crypto.Signer)
@@ -507,6 +519,9 @@ func TestMacKMS_CreateSigner(t *testing.T) {
507519
{"ok simple name", &MacKMS{}, args{&apiv1.CreateSignerRequest{
508520
SigningKey: "mackms:label=test-p256",
509521
}}, assertSigner, assert.NoError},
522+
{"ok hash", &MacKMS{}, args{&apiv1.CreateSignerRequest{
523+
SigningKey: fmt.Sprintf("mackms:hash=%s", hash),
524+
}}, assertSigner, assert.NoError},
510525
{"fail signingKey", &MacKMS{}, args{&apiv1.CreateSignerRequest{}}, require.Nil, assert.Error},
511526
{"fail uri", &MacKMS{}, args{&apiv1.CreateSignerRequest{SigningKey: "mackms:tag=foo"}}, require.Nil, assert.Error},
512527
{"fail missing", &MacKMS{}, args{&apiv1.CreateSignerRequest{SigningKey: "mackms:label=test-p384"}}, require.Nil, assert.Error},
@@ -552,26 +567,30 @@ func TestMacKMS_DeleteKey(t *testing.T) {
552567

553568
func Test_parseURI(t *testing.T) {
554569
type args struct {
555-
rawuri string
570+
rawuri string
571+
requireLabel bool
556572
}
557573
tests := []struct {
558574
name string
559575
args args
560576
want *keyAttributes
561577
assertion assert.ErrorAssertionFunc
562578
}{
563-
{"ok", args{"mackms:label=the-label;tag=the-tag;hash=0102abcd"}, &keyAttributes{label: "the-label", tag: "the-tag", hash: []byte{1, 2, 171, 205}}, assert.NoError},
564-
{"ok label", args{"the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
565-
{"ok label uri", args{"mackms:label=the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
566-
{"ok label uri simple", args{"mackms:the-label"}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
567-
{"ok label empty tag", args{"mackms:label=the-label;tag="}, &keyAttributes{label: "the-label", tag: ""}, assert.NoError},
568-
{"ok label empty tag no equal", args{"mackms:label=the-label;tag"}, &keyAttributes{label: "the-label", tag: ""}, assert.NoError},
569-
{"fail parse", args{"mackms:%label=the-label"}, nil, assert.Error},
570-
{"fail missing label", args{"mackms:hash=0102abcd"}, nil, assert.Error},
579+
{"ok", args{"mackms:label=the-label;tag=the-tag;hash=0102abcd", true}, &keyAttributes{label: "the-label", tag: "the-tag", hash: []byte{1, 2, 171, 205}}, assert.NoError},
580+
{"ok label", args{"the-label", true}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
581+
{"ok label uri", args{"mackms:label=the-label", true}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
582+
{"ok label uri simple", args{"mackms:the-label", true}, &keyAttributes{label: "the-label", tag: DefaultTag, retry: true}, assert.NoError},
583+
{"ok label empty tag", args{"mackms:label=the-label;tag=", true}, &keyAttributes{label: "the-label", tag: ""}, assert.NoError},
584+
{"ok label empty tag no equal", args{"mackms:label=the-label;tag", true}, &keyAttributes{label: "the-label", tag: ""}, assert.NoError},
585+
{"ok empty label and tag with hash", args{"mackms:hash=0102abcd", false}, &keyAttributes{label: "", tag: DefaultTag, hash: []byte{1, 2, 171, 205}, retry: true}, assert.NoError},
586+
{"ok empty label with tag and hash", args{"mackms:hash=0102abcd;tag=the-tag", false}, &keyAttributes{label: "", tag: "the-tag", hash: []byte{1, 2, 171, 205}}, assert.NoError},
587+
{"fail parse", args{"mackms:%label=the-label", true}, nil, assert.Error},
588+
{"fail missing label", args{"mackms:hash=0102abcd", true}, nil, assert.Error},
589+
{"fail missing label and hash", args{"mackms:tag=the-tag", false}, nil, assert.Error},
571590
}
572591
for _, tt := range tests {
573592
t.Run(tt.name, func(t *testing.T) {
574-
got, err := parseURI(tt.args.rawuri)
593+
got, err := parseURI(tt.args.rawuri, tt.args.requireLabel)
575594
tt.assertion(t, err)
576595
assert.Equal(t, tt.want, got)
577596
})
@@ -2158,7 +2177,7 @@ func Test_keyAttributes_retryAttributes(t *testing.T) {
21582177

21592178
mustFields := func(s string) fields {
21602179
t.Helper()
2161-
u, err := parseURI(s)
2180+
u, err := parseURI(s, true)
21622181
require.NoError(t, err)
21632182
return fields{
21642183
label: u.label,
@@ -2239,7 +2258,7 @@ func Test_createHash(t *testing.T) {
22392258

22402259
getHash := func(r *apiv1.CreateKeyResponse) []byte {
22412260
t.Helper()
2242-
u, err := parseURI(r.Name)
2261+
u, err := parseURI(r.Name, true)
22432262
require.NoError(t, err)
22442263
return u.hash
22452264
}

0 commit comments

Comments
 (0)