From d8fcbfe22febe8b21f432279a6a1f774fa66c401 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 14 Mar 2024 20:09:08 -0700 Subject: [PATCH 1/8] Add store and load certificates in mackms This commit adds support for loading and storing simple certificates using LoadCertificate and StoreCertificate --- .../corefoundation/core_foundation_darwin.go | 18 +- internal/darwin/security/security_darwin.go | 58 +++- kms/mackms/mackms.go | 225 +++++++++++++- kms/mackms/mackms_test.go | 280 ++++++++++++++++++ 4 files changed, 572 insertions(+), 9 deletions(-) diff --git a/internal/darwin/corefoundation/core_foundation_darwin.go b/internal/darwin/corefoundation/core_foundation_darwin.go index cb2e967c..89f5055e 100644 --- a/internal/darwin/corefoundation/core_foundation_darwin.go +++ b/internal/darwin/corefoundation/core_foundation_darwin.go @@ -20,9 +20,15 @@ package corefoundation /* -#cgo LDFLAGS: -framework CoreFoundation +#cgo CFLAGS: -x objective-c +#cgo LDFLAGS: -framework CoreFoundation -framework Foundation +#include #include + +void nslog(CFTypeRef ref) { + NSLog(@"%@", ref); +} */ import "C" import ( @@ -159,6 +165,12 @@ func NewDictionary(m Dictionary) (*DictionaryRef, error) { }, nil } +func NewDictionaryRef(ref TypeRef) *DictionaryRef { + return &DictionaryRef{ + Value: C.CFDictionaryRef(ref), + } +} + func (v *DictionaryRef) Release() { Release(v) } func (v *DictionaryRef) TypeRef() CFTypeRef { return C.CFTypeRef(v.Value) } @@ -181,3 +193,7 @@ func (e ErrorRef) Error() string { func (e ErrorRef) Release() { Release(e) } func (e ErrorRef) TypeRef() CFTypeRef { return C.CFTypeRef(C.CFErrorRef(e)) } + +func Log(v TypeReferer) { + C.nslog(v.TypeRef()) +} diff --git a/internal/darwin/security/security_darwin.go b/internal/darwin/security/security_darwin.go index 67ec5ff8..2f9a1116 100644 --- a/internal/darwin/security/security_darwin.go +++ b/internal/darwin/security/security_darwin.go @@ -38,9 +38,14 @@ const ( nilSecKey C.SecKeyRef = 0 nilSecAccessControl C.SecAccessControlRef = 0 nilCFString C.CFStringRef = 0 + nilCFData C.CFDataRef = 0 ) -var ErrNotFound = errors.New("not found") +var ( + ErrNotFound = errors.New("not found") + ErrAlreadyExists = errors.New("already exists") + ErrInvalidData = errors.New("invalid data") +) var ( KSecAttrAccessControl = cf.TypeRef(C.kSecAttrAccessControl) @@ -62,14 +67,19 @@ var ( KSecAttrLabel = cf.TypeRef(C.kSecAttrLabel) KSecAttrTokenID = cf.TypeRef(C.kSecAttrTokenID) KSecAttrTokenIDSecureEnclave = cf.TypeRef(C.kSecAttrTokenIDSecureEnclave) + KSecAttrSerialNumber = cf.TypeRef(C.kSecAttrSerialNumber) + KSecAttrIssuer = cf.TypeRef(C.kSecAttrIssuer) KSecClass = cf.TypeRef(C.kSecClass) KSecClassKey = cf.TypeRef(C.kSecClassKey) + KSecClassCertificate = cf.TypeRef(C.kSecClassCertificate) + KSecClassIdentity = cf.TypeRef(C.kSecClassIdentity) KSecMatchLimit = cf.TypeRef(C.kSecMatchLimit) KSecMatchLimitOne = cf.TypeRef(C.kSecMatchLimitOne) KSecPublicKeyAttrs = cf.TypeRef(C.kSecPublicKeyAttrs) KSecPrivateKeyAttrs = cf.TypeRef(C.kSecPrivateKeyAttrs) KSecReturnRef = cf.TypeRef(C.kSecReturnRef) KSecValueRef = cf.TypeRef(C.kSecValueRef) + KSecValueData = cf.TypeRef(C.kSecValueData) ) type SecKeyAlgorithm = C.SecKeyAlgorithm @@ -135,6 +145,19 @@ func NewSecKeyRef(ref cf.TypeRef) *SecKeyRef { func (v *SecKeyRef) Release() { cf.Release(v) } func (v *SecKeyRef) TypeRef() cf.CFTypeRef { return cf.CFTypeRef(v.Value) } +type SecCertificateRef struct { + Value C.SecCertificateRef +} + +func NewSecCertificateRef(ref cf.TypeRef) *SecCertificateRef { + return &SecCertificateRef{ + Value: C.SecCertificateRef(ref), + } +} + +func (v *SecCertificateRef) Release() { cf.Release(v) } +func (v *SecCertificateRef) TypeRef() cf.CFTypeRef { return cf.CFTypeRef(v.Value) } + type SecAccessControlRef struct { ref C.SecAccessControlRef } @@ -147,6 +170,11 @@ func SecItemAdd(attributes *cf.DictionaryRef, result *cf.TypeRef) error { return goOSStatus(status) } +func SecItemUpdate(query *cf.DictionaryRef, attributesToUpdate *cf.DictionaryRef) error { + status := C.SecItemUpdate(C.CFDictionaryRef(query.Value), C.CFDictionaryRef(attributesToUpdate.Value)) + return goOSStatus(status) +} + func SecItemDelete(query *cf.DictionaryRef) error { status := C.SecItemDelete(C.CFDictionaryRef(query.Value)) return goOSStatus(status) @@ -218,6 +246,26 @@ func SecKeyCreateSignature(key *SecKeyRef, algorithm SecKeyAlgorithm, dataToSign }, nil } +func SecCertificateCopyData(cert *SecCertificateRef) (*cf.DataRef, error) { + data := C.SecCertificateCopyData(cert.Value) + if data == nilCFData { + return nil, ErrInvalidData + } + return &cf.DataRef{ + Value: cf.CFDataRef(data), + }, nil +} + +func SecCertificateCreateWithData(certData *cf.DataRef) (*SecCertificateRef, error) { + certRef := C.SecCertificateCreateWithData(C.kCFAllocatorDefault, C.CFDataRef(certData.Value)) + if certRef == 0 { + return nil, ErrInvalidData + } + return &SecCertificateRef{ + Value: certRef, + }, nil +} + func SecCopyErrorMessageString(status C.OSStatus) *cf.StringRef { s := C.SecCopyErrorMessageString(status, nil) return &cf.StringRef{ @@ -254,11 +302,13 @@ func (e osStatusError) Error() string { } func goOSStatus(status C.OSStatus) error { - if status == 0 { + switch status { + case 0: return nil - } - if status == C.errSecItemNotFound { + case C.errSecItemNotFound: // -25300 return ErrNotFound + case C.errSecDuplicateItem: // -25299 + return ErrAlreadyExists } var message string diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index e7bc8f2b..957421ba 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -57,6 +57,11 @@ type keyAttributes struct { keySize int } +type certAttributes struct { + label string + serialNumber *big.Int +} + type algorithmAttributes struct { Type string Size int @@ -316,6 +321,162 @@ func (k *MacKMS) CreateSigner(req *apiv1.CreateSignerRequest) (crypto.Signer, er }, nil } +// LoadCertificate returns an x509.Certificate by its label and/or its serial +// number. By default Apple Keychain will use the certificate common name as the +// label. +// +// Valid names (URIs) are: +// - mackms:label=test@example.com +// - mackms:serial=2c273934eda8454d2595a94497e2395a +// - mackms:label=test@example.com;serial=2c273934eda8454d2595a94497e2395a +func (k *MacKMS) LoadCertificate(req *apiv1.LoadCertificateRequest) (*x509.Certificate, error) { + if req.Name == "" { + return nil, fmt.Errorf("loadCertificateRequest 'name' cannot be empty") + } + + // Require label or serial + u, err := parseCertURI(req.Name, true) + if err != nil { + return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) + } + + query := cf.Dictionary{ + security.KSecClass: security.KSecClassCertificate, + } + if u.label != "" { + cfLabel, err := cf.NewString(u.label) + if err != nil { + return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) + } + defer cfLabel.Release() + query[security.KSecAttrLabel] = cfLabel + } + if u.serialNumber != nil { + cfSerial, err := cf.NewData(encodeSerialNumber(u.serialNumber)) + if err != nil { + return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) + } + defer cfSerial.Release() + query[security.KSecAttrSerialNumber] = cfSerial + } + + cfQuery, err := cf.NewDictionary(query) + if err != nil { + return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) + } + defer cfQuery.Release() + + var ref cf.TypeRef + if err := security.SecItemCopyMatching(cfQuery, &ref); err != nil { + return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) + } + defer ref.Release() + + data, err := security.SecCertificateCopyData(security.NewSecCertificateRef(ref)) + if err != nil { + return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) + } + defer data.Release() + + cert, err := x509.ParseCertificate(data.Bytes()) + if err != nil { + return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) + } + return cert, nil +} + +func (k *MacKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([]*x509.Certificate, error) { + return nil, apiv1.NotImplementedError{} +} + +// StoreCertificate stores a certificate in the Apple Keychain. There is no need +// to provide a label in the URI as Apple will use the CommonName as the default +// label, but if one is provided, the certificate in the Keychain will be +// updated with the given label: +// +// Valid names (URIs) are: +// - "" will use the common name as the label +// - "mackms:" will use the common +// - "mackms:label=my-label" will use "my-label" +// - "mackms:my-label" will use the "my-label" +func (k *MacKMS) StoreCertificate(req *apiv1.StoreCertificateRequest) error { + // There's not really need to require the name as macOS will use the common + // name as default. + if req.Certificate == nil { + return fmt.Errorf("storeCertificateRequest 'certificate' cannot be empty") + } + + // Do not require any parameter. Using mackms: is allowed as macOS will set + // the commonName as label. + u, err := parseCertURI(req.Name, false) + if err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + + cfData, err := cf.NewData(req.Certificate.Raw) + if err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + defer cfData.Release() + + certRef, err := security.SecCertificateCreateWithData(cfData) + if err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + defer certRef.Release() + + // Adding the label here doesn't have any effect. Apple Keychain always uses + // the commonName. + attributes, err := cf.NewDictionary(cf.Dictionary{ + security.KSecClass: security.KSecClassCertificate, + security.KSecValueRef: certRef, + }) + if err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + defer attributes.Release() + + // Store the certificate + if err := security.SecItemAdd(attributes, nil); err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + + // Update the label if necessary + if u.label != "" && u.label != req.Certificate.Subject.CommonName { + cfLabel, err := cf.NewString(u.label) + if err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + defer cfLabel.Release() + + query, err := cf.NewDictionary(cf.Dictionary{ + security.KSecValueRef: certRef, + }) + if err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + defer query.Release() + + update, err := cf.NewDictionary(cf.Dictionary{ + security.KSecAttrLabel: cfLabel, + }) + if err != nil { + return fmt.Errorf("mackms StoreCertificate failed: %w", err) + } + defer update.Release() + + if err := security.SecItemUpdate(query, update); err != nil { + return fmt.Errorf("mackms StoreCertificate failed2: %w", err) + } + } + + return nil +} + +func (k *MacKMS) StoreCertificateChain(req *apiv1.StoreCertificateChainRequest) error { + return apiv1.NotImplementedError{} +} + // DeleteKey deletes the key referenced by the URI in the request name. // // # Experimental @@ -497,10 +658,8 @@ func extractPublicKey(secKeyRef *security.SecKeyRef) (crypto.PublicKey, []byte, } func parseURI(rawuri string) (*keyAttributes, error) { - s := strings.ToLower(rawuri) - // When rawuri is just the key name - if !strings.HasPrefix(s, Scheme) { + if !strings.HasPrefix(strings.ToLower(rawuri), Scheme) { return &keyAttributes{ label: rawuri, tag: DefaultTag, @@ -508,7 +667,7 @@ func parseURI(rawuri string) (*keyAttributes, error) { } // When rawuri is a mackms uri. - u, err := uri.ParseWithScheme(Scheme, s) + u, err := uri.ParseWithScheme(Scheme, rawuri) if err != nil { return nil, err } @@ -544,6 +703,64 @@ func parseURI(rawuri string) (*keyAttributes, error) { }, nil } +func parseCertURI(rawuri string, requireValue bool) (*certAttributes, error) { + // When rawuri is just the label + if !strings.HasPrefix(strings.ToLower(rawuri), Scheme) { + return &certAttributes{ + label: rawuri, + }, nil + } + + // When rawuri is a mackms uri. + u, err := uri.ParseWithScheme(Scheme, rawuri) + if err != nil { + return nil, err + } + + // Special case for mackms:label + if len(u.Values) == 1 { + for k, v := range u.Values { + if (len(v) == 1 && v[0] == "") || len(v) == 0 { + return &certAttributes{ + label: k, + }, nil + } + } + } + + // With regular values, uris look like this: + // mackms:label=my-cert;serial=01020A0B... + label := u.Get("label") + serial := u.GetEncoded("serial") + if requireValue && label == "" && len(serial) == 0 { + return nil, fmt.Errorf("error parsing %q: label or serial are required", rawuri) + } + + var serialNumber *big.Int + if len(serial) > 0 { + serialNumber = new(big.Int).SetBytes(serial) + } + + return &certAttributes{ + label: label, + serialNumber: serialNumber, + }, nil +} + +// encodeSerialNumber encodes the serial number of a certificate in ASN.1. +// Negative serial numbers are now allowed. +func encodeSerialNumber(s *big.Int) []byte { + if s.Sign() == 0 { + return []byte{0x00} + } + b := s.Bytes() + if b[0]&0x80 != 0 { + // Pad this with 0x00 in order to stop it looking like a negative number. + return append([]byte{0x00}, b...) + } + return b +} + func parseECDSAPublicKey(raw []byte) (crypto.PublicKey, error) { switch len(raw) / 2 { case 32: // 65 bytes diff --git a/kms/mackms/mackms_test.go b/kms/mackms/mackms_test.go index fb087af2..25325d18 100644 --- a/kms/mackms/mackms_test.go +++ b/kms/mackms/mackms_test.go @@ -26,7 +26,10 @@ import ( "crypto/rand" "crypto/rsa" "crypto/sha256" + "crypto/x509" + "crypto/x509/pkix" "encoding/hex" + "math/big" "net/url" "testing" @@ -36,6 +39,7 @@ import ( "go.step.sm/crypto/internal/darwin/security" "go.step.sm/crypto/kms/apiv1" "go.step.sm/crypto/kms/uri" + "go.step.sm/crypto/minica" "go.step.sm/crypto/randutil" ) @@ -687,3 +691,279 @@ func Test_ecdhToECDSAPublicKey(t *testing.T) { }) } } + +func deleteCertificate(t *testing.T, label string, cert *x509.Certificate) { + if label == "" { + label = cert.Subject.CommonName + } + + cfLabel, err := cf.NewString(label) + require.NoError(t, err) + defer cfLabel.Release() + + serialNumber, err := cf.NewData(encodeSerialNumber(cert.SerialNumber)) + require.NoError(t, err) + defer serialNumber.Release() + + query, err := cf.NewDictionary(cf.Dictionary{ + security.KSecClass: security.KSecClassCertificate, + security.KSecAttrLabel: cfLabel, + security.KSecAttrSerialNumber: serialNumber, + }) + require.NoError(t, err) + defer query.Release() + + require.NoError(t, security.SecItemDelete(query), "SecItemDelete failed: label=%s, serial=0x%X", label, encodeSerialNumber(cert.SerialNumber)) +} + +func TestMacKMS_LoadCertificate(t *testing.T) { + testName := t.Name() + ca, err := minica.New(minica.WithName(testName)) + require.NoError(t, err) + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + cert1, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{CommonName: testName + "1@example.com"}, + EmailAddresses: []string{testName + "1@example.com"}, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + cert2, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{CommonName: testName + "2@example.com"}, + EmailAddresses: []string{testName + "2@example.com"}, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + suffix, err := randutil.Alphanumeric(8) + require.NoError(t, err) + label := "test-" + suffix + + kms := &MacKMS{} + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:", Certificate: cert1, + })) + t.Cleanup(func() { deleteCertificate(t, "", cert1) }) + + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:label=" + label, Certificate: cert2, + })) + t.Cleanup(func() { deleteCertificate(t, label, cert2) }) + + type args struct { + req *apiv1.LoadCertificateRequest + } + tests := []struct { + name string + k *MacKMS + args args + want *x509.Certificate + assertion assert.ErrorAssertionFunc + }{ + {"ok commonName", &MacKMS{}, args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + cert1.Subject.CommonName, + }}, cert1, assert.NoError}, + {"ok commonName short uri", &MacKMS{}, args{&apiv1.LoadCertificateRequest{ + Name: "mackms:" + cert1.Subject.CommonName, + }}, cert1, assert.NoError}, + {"ok commonName no uri", &MacKMS{}, args{&apiv1.LoadCertificateRequest{ + Name: cert1.Subject.CommonName, + }}, cert1, assert.NoError}, + {"ok custom label", &MacKMS{}, args{&apiv1.LoadCertificateRequest{ + Name: "mackms:label=" + label, + }}, cert2, assert.NoError}, + {"ok serial number", &MacKMS{}, args{&apiv1.LoadCertificateRequest{ + Name: "mackms:serial=" + hex.EncodeToString(cert1.SerialNumber.Bytes()), + }}, cert1, assert.NoError}, + {"fail name", &MacKMS{}, args{&apiv1.LoadCertificateRequest{}}, nil, assert.Error}, + {"fail uri", &MacKMS{}, args{&apiv1.LoadCertificateRequest{Name: "mackms:"}}, nil, assert.Error}, + {"fail missing label", &MacKMS{}, args{&apiv1.LoadCertificateRequest{Name: "mackms:label=missing-" + suffix}}, nil, assert.Error}, + {"fail missing serial", &MacKMS{}, args{&apiv1.LoadCertificateRequest{Name: "mackms:serial=010a020b030c"}}, nil, assert.Error}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + k := &MacKMS{} + got, err := k.LoadCertificate(tt.args.req) + tt.assertion(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestMacKMS_StoreCertificate(t *testing.T) { + testName := t.Name() + + ca, err := minica.New(minica.WithName(testName)) + require.NoError(t, err) + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + cert, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{CommonName: testName + "@example.com"}, + EmailAddresses: []string{testName + "@example.com"}, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + verifyCertificate := func(name, label string, cert *x509.Certificate) func(t *testing.T) { + return func(t *testing.T) { + t.Helper() + + kms := &MacKMS{} + got, err := kms.LoadCertificate(&apiv1.LoadCertificateRequest{ + Name: name, + }) + if assert.NoError(t, err) && assert.Equal(t, cert, got) { + deleteCertificate(t, label, cert) + } + } + } + commonName := func(cert *x509.Certificate) string { + return cert.Subject.CommonName + } + serial := func(cert *x509.Certificate) string { + return hex.EncodeToString(cert.SerialNumber.Bytes()) + } + randLabel := func(n int) string { + s, err := randutil.Alphanumeric(n) + require.NoError(t, err) + return s + } + + rootLabel := "test-" + randLabel(8) + intermediateLabel := "test-" + randLabel(8) + certLabel := "test-" + randLabel(8) + + type args struct { + req *apiv1.StoreCertificateRequest + } + tests := []struct { + name string + k *MacKMS + args args + verify func(*testing.T) + assertion assert.ErrorAssertionFunc + }{ + {"ok root", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:", + Certificate: ca.Root, + }}, verifyCertificate("mackms:label="+commonName(ca.Root), "", ca.Root), assert.NoError}, + {"ok intermediate", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:", + Certificate: ca.Intermediate, + }}, verifyCertificate("mackms:serial="+serial(ca.Intermediate), "", ca.Intermediate), assert.NoError}, + {"ok cert", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:", + Certificate: cert, + }}, verifyCertificate("mackms:label="+commonName(cert)+";serial="+serial(cert), "", cert), assert.NoError}, + {"ok root with label", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:label=" + rootLabel, + Certificate: ca.Root, + }}, verifyCertificate("mackms:label="+rootLabel, rootLabel, ca.Root), assert.NoError}, + {"ok intermediate with label", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: intermediateLabel, + Certificate: ca.Intermediate, + }}, verifyCertificate("mackms:serial="+serial(ca.Intermediate), intermediateLabel, ca.Intermediate), assert.NoError}, + {"ok cert with label", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:" + certLabel, + Certificate: cert, + }}, verifyCertificate("mackms:label="+certLabel+";serial="+serial(cert), certLabel, cert), assert.NoError}, + {"ok cert no name", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Certificate: cert, + }}, verifyCertificate("mackms:serial="+serial(cert), "", cert), assert.NoError}, + {"fail certificate", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:label=my-label", + }}, func(t *testing.T) {}, assert.Error}, + {"fail uri", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms", + }}, func(t *testing.T) {}, assert.Error}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + k := &MacKMS{} + tt.assertion(t, k.StoreCertificate(tt.args.req)) + tt.verify(t) + }) + } +} + +func TestMacKMS_StoreCertificate_duplicated(t *testing.T) { + ca, err := minica.New(minica.WithName(t.Name())) + require.NoError(t, err) + + suffix, err := randutil.Alphanumeric(8) + require.NoError(t, err) + label := "test-" + suffix + + type args struct { + req *apiv1.StoreCertificateRequest + } + tests := []struct { + name string + k *MacKMS + args args + cleanup func(t *testing.T) + }{ + {"ok", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:", + Certificate: ca.Root, + }}, func(t *testing.T) { + deleteCertificate(t, "", ca.Root) + }}, + {"ok with label", &MacKMS{}, args{&apiv1.StoreCertificateRequest{ + Name: "mackms:label=" + label, + Certificate: ca.Intermediate, + }}, func(t *testing.T) { + deleteCertificate(t, label, ca.Intermediate) + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Cleanup(func() { tt.cleanup(t) }) + k := &MacKMS{} + if assert.NoError(t, k.StoreCertificate(tt.args.req)) { + assert.Error(t, k.StoreCertificate(tt.args.req)) + } + }) + } +} + +func TestMariano(t *testing.T) { + _, err := parseCertURI("mackm", false) + assert.NoError(t, err) +} + +func Test_encodeSerialNumber(t *testing.T) { + getBigInt := func(s string) *big.Int { + b, err := hex.DecodeString(s) + require.NoError(t, err) + return new(big.Int).SetBytes(b) + } + getBytes := func(s string) []byte { + b, err := hex.DecodeString(s) + require.NoError(t, err) + return b + } + + type args struct { + s *big.Int + } + tests := []struct { + name string + args args + want []byte + }{ + {"ok zero", args{big.NewInt(0)}, []byte{0}}, + {"ok no pad", args{getBigInt("7df0e2ea242fd1a0650cf652aa31bfa0")}, getBytes("7df0e2ea242fd1a0650cf652aa31bfa0")}, + {"ok with pad", args{getBigInt("c4b3e6e28985f1a012aa38e7493b6f35")}, getBytes("00c4b3e6e28985f1a012aa38e7493b6f35")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, encodeSerialNumber(tt.args.s)) + }) + } +} From 27d440404df7ed3ffd8195f2dd3f194d0e87f5b0 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 15 Mar 2024 16:55:04 -0700 Subject: [PATCH 2/8] Add support for certificate chains on mackms This commit adds to mackms the methods LoadCertificateChain, StoreCertificateChain, and DeleteCertificate. --- internal/darwin/security/security_darwin.go | 5 + kms/apiv1/requests.go | 13 +- kms/mackms/mackms.go | 350 +++++++++++++++----- kms/mackms/mackms_test.go | 303 ++++++++++++++--- 4 files changed, 544 insertions(+), 127 deletions(-) diff --git a/internal/darwin/security/security_darwin.go b/internal/darwin/security/security_darwin.go index 2f9a1116..bdb46642 100644 --- a/internal/darwin/security/security_darwin.go +++ b/internal/darwin/security/security_darwin.go @@ -49,6 +49,7 @@ var ( var ( KSecAttrAccessControl = cf.TypeRef(C.kSecAttrAccessControl) + KSecAttrAccessGroup = cf.TypeRef(C.kSecAttrAccessGroup) KSecAttrAccessibleWhenUnlocked = cf.TypeRef(C.kSecAttrAccessibleWhenUnlocked) KSecAttrAccessibleWhenPasscodeSetThisDeviceOnly = cf.TypeRef(C.kSecAttrAccessibleWhenPasscodeSetThisDeviceOnly) KSecAttrAccessibleWhenUnlockedThisDeviceOnly = cf.TypeRef(C.kSecAttrAccessibleWhenUnlockedThisDeviceOnly) @@ -68,7 +69,11 @@ var ( KSecAttrTokenID = cf.TypeRef(C.kSecAttrTokenID) KSecAttrTokenIDSecureEnclave = cf.TypeRef(C.kSecAttrTokenIDSecureEnclave) KSecAttrSerialNumber = cf.TypeRef(C.kSecAttrSerialNumber) + KSecAttrSubjectKeyID = cf.TypeRef(C.kSecAttrSubjectKeyID) + KSecAttrSubject = cf.TypeRef(C.kSecAttrSubject) KSecAttrIssuer = cf.TypeRef(C.kSecAttrIssuer) + kSecAttrSynchronizable = cf.TypeRef(C.kSecAttrSynchronizable) + kSecUseDataProtectionKeychain = cf.TypeRef(C.kSecUseDataProtectionKeychain) KSecClass = cf.TypeRef(C.kSecClass) KSecClassKey = cf.TypeRef(C.kSecClassKey) KSecClassCertificate = cf.TypeRef(C.kSecClassCertificate) diff --git a/kms/apiv1/requests.go b/kms/apiv1/requests.go index 4722cf52..f1430e1c 100644 --- a/kms/apiv1/requests.go +++ b/kms/apiv1/requests.go @@ -131,7 +131,7 @@ type GetPublicKeyRequest struct { type CreateKeyRequest struct { // Name represents the key name or label used to identify a key. // - // Used by: awskms, cloudkms, azurekms, pkcs11, yubikey, tpmkms. + // Used by: awskms, cloudkms, azurekms, pkcs11, yubikey, tpmkms, mackms. Name string // SignatureAlgorithm represents the type of key to create. @@ -298,3 +298,14 @@ type CreateAttestationResponse struct { type DeleteKeyRequest struct { Name string } + +// DeleteCertificateRequest is the parameter used in the kms.DeleteCertificate +// method. +// +// # Experimental +// +// Notice: This API is EXPERIMENTAL and may be changed or removed in a later +// release. +type DeleteCertificateRequest struct { + Name string +} diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index 957421ba..3133cf30 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -21,6 +21,7 @@ package mackms import ( + "bytes" "context" "crypto" "crypto/ecdh" @@ -321,7 +322,7 @@ func (k *MacKMS) CreateSigner(req *apiv1.CreateSignerRequest) (crypto.Signer, er }, nil } -// LoadCertificate returns an x509.Certificate by its label and/or its serial +// LoadCertificate returns an x509.Certificate by its label and/or serial // number. By default Apple Keychain will use the certificate common name as the // label. // @@ -340,55 +341,14 @@ func (k *MacKMS) LoadCertificate(req *apiv1.LoadCertificateRequest) (*x509.Certi return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) } - query := cf.Dictionary{ - security.KSecClass: security.KSecClassCertificate, - } - if u.label != "" { - cfLabel, err := cf.NewString(u.label) - if err != nil { - return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) - } - defer cfLabel.Release() - query[security.KSecAttrLabel] = cfLabel - } - if u.serialNumber != nil { - cfSerial, err := cf.NewData(encodeSerialNumber(u.serialNumber)) - if err != nil { - return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) - } - defer cfSerial.Release() - query[security.KSecAttrSerialNumber] = cfSerial - } - - cfQuery, err := cf.NewDictionary(query) - if err != nil { - return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) - } - defer cfQuery.Release() - - var ref cf.TypeRef - if err := security.SecItemCopyMatching(cfQuery, &ref); err != nil { - return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) - } - defer ref.Release() - - data, err := security.SecCertificateCopyData(security.NewSecCertificateRef(ref)) + cert, err := loadCertificate(u.label, u.serialNumber, nil) if err != nil { return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) } - defer data.Release() - cert, err := x509.ParseCertificate(data.Bytes()) - if err != nil { - return nil, fmt.Errorf("mackms LoadCertificate failed: %w", err) - } return cert, nil } -func (k *MacKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([]*x509.Certificate, error) { - return nil, apiv1.NotImplementedError{} -} - // StoreCertificate stores a certificate in the Apple Keychain. There is no need // to provide a label in the URI as Apple will use the CommonName as the default // label, but if one is provided, the certificate in the Keychain will be @@ -413,70 +373,99 @@ func (k *MacKMS) StoreCertificate(req *apiv1.StoreCertificateRequest) error { return fmt.Errorf("mackms StoreCertificate failed: %w", err) } - cfData, err := cf.NewData(req.Certificate.Raw) - if err != nil { + // Store the certificate and update the label if required + if err := storeCertificate(u.label, req.Certificate); err != nil { return fmt.Errorf("mackms StoreCertificate failed: %w", err) } - defer cfData.Release() - certRef, err := security.SecCertificateCreateWithData(cfData) + return nil +} + +// LoadCertificateChain returns the leaf certificate by label and/or serial +// number and its intermediate certificates. By default Apple Keychain will use +// the certificate common name as the label. +// +// Valid names (URIs) are: +// - mackms:label=test@example.com +// - mackms:serial=2c273934eda8454d2595a94497e2395a +// - mackms:label=test@example.com;serial=2c273934eda8454d2595a94497e2395a +func (k *MacKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([]*x509.Certificate, error) { + if req.Name == "" { + return nil, fmt.Errorf("loadCertificateChainRequest 'name' cannot be empty") + } + + // Require label or serial + u, err := parseCertURI(req.Name, true) if err != nil { - return fmt.Errorf("mackms StoreCertificate failed: %w", err) + return nil, fmt.Errorf("mackms LoadCertificateChain failed: %w", err) } - defer certRef.Release() - // Adding the label here doesn't have any effect. Apple Keychain always uses - // the commonName. - attributes, err := cf.NewDictionary(cf.Dictionary{ - security.KSecClass: security.KSecClassCertificate, - security.KSecValueRef: certRef, - }) + cert, err := loadCertificate(u.label, u.serialNumber, nil) if err != nil { - return fmt.Errorf("mackms StoreCertificate failed: %w", err) + return nil, fmt.Errorf("mackms LoadCertificateChain failed1: %w", err) } - defer attributes.Release() - // Store the certificate - if err := security.SecItemAdd(attributes, nil); err != nil { - return fmt.Errorf("mackms StoreCertificate failed: %w", err) + chain := []*x509.Certificate{cert} + if isSelfSigned(cert) { + return chain, nil } - // Update the label if necessary - if u.label != "" && u.label != req.Certificate.Subject.CommonName { - cfLabel, err := cf.NewString(u.label) - if err != nil { - return fmt.Errorf("mackms StoreCertificate failed: %w", err) + // Look for the rest of intermediates skipping the root. + for { + // The Keychain stores the subject as an attribute, but it safes some of + // the values to uppercase. We cannot use the cert.RawIssuer to restrict + // more the search with KSecAttrSubjectKeyID and kSecAttrSubject. We + // would need to "normalize" it in the same way. + parent, err := loadCertificate("", nil, cert.AuthorityKeyId) + if err != nil || isSelfSigned(parent) || cert.CheckSignatureFrom(parent) != nil { + break } - defer cfLabel.Release() + cert = parent + chain = append(chain, cert) + } - query, err := cf.NewDictionary(cf.Dictionary{ - security.KSecValueRef: certRef, - }) - if err != nil { - return fmt.Errorf("mackms StoreCertificate failed: %w", err) - } - defer query.Release() + return chain, nil +} - update, err := cf.NewDictionary(cf.Dictionary{ - security.KSecAttrLabel: cfLabel, - }) - if err != nil { - return fmt.Errorf("mackms StoreCertificate failed: %w", err) - } - defer update.Release() +// StoreCertificateChain stores a certificate chain in the Apple Keychain. There +// is no need to provide a label in the URI as Apple will use the CommonName as +// the default label, but if one is provided, the leaf certificate in the +// Keychain will be updated with the given label: +// +// Valid names (URIs) are: +// - "" will use the common name as the label +// - "mackms:" will use the common +// - "mackms:label=my-label" will use "my-label" +// - "mackms:my-label" will use the "my-label" +func (k *MacKMS) StoreCertificateChain(req *apiv1.StoreCertificateChainRequest) error { + // There's not really need to require the name as macOS will use the common + // name as default. + if len(req.CertificateChain) == 0 { + return fmt.Errorf("storeCertificateChainRequest 'certificateChain' cannot be empty") + } - if err := security.SecItemUpdate(query, update); err != nil { - return fmt.Errorf("mackms StoreCertificate failed2: %w", err) + // Do not require any parameter. Using mackms: is allowed as macOS will set + // the commonName as label. + u, err := parseCertURI(req.Name, false) + if err != nil { + return fmt.Errorf("mackms StoreCertificateChain failed: %w", err) + } + + // Store the certificate and update the label if required + if err := storeCertificate(u.label, req.CertificateChain[0]); err != nil { + return fmt.Errorf("mackms StoreCertificateChain failed: %w", err) + } + + // Store the rest of the chain but do not fail if already exists + for _, cert := range req.CertificateChain[1:] { + if err := storeCertificate("", cert); err != nil && !errors.Is(err, security.ErrAlreadyExists) { + return fmt.Errorf("mackms StoreCertificateChain failed: %w", err) } } return nil } -func (k *MacKMS) StoreCertificateChain(req *apiv1.StoreCertificateChainRequest) error { - return apiv1.NotImplementedError{} -} - // DeleteKey deletes the key referenced by the URI in the request name. // // # Experimental @@ -514,8 +503,52 @@ func (*MacKMS) DeleteKey(req *apiv1.DeleteKeyRequest) error { } // Extract logic to deleteItem to avoid defer on loops if err := deleteItem(dict, u.hash); err != nil { - return err + return fmt.Errorf("mackms DeleteKey failed: %w", err) + } + } + + return nil +} + +// DeleteCertificateRequest deletes the certificate referenced by the URI in the +// request name. +// +// # Experimental +// +// Notice: This API is EXPERIMENTAL and may be changed or removed in a later +// release. +func (*MacKMS) DeleteCertificate(req *apiv1.DeleteCertificateRequest) error { + if req.Name == "" { + return fmt.Errorf("deleteCertificateRequest 'name' cannot be empty") + } + + u, err := parseCertURI(req.Name, true) + if err != nil { + return fmt.Errorf("mackms DeleteCertificate failed: %w", err) + } + + query := cf.Dictionary{ + security.KSecClass: security.KSecClassCertificate, + } + if u.label != "" { + cfLabel, err := cf.NewString(u.label) + if err != nil { + return fmt.Errorf("mackms DeleteCertificate failed: %w", err) + } + defer cfLabel.Release() + query[security.KSecAttrLabel] = cfLabel + } + if u.serialNumber != nil { + cfSerial, err := cf.NewData(encodeSerialNumber(u.serialNumber)) + if err != nil { + return fmt.Errorf("mackms DeleteCertificate failed: %w", err) } + defer cfSerial.Release() + query[security.KSecAttrSerialNumber] = cfSerial + } + + if err := deleteItem(query, nil); err != nil { + return fmt.Errorf("mackms DeleteCertificate failed: %w", err) } return nil @@ -533,7 +566,7 @@ func deleteItem(dict cf.Dictionary, hash []byte) error { query, err := cf.NewDictionary(dict) if err != nil { - return fmt.Errorf("mackms DeleteKey failed: %w", err) + return err } defer query.Release() @@ -541,7 +574,7 @@ func deleteItem(dict cf.Dictionary, hash []byte) error { if dict[security.KSecAttrKeyClass] == security.KSecAttrKeyClassPublic && errors.Is(err, security.ErrNotFound) { return nil } - return fmt.Errorf("mackms DeleteKey failed: %w", err) + return err } return nil @@ -657,6 +690,145 @@ func extractPublicKey(secKeyRef *security.SecKeyRef) (crypto.PublicKey, []byte, return priv.Public(), hash, nil } +// isSelfSinged checks if a certificate is self signed. The algorithm looks like this: +// +// If subject != issuer: false +// ElseIf subjectKeyID != authorityKey: false +// ElseIf checkSignature: true +// Otherwise: false +func isSelfSigned(cert *x509.Certificate) bool { + // If subject != issuer: false + // ElseIf subjectKeyID != authorityKey: false + // ElseIf checkSignature: true + // Otherwise: false + if bytes.Equal(cert.RawSubject, cert.RawIssuer) { + if cert.SubjectKeyId != nil && cert.AuthorityKeyId != nil && !bytes.Equal(cert.SubjectKeyId, cert.AuthorityKeyId) { + return false + } + + return cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature) == nil + } + + return false + +} + +func loadCertificate(label string, serialNumber *big.Int, subjectKeyID []byte) (*x509.Certificate, error) { + query := cf.Dictionary{ + security.KSecClass: security.KSecClassCertificate, + } + if label != "" { + cfLabel, err := cf.NewString(label) + if err != nil { + return nil, err + } + defer cfLabel.Release() + query[security.KSecAttrLabel] = cfLabel + } + if serialNumber != nil { + cfSerial, err := cf.NewData(encodeSerialNumber(serialNumber)) + if err != nil { + return nil, err + } + defer cfSerial.Release() + query[security.KSecAttrSerialNumber] = cfSerial + } + if subjectKeyID != nil { + cfSubjectKeyID, err := cf.NewData(subjectKeyID) + if err != nil { + return nil, err + } + defer cfSubjectKeyID.Release() + query[security.KSecAttrSubjectKeyID] = cfSubjectKeyID + } + + cfQuery, err := cf.NewDictionary(query) + if err != nil { + return nil, err + } + defer cfQuery.Release() + + var ref cf.TypeRef + if err := security.SecItemCopyMatching(cfQuery, &ref); err != nil { + return nil, err + } + defer ref.Release() + + data, err := security.SecCertificateCopyData(security.NewSecCertificateRef(ref)) + if err != nil { + return nil, err + } + defer data.Release() + + cert, err := x509.ParseCertificate(data.Bytes()) + if err != nil { + return nil, err + } + + return cert, nil +} + +func storeCertificate(label string, cert *x509.Certificate) error { + cfData, err := cf.NewData(cert.Raw) + if err != nil { + return err + } + defer cfData.Release() + + certRef, err := security.SecCertificateCreateWithData(cfData) + if err != nil { + return err + } + defer certRef.Release() + + // Adding the label here doesn't have any effect. Apple Keychain always uses + // the commonName. + attributes, err := cf.NewDictionary(cf.Dictionary{ + security.KSecClass: security.KSecClassCertificate, + security.KSecValueRef: certRef, + }) + if err != nil { + return err + } + defer attributes.Release() + + // Store the certificate + if err := security.SecItemAdd(attributes, nil); err != nil { + return err + } + + // Update the label if necessary + if label != "" && label != cert.Subject.CommonName { + cfLabel, err := cf.NewString(label) + if err != nil { + return err + } + defer cfLabel.Release() + + query, err := cf.NewDictionary(cf.Dictionary{ + security.KSecValueRef: certRef, + }) + if err != nil { + return err + } + defer query.Release() + + update, err := cf.NewDictionary(cf.Dictionary{ + security.KSecAttrLabel: cfLabel, + }) + if err != nil { + return err + } + defer update.Release() + + if err := security.SecItemUpdate(query, update); err != nil { + return err + } + } + + return nil +} + func parseURI(rawuri string) (*keyAttributes, error) { // When rawuri is just the key name if !strings.HasPrefix(strings.ToLower(rawuri), Scheme) { diff --git a/kms/mackms/mackms_test.go b/kms/mackms/mackms_test.go index 25325d18..341f6128 100644 --- a/kms/mackms/mackms_test.go +++ b/kms/mackms/mackms_test.go @@ -692,28 +692,46 @@ func Test_ecdhToECDSAPublicKey(t *testing.T) { } } +func Test_encodeSerialNumber(t *testing.T) { + getBigInt := func(s string) *big.Int { + b, err := hex.DecodeString(s) + require.NoError(t, err) + return new(big.Int).SetBytes(b) + } + getBytes := func(s string) []byte { + b, err := hex.DecodeString(s) + require.NoError(t, err) + return b + } + + type args struct { + s *big.Int + } + tests := []struct { + name string + args args + want []byte + }{ + {"ok zero", args{big.NewInt(0)}, []byte{0}}, + {"ok no pad", args{getBigInt("7df0e2ea242fd1a0650cf652aa31bfa0")}, getBytes("7df0e2ea242fd1a0650cf652aa31bfa0")}, + {"ok with pad", args{getBigInt("c4b3e6e28985f1a012aa38e7493b6f35")}, getBytes("00c4b3e6e28985f1a012aa38e7493b6f35")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, encodeSerialNumber(tt.args.s)) + }) + } +} + func deleteCertificate(t *testing.T, label string, cert *x509.Certificate) { if label == "" { label = cert.Subject.CommonName } - cfLabel, err := cf.NewString(label) - require.NoError(t, err) - defer cfLabel.Release() - - serialNumber, err := cf.NewData(encodeSerialNumber(cert.SerialNumber)) - require.NoError(t, err) - defer serialNumber.Release() - - query, err := cf.NewDictionary(cf.Dictionary{ - security.KSecClass: security.KSecClassCertificate, - security.KSecAttrLabel: cfLabel, - security.KSecAttrSerialNumber: serialNumber, - }) - require.NoError(t, err) - defer query.Release() - - require.NoError(t, security.SecItemDelete(query), "SecItemDelete failed: label=%s, serial=0x%X", label, encodeSerialNumber(cert.SerialNumber)) + kms := &MacKMS{} + require.NoError(t, kms.DeleteCertificate(&apiv1.DeleteCertificateRequest{ + Name: "mackms:label=" + label + ";serial=" + hex.EncodeToString(cert.SerialNumber.Bytes()), + })) } func TestMacKMS_LoadCertificate(t *testing.T) { @@ -932,38 +950,249 @@ func TestMacKMS_StoreCertificate_duplicated(t *testing.T) { } } -func TestMariano(t *testing.T) { - _, err := parseCertURI("mackm", false) - assert.NoError(t, err) +func TestMacKMS_LoadCertificateChain(t *testing.T) { + testName := t.Name() + ca, err := minica.New(minica.WithName(testName)) + require.NoError(t, err) + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + cert, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{CommonName: testName + "@example.com"}, + EmailAddresses: []string{testName + "@example.com"}, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + kms := &MacKMS{} + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:", Certificate: ca.Root, + })) + t.Cleanup(func() { deleteCertificate(t, "", ca.Root) }) + + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:", Certificate: ca.Intermediate, + })) + t.Cleanup(func() { deleteCertificate(t, "", ca.Intermediate) }) + + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:", Certificate: cert, + })) + t.Cleanup(func() { deleteCertificate(t, "", cert) }) + + type args struct { + req *apiv1.LoadCertificateChainRequest + } + tests := []struct { + name string + k *MacKMS + args args + want []*x509.Certificate + assertion assert.ErrorAssertionFunc + }{ + {"ok label", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ + Name: "mackms:label=" + cert.Subject.CommonName, + }}, []*x509.Certificate{cert, ca.Intermediate}, assert.NoError}, + {"ok serial", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ + Name: "mackms:serial=" + hex.EncodeToString(cert.SerialNumber.Bytes()), + }}, []*x509.Certificate{cert, ca.Intermediate}, assert.NoError}, + {"ok label and serial", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ + Name: "mackms:labeld=" + cert.Subject.CommonName + ";serial=" + hex.EncodeToString(cert.SerialNumber.Bytes()), + }}, []*x509.Certificate{cert, ca.Intermediate}, assert.NoError}, + {"ok self-signed", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{ + Name: "mackms:label=" + ca.Root.Subject.CommonName, + }}, []*x509.Certificate{ca.Root}, assert.NoError}, + {"fail name", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{}}, nil, assert.Error}, + {"fail uri", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{Name: "mackms:"}}, nil, assert.Error}, + {"fail missing", &MacKMS{}, args{&apiv1.LoadCertificateChainRequest{Name: "mackms:label=missing-" + testName}}, nil, assert.Error}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + k := &MacKMS{} + got, err := k.LoadCertificateChain(tt.args.req) + tt.assertion(t, err) + assert.Equal(t, tt.want, got) + }) + } } -func Test_encodeSerialNumber(t *testing.T) { - getBigInt := func(s string) *big.Int { - b, err := hex.DecodeString(s) - require.NoError(t, err) - return new(big.Int).SetBytes(b) +func TestMacKMS_StoreCertificateChain(t *testing.T) { + testName := t.Name() + ca, err := minica.New(minica.WithName(testName)) + require.NoError(t, err) + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + cert, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{CommonName: testName + "@example.com"}, + EmailAddresses: []string{testName + "@example.com"}, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + verifyCertificates := func(name, label string, chain []*x509.Certificate) func(t *testing.T) { + return func(t *testing.T) { + t.Helper() + + kms := &MacKMS{} + got, err := kms.LoadCertificateChain(&apiv1.LoadCertificateChainRequest{ + Name: name, + }) + if assert.NoError(t, err) && assert.Equal(t, chain, got) { + deleteCertificate(t, label, chain[0]) + for _, crt := range chain[1:] { + deleteCertificate(t, "", crt) + } + } + } } - getBytes := func(s string) []byte { - b, err := hex.DecodeString(s) - require.NoError(t, err) - return b + + suffix, err := randutil.Alphanumeric(8) + require.NoError(t, err) + label := "test-" + suffix + + type args struct { + req *apiv1.StoreCertificateChainRequest } + tests := []struct { + name string + k *MacKMS + args args + verify func(*testing.T) + assertion assert.ErrorAssertionFunc + }{ + {"ok", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "mackms:", + CertificateChain: []*x509.Certificate{cert, ca.Intermediate, ca.Root}, + }}, func(t *testing.T) { + t.Cleanup(func() { + deleteCertificate(t, "", ca.Root) + }) + fn := verifyCertificates("mackms:label="+cert.Subject.CommonName, "", []*x509.Certificate{cert, ca.Intermediate}) + fn(t) + }, assert.NoError}, + {"ok leaf", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "", + CertificateChain: []*x509.Certificate{cert}, + }}, verifyCertificates("mackms:label="+cert.Subject.CommonName, "", []*x509.Certificate{cert}), assert.NoError}, + {"ok with label", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "mackms:label=" + label, + CertificateChain: []*x509.Certificate{cert, ca.Intermediate}, + }}, verifyCertificates("mackms:label="+label, label, []*x509.Certificate{cert, ca.Intermediate}), assert.NoError}, + {"ok already exists", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "mackms:", + CertificateChain: []*x509.Certificate{cert, ca.Intermediate, ca.Intermediate}, + }}, verifyCertificates("mackms:label="+cert.Subject.CommonName, "", []*x509.Certificate{cert, ca.Intermediate}), assert.NoError}, + {"fail certificates", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "mackms:", + }}, func(t *testing.T) {}, assert.Error}, + {"fail uri", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "mackms", + CertificateChain: []*x509.Certificate{cert, ca.Intermediate}, + }}, func(t *testing.T) {}, assert.Error}, + {"fail store certificate", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "mackms:", + CertificateChain: []*x509.Certificate{{}, ca.Intermediate}, + }}, func(t *testing.T) {}, assert.Error}, + {"fail store certificate chain", &MacKMS{}, args{&apiv1.StoreCertificateChainRequest{ + Name: "mackms:", + CertificateChain: []*x509.Certificate{cert, {}}, + }}, verifyCertificates("mackms:label="+cert.Subject.CommonName, "", []*x509.Certificate{cert}), assert.Error}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.name == "fail duplicated" { + t.Log("foo") + } + k := &MacKMS{} + tt.assertion(t, k.StoreCertificateChain(tt.args.req)) + tt.verify(t) + }) + } +} + +func TestMacKMS_DeleteCertificate(t *testing.T) { + testName := t.Name() + ca, err := minica.New(minica.WithName(testName)) + require.NoError(t, err) + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + cert1, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{CommonName: testName + "1@example.com"}, + EmailAddresses: []string{testName + "1@example.com"}, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + cert2, err := ca.Sign(&x509.Certificate{ + Subject: pkix.Name{CommonName: testName + "2@example.com"}, + EmailAddresses: []string{testName + "2@example.com"}, + PublicKey: key.Public(), + }) + require.NoError(t, err) + + suffix, err := randutil.Alphanumeric(8) + require.NoError(t, err) + + notExistsCheck := func(cert *x509.Certificate) { + kms := &MacKMS{} + _, err := kms.LoadCertificate(&apiv1.LoadCertificateRequest{ + Name: "mackms:serial=" + hex.EncodeToString(cert.SerialNumber.Bytes()), + }) + assert.ErrorIs(t, err, security.ErrNotFound) + } + + kms := &MacKMS{} + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:", Certificate: ca.Root, + })) + t.Cleanup(func() { notExistsCheck(ca.Root) }) + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:label=test-intermediate-" + suffix, Certificate: ca.Intermediate, + })) + t.Cleanup(func() { notExistsCheck(ca.Intermediate) }) + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:", Certificate: cert1, + })) + t.Cleanup(func() { notExistsCheck(cert1) }) + require.NoError(t, kms.StoreCertificate(&apiv1.StoreCertificateRequest{ + Name: "mackms:label=test-leaf-" + suffix, Certificate: cert2, + })) + t.Cleanup(func() { notExistsCheck(cert2) }) type args struct { - s *big.Int + req *apiv1.DeleteCertificateRequest } tests := []struct { - name string - args args - want []byte + name string + m *MacKMS + args args + assertion assert.ErrorAssertionFunc }{ - {"ok zero", args{big.NewInt(0)}, []byte{0}}, - {"ok no pad", args{getBigInt("7df0e2ea242fd1a0650cf652aa31bfa0")}, getBytes("7df0e2ea242fd1a0650cf652aa31bfa0")}, - {"ok with pad", args{getBigInt("c4b3e6e28985f1a012aa38e7493b6f35")}, getBytes("00c4b3e6e28985f1a012aa38e7493b6f35")}, + {"ok", &MacKMS{}, args{&apiv1.DeleteCertificateRequest{ + Name: "mackms:" + ca.Root.Subject.CommonName, + }}, assert.NoError}, + {"ok label", &MacKMS{}, args{&apiv1.DeleteCertificateRequest{ + Name: "mackms:label=test-intermediate-" + suffix, + }}, assert.NoError}, + {"ok serial", &MacKMS{}, args{&apiv1.DeleteCertificateRequest{ + Name: "mackms:serial=" + hex.EncodeToString(cert1.SerialNumber.Bytes()), + }}, assert.NoError}, + {"ok label and serial", &MacKMS{}, args{&apiv1.DeleteCertificateRequest{ + Name: "mackms:label=test-leaf-" + suffix + ";serial=" + hex.EncodeToString(cert2.SerialNumber.Bytes()), + }}, assert.NoError}, + {"fail name", &MacKMS{}, args{&apiv1.DeleteCertificateRequest{}}, assert.Error}, + {"fail uri", &MacKMS{}, args{&apiv1.DeleteCertificateRequest{Name: "mackms"}}, assert.Error}, + {"fail missing", &MacKMS{}, args{&apiv1.DeleteCertificateRequest{Name: "mackms:label=" + testName}}, assert.Error}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, encodeSerialNumber(tt.args.s)) + m := &MacKMS{} + tt.assertion(t, m.DeleteCertificate(tt.args.req)) }) } } From 6e99ba30f2fb9ab0dc7ecf255800dc3fd5df7cc7 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 18 Mar 2024 11:20:13 -0700 Subject: [PATCH 3/8] Apply suggestions from code review Co-authored-by: Herman Slatman --- kms/mackms/mackms.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index 3133cf30..b5e07a0f 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -412,7 +412,7 @@ func (k *MacKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([ // Look for the rest of intermediates skipping the root. for { - // The Keychain stores the subject as an attribute, but it safes some of + // The Keychain stores the subject as an attribute, but it saves some of // the values to uppercase. We cannot use the cert.RawIssuer to restrict // more the search with KSecAttrSubjectKeyID and kSecAttrSubject. We // would need to "normalize" it in the same way. @@ -434,7 +434,7 @@ func (k *MacKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([ // // Valid names (URIs) are: // - "" will use the common name as the label -// - "mackms:" will use the common +// - "mackms:" will use the common name // - "mackms:label=my-label" will use "my-label" // - "mackms:my-label" will use the "my-label" func (k *MacKMS) StoreCertificateChain(req *apiv1.StoreCertificateChainRequest) error { @@ -510,7 +510,7 @@ func (*MacKMS) DeleteKey(req *apiv1.DeleteKeyRequest) error { return nil } -// DeleteCertificateRequest deletes the certificate referenced by the URI in the +// DeleteCertificate deletes the certificate referenced by the URI in the // request name. // // # Experimental @@ -690,17 +690,13 @@ func extractPublicKey(secKeyRef *security.SecKeyRef) (crypto.PublicKey, []byte, return priv.Public(), hash, nil } -// isSelfSinged checks if a certificate is self signed. The algorithm looks like this: +// isSelfSigned checks if a certificate is self signed. The algorithm looks like this: // // If subject != issuer: false // ElseIf subjectKeyID != authorityKey: false // ElseIf checkSignature: true // Otherwise: false func isSelfSigned(cert *x509.Certificate) bool { - // If subject != issuer: false - // ElseIf subjectKeyID != authorityKey: false - // ElseIf checkSignature: true - // Otherwise: false if bytes.Equal(cert.RawSubject, cert.RawIssuer) { if cert.SubjectKeyId != nil && cert.AuthorityKeyId != nil && !bytes.Equal(cert.SubjectKeyId, cert.AuthorityKeyId) { return false From 23cdc53d25bcecfe1e5760dcc89d34834083651e Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 18 Mar 2024 11:40:19 -0700 Subject: [PATCH 4/8] Remove debug log method --- .../darwin/corefoundation/core_foundation_darwin.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/internal/darwin/corefoundation/core_foundation_darwin.go b/internal/darwin/corefoundation/core_foundation_darwin.go index 89f5055e..1008ddfb 100644 --- a/internal/darwin/corefoundation/core_foundation_darwin.go +++ b/internal/darwin/corefoundation/core_foundation_darwin.go @@ -20,15 +20,8 @@ package corefoundation /* -#cgo CFLAGS: -x objective-c -#cgo LDFLAGS: -framework CoreFoundation -framework Foundation - -#include +#cgo LDFLAGS: -framework CoreFoundation #include - -void nslog(CFTypeRef ref) { - NSLog(@"%@", ref); -} */ import "C" import ( @@ -193,7 +186,3 @@ func (e ErrorRef) Error() string { func (e ErrorRef) Release() { Release(e) } func (e ErrorRef) TypeRef() CFTypeRef { return C.CFTypeRef(C.CFErrorRef(e)) } - -func Log(v TypeReferer) { - C.nslog(v.TypeRef()) -} From 82ec53ab4a44f68d46f2fd35db72458ca498c35d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 18 Mar 2024 11:41:43 -0700 Subject: [PATCH 5/8] Fix export of security attributes --- internal/darwin/security/security_darwin.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/darwin/security/security_darwin.go b/internal/darwin/security/security_darwin.go index bdb46642..e1a37fe8 100644 --- a/internal/darwin/security/security_darwin.go +++ b/internal/darwin/security/security_darwin.go @@ -72,8 +72,8 @@ var ( KSecAttrSubjectKeyID = cf.TypeRef(C.kSecAttrSubjectKeyID) KSecAttrSubject = cf.TypeRef(C.kSecAttrSubject) KSecAttrIssuer = cf.TypeRef(C.kSecAttrIssuer) - kSecAttrSynchronizable = cf.TypeRef(C.kSecAttrSynchronizable) - kSecUseDataProtectionKeychain = cf.TypeRef(C.kSecUseDataProtectionKeychain) + KSecAttrSynchronizable = cf.TypeRef(C.kSecAttrSynchronizable) + KSecUseDataProtectionKeychain = cf.TypeRef(C.kSecUseDataProtectionKeychain) KSecClass = cf.TypeRef(C.kSecClass) KSecClassKey = cf.TypeRef(C.kSecClassKey) KSecClassCertificate = cf.TypeRef(C.kSecClassCertificate) From 5df42e1bbf50bff35c6ebd355e79f27ec93f8e50 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 18 Mar 2024 11:46:25 -0700 Subject: [PATCH 6/8] Fix unclear comments --- kms/mackms/mackms.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index b5e07a0f..9f240b22 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -916,7 +916,7 @@ func parseCertURI(rawuri string, requireValue bool) (*certAttributes, error) { } // encodeSerialNumber encodes the serial number of a certificate in ASN.1. -// Negative serial numbers are now allowed. +// Negative serial numbers are not allowed. func encodeSerialNumber(s *big.Int) []byte { if s.Sign() == 0 { return []byte{0x00} From f6e8fe8d60be0cc1d13307a232216de1c38e84a7 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 18 Mar 2024 11:54:11 -0700 Subject: [PATCH 7/8] Clarify comment --- kms/mackms/mackms.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index 9f240b22..187eac9f 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -413,9 +413,9 @@ func (k *MacKMS) LoadCertificateChain(req *apiv1.LoadCertificateChainRequest) ([ // Look for the rest of intermediates skipping the root. for { // The Keychain stores the subject as an attribute, but it saves some of - // the values to uppercase. We cannot use the cert.RawIssuer to restrict - // more the search with KSecAttrSubjectKeyID and kSecAttrSubject. We - // would need to "normalize" it in the same way. + // the values in uppercase. We cannot use the cert.RawIssuer to restrict + // more the search with KSecAttrSubjectKeyID and kSecAttrSubject. To do + // it we will need to "normalize" the subject it in the same way. parent, err := loadCertificate("", nil, cert.AuthorityKeyId) if err != nil || isSelfSigned(parent) || cert.CheckSignatureFrom(parent) != nil { break From 69660dc06f18c678d6e4efa14cd5c8186fbfb3cf Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 19 Mar 2024 10:37:04 -0700 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: Herman Slatman --- kms/mackms/mackms.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/kms/mackms/mackms.go b/kms/mackms/mackms.go index 187eac9f..f45924ca 100644 --- a/kms/mackms/mackms.go +++ b/kms/mackms/mackms.go @@ -356,7 +356,7 @@ func (k *MacKMS) LoadCertificate(req *apiv1.LoadCertificateRequest) (*x509.Certi // // Valid names (URIs) are: // - "" will use the common name as the label -// - "mackms:" will use the common +// - "mackms:" will use the common name // - "mackms:label=my-label" will use "my-label" // - "mackms:my-label" will use the "my-label" func (k *MacKMS) StoreCertificate(req *apiv1.StoreCertificateRequest) error { @@ -706,7 +706,6 @@ func isSelfSigned(cert *x509.Certificate) bool { } return false - } func loadCertificate(label string, serialNumber *big.Int, subjectKeyID []byte) (*x509.Certificate, error) {