From cbe44eefe900aa52e58960dfcc8b1010cdba42c6 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Tue, 7 Jan 2025 16:14:12 -0600 Subject: [PATCH] Refactoring crypto code for future reuse. (#25148) Refactoring crypto code for future reuse for #24869. No functional changes. --- pkg/mdm/mdmtest/apple.go | 2 +- server/mdm/cryptoutil/cryptoutil.go | 68 ++++++++++++++ server/mdm/cryptoutil/cryptoutil_test.go | 94 +++++++++++++++++++ server/mdm/{scep => }/cryptoutil/doc.go | 0 .../cryptoutil}/testdata/pkcs1.key | 0 .../cryptoutil}/testdata/pkcs8-encrypted.key | 0 .../cryptoutil}/testdata/pkcs8-rsa.key | 0 .../cryptoutil}/testdata/pkcs8-x25519.key | 0 server/mdm/scep/cmd/scepclient/csr.go | 2 +- server/mdm/scep/cryptoutil/cryptoutil.go | 36 ------- server/mdm/scep/cryptoutil/cryptoutil_test.go | 58 ------------ server/mdm/scep/depot/cacert.go | 2 +- server/mdm/scep/depot/signer.go | 2 +- .../mdm/scep/{cryptoutil => }/x509util/doc.go | 0 .../{cryptoutil => }/x509util/x509util.go | 0 .../x509util/x509util_test.go | 0 server/service/mdm.go | 35 +------ server/service/mdm_test.go | 44 +-------- 18 files changed, 170 insertions(+), 173 deletions(-) create mode 100644 server/mdm/cryptoutil/cryptoutil.go create mode 100644 server/mdm/cryptoutil/cryptoutil_test.go rename server/mdm/{scep => }/cryptoutil/doc.go (100%) rename server/{service => mdm/cryptoutil}/testdata/pkcs1.key (100%) rename server/{service => mdm/cryptoutil}/testdata/pkcs8-encrypted.key (100%) rename server/{service => mdm/cryptoutil}/testdata/pkcs8-rsa.key (100%) rename server/{service => mdm/cryptoutil}/testdata/pkcs8-x25519.key (100%) delete mode 100644 server/mdm/scep/cryptoutil/cryptoutil.go delete mode 100644 server/mdm/scep/cryptoutil/cryptoutil_test.go rename server/mdm/scep/{cryptoutil => }/x509util/doc.go (100%) rename server/mdm/scep/{cryptoutil => }/x509util/x509util.go (100%) rename server/mdm/scep/{cryptoutil => }/x509util/x509util_test.go (100%) diff --git a/pkg/mdm/mdmtest/apple.go b/pkg/mdm/mdmtest/apple.go index e6cbbb0e1a06..e78aea43a1f6 100644 --- a/pkg/mdm/mdmtest/apple.go +++ b/pkg/mdm/mdmtest/apple.go @@ -25,8 +25,8 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" apple_mdm "github.com/fleetdm/fleet/v4/server/mdm/apple" "github.com/fleetdm/fleet/v4/server/mdm/nanomdm/mdm" - "github.com/fleetdm/fleet/v4/server/mdm/scep/cryptoutil/x509util" scepserver "github.com/fleetdm/fleet/v4/server/mdm/scep/server" + "github.com/fleetdm/fleet/v4/server/mdm/scep/x509util" httptransport "github.com/go-kit/kit/transport/http" "github.com/go-kit/log" kitlog "github.com/go-kit/log" diff --git a/server/mdm/cryptoutil/cryptoutil.go b/server/mdm/cryptoutil/cryptoutil.go new file mode 100644 index 000000000000..1b0395145db8 --- /dev/null +++ b/server/mdm/cryptoutil/cryptoutil.go @@ -0,0 +1,68 @@ +package cryptoutil + +import ( + "crypto" + "crypto/ecdsa" + "crypto/ed25519" + "crypto/elliptic" + "crypto/rsa" + "crypto/sha256" + "crypto/x509" + "encoding/asn1" + "encoding/pem" + "errors" + "fmt" +) + +// GenerateSubjectKeyID generates Subject Key Identifier (SKI) using SHA-256 +// hash of the public key bytes according to RFC 7093 section 2. +func GenerateSubjectKeyID(pub crypto.PublicKey) ([]byte, error) { + var pubBytes []byte + var err error + switch pub := pub.(type) { + case *rsa.PublicKey: + pubBytes, err = asn1.Marshal(*pub) + if err != nil { + return nil, err + } + case *ecdsa.PublicKey: + pubBytes = elliptic.Marshal(pub.Curve, pub.X, pub.Y) + default: + return nil, errors.New("only ECDSA and RSA public keys are supported") + } + + hash := sha256.Sum256(pubBytes) + + // According to RFC 7093, The keyIdentifier is composed of the leftmost + // 160-bits of the SHA-256 hash of the value of the BIT STRING + // subjectPublicKey (excluding the tag, length, and number of unused bits). + return hash[:20], nil +} + +// ParsePrivateKey parses a PEM encoded private key and returns a crypto.PrivateKey. +// It can be used for private keys passed in from environment variables or command line or files. +func ParsePrivateKey(privKeyPEM []byte, keyName string) (crypto.PrivateKey, error) { + block, _ := pem.Decode(privKeyPEM) + if block == nil { + return nil, fmt.Errorf("failed to decode %s", keyName) + } + + // The code below is based on tls.parsePrivateKey + // https://cs.opensource.google/go/go/+/release-branch.go1.23:src/crypto/tls/tls.go;l=355-372 + if key, err := x509.ParsePKCS1PrivateKey(block.Bytes); err == nil { + return key, nil + } + if key, err := x509.ParsePKCS8PrivateKey(block.Bytes); err == nil { + switch key := key.(type) { + case *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey: + return key, nil + default: + return nil, fmt.Errorf("unmarshaled PKCS8 %s is not an RSA, ECDSA, or Ed25519 private key", keyName) + } + } + if key, err := x509.ParseECPrivateKey(block.Bytes); err == nil { + return key, nil + } + + return nil, fmt.Errorf("failed to parse %s of type %s", keyName, block.Type) +} diff --git a/server/mdm/cryptoutil/cryptoutil_test.go b/server/mdm/cryptoutil/cryptoutil_test.go new file mode 100644 index 000000000000..0224c7e7906f --- /dev/null +++ b/server/mdm/cryptoutil/cryptoutil_test.go @@ -0,0 +1,94 @@ +package cryptoutil + +import ( + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "math/big" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGenerateSubjectKeyID(t *testing.T) { + ecKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader) + if err != nil { + t.Fatal(err) + } + for _, test := range []struct { + testName string + pub crypto.PublicKey + }{ + {"RSA", &rsa.PublicKey{N: big.NewInt(123), E: 65537}}, + {"ECDSA", ecKey.Public()}, + } { + test := test + t.Run(test.testName, func(t *testing.T) { + t.Parallel() + ski, err := GenerateSubjectKeyID(test.pub) + if err != nil { + t.Fatal(err) + } + if len(ski) != 20 { + t.Fatalf("unexpected subject public key identifier length: %d", len(ski)) + } + ski2, err := GenerateSubjectKeyID(test.pub) + if err != nil { + t.Fatal(err) + } + if !testSKIEq(ski, ski2) { + t.Fatal("subject key identifier generation is not deterministic") + } + }) + } +} + +func testSKIEq(a, b []byte) bool { + if len(a) != len(b) { + return false + } + + for i := range a { + if a[i] != b[i] { + return false + } + } + + return true +} + +func TestParsePrivateKey(t *testing.T) { + t.Parallel() + // nil block not allowed + _, err := ParsePrivateKey(nil, "APNS private key") + assert.ErrorContains(t, err, "failed to decode") + + // encrypted pkcs8 not supported + pkcs8Encrypted, err := os.ReadFile("testdata/pkcs8-encrypted.key") + require.NoError(t, err) + _, err = ParsePrivateKey(pkcs8Encrypted, "APNS private key") + assert.ErrorContains(t, err, "failed to parse APNS private key of type ENCRYPTED PRIVATE KEY") + + // X25519 pkcs8 not supported + pkcs8Encrypted, err = os.ReadFile("testdata/pkcs8-x25519.key") + require.NoError(t, err) + _, err = ParsePrivateKey(pkcs8Encrypted, "APNS private key") + assert.ErrorContains(t, err, "unmarshaled PKCS8 APNS private key is not") + + // In this test, the pkcs1 key and pkcs8 keys are the same key, just different formats + pkcs1, err := os.ReadFile("testdata/pkcs1.key") + require.NoError(t, err) + pkcs1Key, err := ParsePrivateKey(pkcs1, "APNS private key") + require.NoError(t, err) + + pkcs8, err := os.ReadFile("testdata/pkcs8-rsa.key") + require.NoError(t, err) + pkcs8Key, err := ParsePrivateKey(pkcs8, "APNS private key") + require.NoError(t, err) + + assert.Equal(t, pkcs1Key, pkcs8Key) +} diff --git a/server/mdm/scep/cryptoutil/doc.go b/server/mdm/cryptoutil/doc.go similarity index 100% rename from server/mdm/scep/cryptoutil/doc.go rename to server/mdm/cryptoutil/doc.go diff --git a/server/service/testdata/pkcs1.key b/server/mdm/cryptoutil/testdata/pkcs1.key similarity index 100% rename from server/service/testdata/pkcs1.key rename to server/mdm/cryptoutil/testdata/pkcs1.key diff --git a/server/service/testdata/pkcs8-encrypted.key b/server/mdm/cryptoutil/testdata/pkcs8-encrypted.key similarity index 100% rename from server/service/testdata/pkcs8-encrypted.key rename to server/mdm/cryptoutil/testdata/pkcs8-encrypted.key diff --git a/server/service/testdata/pkcs8-rsa.key b/server/mdm/cryptoutil/testdata/pkcs8-rsa.key similarity index 100% rename from server/service/testdata/pkcs8-rsa.key rename to server/mdm/cryptoutil/testdata/pkcs8-rsa.key diff --git a/server/service/testdata/pkcs8-x25519.key b/server/mdm/cryptoutil/testdata/pkcs8-x25519.key similarity index 100% rename from server/service/testdata/pkcs8-x25519.key rename to server/mdm/cryptoutil/testdata/pkcs8-x25519.key diff --git a/server/mdm/scep/cmd/scepclient/csr.go b/server/mdm/scep/cmd/scepclient/csr.go index 0eadcac4e355..195c7c6f360a 100644 --- a/server/mdm/scep/cmd/scepclient/csr.go +++ b/server/mdm/scep/cmd/scepclient/csr.go @@ -10,7 +10,7 @@ import ( "io/ioutil" "os" - "github.com/fleetdm/fleet/v4/server/mdm/scep/cryptoutil/x509util" + "github.com/fleetdm/fleet/v4/server/mdm/scep/x509util" ) const ( diff --git a/server/mdm/scep/cryptoutil/cryptoutil.go b/server/mdm/scep/cryptoutil/cryptoutil.go deleted file mode 100644 index 6512c6154cc5..000000000000 --- a/server/mdm/scep/cryptoutil/cryptoutil.go +++ /dev/null @@ -1,36 +0,0 @@ -package cryptoutil - -import ( - "crypto" - "crypto/ecdsa" - "crypto/elliptic" - "crypto/rsa" - "crypto/sha256" - "encoding/asn1" - "errors" -) - -// GenerateSubjectKeyID generates Subject Key Identifier (SKI) using SHA-256 -// hash of the public key bytes according to RFC 7093 section 2. -func GenerateSubjectKeyID(pub crypto.PublicKey) ([]byte, error) { - var pubBytes []byte - var err error - switch pub := pub.(type) { - case *rsa.PublicKey: - pubBytes, err = asn1.Marshal(*pub) - if err != nil { - return nil, err - } - case *ecdsa.PublicKey: - pubBytes = elliptic.Marshal(pub.Curve, pub.X, pub.Y) - default: - return nil, errors.New("only ECDSA and RSA public keys are supported") - } - - hash := sha256.Sum256(pubBytes) - - // According to RFC 7093, The keyIdentifier is composed of the leftmost - // 160-bits of the SHA-256 hash of the value of the BIT STRING - // subjectPublicKey (excluding the tag, length, and number of unused bits). - return hash[:20], nil -} diff --git a/server/mdm/scep/cryptoutil/cryptoutil_test.go b/server/mdm/scep/cryptoutil/cryptoutil_test.go deleted file mode 100644 index 53a73ee9b36f..000000000000 --- a/server/mdm/scep/cryptoutil/cryptoutil_test.go +++ /dev/null @@ -1,58 +0,0 @@ -package cryptoutil - -import ( - "crypto" - "crypto/ecdsa" - "crypto/elliptic" - "crypto/rand" - "crypto/rsa" - "math/big" - "testing" -) - -func TestGenerateSubjectKeyID(t *testing.T) { - ecKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader) - if err != nil { - t.Fatal(err) - } - for _, test := range []struct { - testName string - pub crypto.PublicKey - }{ - {"RSA", &rsa.PublicKey{N: big.NewInt(123), E: 65537}}, - {"ECDSA", ecKey.Public()}, - } { - test := test - t.Run(test.testName, func(t *testing.T) { - t.Parallel() - ski, err := GenerateSubjectKeyID(test.pub) - if err != nil { - t.Fatal(err) - } - if len(ski) != 20 { - t.Fatalf("unexpected subject public key identifier length: %d", len(ski)) - } - ski2, err := GenerateSubjectKeyID(test.pub) - if err != nil { - t.Fatal(err) - } - if !testSKIEq(ski, ski2) { - t.Fatal("subject key identifier generation is not deterministic") - } - }) - } -} - -func testSKIEq(a, b []byte) bool { - if len(a) != len(b) { - return false - } - - for i := range a { - if a[i] != b[i] { - return false - } - } - - return true -} diff --git a/server/mdm/scep/depot/cacert.go b/server/mdm/scep/depot/cacert.go index 7aba250c3115..cf430459be84 100644 --- a/server/mdm/scep/depot/cacert.go +++ b/server/mdm/scep/depot/cacert.go @@ -8,7 +8,7 @@ import ( "math/big" "time" - "github.com/fleetdm/fleet/v4/server/mdm/scep/cryptoutil" + "github.com/fleetdm/fleet/v4/server/mdm/cryptoutil" ) // CACert represents a new self-signed CA certificate diff --git a/server/mdm/scep/depot/signer.go b/server/mdm/scep/depot/signer.go index d2f33ae45174..73476b1f19dc 100644 --- a/server/mdm/scep/depot/signer.go +++ b/server/mdm/scep/depot/signer.go @@ -5,7 +5,7 @@ import ( "crypto/x509" "time" - "github.com/fleetdm/fleet/v4/server/mdm/scep/cryptoutil" + "github.com/fleetdm/fleet/v4/server/mdm/cryptoutil" "github.com/smallstep/scep" ) diff --git a/server/mdm/scep/cryptoutil/x509util/doc.go b/server/mdm/scep/x509util/doc.go similarity index 100% rename from server/mdm/scep/cryptoutil/x509util/doc.go rename to server/mdm/scep/x509util/doc.go diff --git a/server/mdm/scep/cryptoutil/x509util/x509util.go b/server/mdm/scep/x509util/x509util.go similarity index 100% rename from server/mdm/scep/cryptoutil/x509util/x509util.go rename to server/mdm/scep/x509util/x509util.go diff --git a/server/mdm/scep/cryptoutil/x509util/x509util_test.go b/server/mdm/scep/x509util/x509util_test.go similarity index 100% rename from server/mdm/scep/cryptoutil/x509util/x509util_test.go rename to server/mdm/scep/x509util/x509util_test.go diff --git a/server/service/mdm.go b/server/service/mdm.go index 406c79441777..d0de896f6839 100644 --- a/server/service/mdm.go +++ b/server/service/mdm.go @@ -4,11 +4,7 @@ import ( "bytes" "context" "crypto" - "crypto/ecdsa" - "crypto/ed25519" - "crypto/rsa" "crypto/tls" - "crypto/x509" "encoding/json" "encoding/pem" "errors" @@ -34,6 +30,7 @@ import ( "github.com/fleetdm/fleet/v4/server/mdm" apple_mdm "github.com/fleetdm/fleet/v4/server/mdm/apple" "github.com/fleetdm/fleet/v4/server/mdm/assets" + "github.com/fleetdm/fleet/v4/server/mdm/cryptoutil" nanomdm "github.com/fleetdm/fleet/v4/server/mdm/nanomdm/mdm" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/go-kit/log/level" @@ -2496,10 +2493,9 @@ func (svc *Service) GetMDMAppleCSR(ctx context.Context) ([]byte, error) { } } else { rawApnsKey := savedAssets[fleet.MDMAssetAPNSKey] - block, _ := pem.Decode(rawApnsKey.Value) - apnsKey, err = parseAPNSPrivateKey(ctx, block) + apnsKey, err = cryptoutil.ParsePrivateKey(rawApnsKey.Value, "APNS private key") if err != nil { - return nil, err + return nil, ctxerr.Wrap(ctx, err, "parse APNS private key") } } @@ -2546,31 +2542,6 @@ func (svc *Service) GetMDMAppleCSR(ctx context.Context) ([]byte, error) { return signedCSRB64, nil } -func parseAPNSPrivateKey(ctx context.Context, block *pem.Block) (crypto.PrivateKey, error) { - if block == nil { - return nil, ctxerr.New(ctx, "failed to decode saved APNS key") - } - - // The code below is based on tls.parsePrivateKey - // https://cs.opensource.google/go/go/+/release-branch.go1.23:src/crypto/tls/tls.go;l=355-372 - if key, err := x509.ParsePKCS1PrivateKey(block.Bytes); err == nil { - return key, nil - } - if key, err := x509.ParsePKCS8PrivateKey(block.Bytes); err == nil { - switch key := key.(type) { - case *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey: - return key, nil - default: - return nil, errors.New("unmarshaled PKCS8 APNS key is not an RSA, ECDSA, or Ed25519 private key") - } - } - if key, err := x509.ParseECPrivateKey(block.Bytes); err == nil { - return key, nil - } - - return nil, ctxerr.New(ctx, fmt.Sprintf("failed to parse APNS private key of type %s", block.Type)) -} - //////////////////////////////////////////////////////////////////////////////// // POST /mdm/apple/apns_certificate //////////////////////////////////////////////////////////////////////////////// diff --git a/server/service/mdm_test.go b/server/service/mdm_test.go index d7cf53fca5d3..74a59f5a0d5f 100644 --- a/server/service/mdm_test.go +++ b/server/service/mdm_test.go @@ -8,7 +8,6 @@ import ( "crypto/x509" "crypto/x509/pkix" "database/sql" - "encoding/pem" "errors" "math/big" "net/http" @@ -29,7 +28,7 @@ import ( "github.com/fleetdm/fleet/v4/server/contexts/license" "github.com/fleetdm/fleet/v4/server/contexts/viewer" "github.com/fleetdm/fleet/v4/server/fleet" - "github.com/fleetdm/fleet/v4/server/mdm/scep/cryptoutil/x509util" + "github.com/fleetdm/fleet/v4/server/mdm/scep/x509util" "github.com/fleetdm/fleet/v4/server/mock" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/fleetdm/fleet/v4/server/test" @@ -2185,44 +2184,3 @@ func TestBatchSetMDMProfilesLabels(t *testing.T) { assert.Equal(t, ProfileLabels{IncludeAny: true}, *profileLabels["DIncAny"]) assert.Equal(t, ProfileLabels{ExcludeAny: true}, *profileLabels["DExclAny"]) } - -func TestParseAPNSPrivateKey(t *testing.T) { - t.Parallel() - // nil block not allowed - ctx := context.Background() - _, err := parseAPNSPrivateKey(ctx, nil) - assert.ErrorContains(t, err, "failed to decode") - - // encrypted pkcs8 not supported - pkcs8Encrypted, err := os.ReadFile("testdata/pkcs8-encrypted.key") - require.NoError(t, err) - block, _ := pem.Decode(pkcs8Encrypted) - assert.NotNil(t, block) - _, err = parseAPNSPrivateKey(ctx, block) - assert.ErrorContains(t, err, "failed to parse APNS private key of type ENCRYPTED PRIVATE KEY") - - // X25519 pkcs8 not supported - pkcs8Encrypted, err = os.ReadFile("testdata/pkcs8-x25519.key") - require.NoError(t, err) - block, _ = pem.Decode(pkcs8Encrypted) - assert.NotNil(t, block) - _, err = parseAPNSPrivateKey(ctx, block) - assert.ErrorContains(t, err, "unmarshaled PKCS8 APNS key is not") - - // In this test, the pkcs1 key and pkcs8 keys are the same key, just different formats - pkcs1, err := os.ReadFile("testdata/pkcs1.key") - require.NoError(t, err) - block, _ = pem.Decode(pkcs1) - assert.NotNil(t, block) - pkcs1Key, err := parseAPNSPrivateKey(ctx, block) - require.NoError(t, err) - - pkcs8, err := os.ReadFile("testdata/pkcs8-rsa.key") - require.NoError(t, err) - block, _ = pem.Decode(pkcs8) - assert.NotNil(t, block) - pkcs8Key, err := parseAPNSPrivateKey(ctx, block) - require.NoError(t, err) - - assert.Equal(t, pkcs1Key, pkcs8Key) -}