diff --git a/cmd/kubectl-ate/internal/cmd/admin_make_ca_pool.go b/cmd/kubectl-ate/internal/cmd/admin_make_ca_pool.go index ab42f2690..082447180 100644 --- a/cmd/kubectl-ate/internal/cmd/admin_make_ca_pool.go +++ b/cmd/kubectl-ate/internal/cmd/admin_make_ca_pool.go @@ -28,6 +28,10 @@ import ( var caID string var targetSecretNamespace string var targetSecretName string +var caKeyType string +var caCommonName string +var caPermittedDNSDomains []string +var caMaxPathLen int var makeCaPoolCmd = &cobra.Command{ Use: "make-ca-pool", @@ -45,7 +49,19 @@ var makeCaPoolCmd = &cobra.Command{ return fmt.Errorf("while creating Kubernetes client: %w", err) } - ca, err := localca.GenerateED25519CA(caID) + opts := localca.GenerateOptions{ + ID: caID, + CommonName: caCommonName, + KeyType: localca.KeyType(caKeyType), + PermittedDNSDomains: caPermittedDNSDomains, + } + // -1 is the "say nothing about path length" sentinel, because 0 is a + // meaningful value here: it forbids intermediates entirely. + if caMaxPathLen >= 0 { + opts.MaxPathLen = &caMaxPathLen + } + + ca, err := localca.GenerateCA(opts) if err != nil { return fmt.Errorf("while generating CA: %w", err) } @@ -84,5 +100,13 @@ func init() { makeCaPoolCmd.Flags().StringVar(&caID, "ca-id", "", "The ID of the initial CA in the Pool") makeCaPoolCmd.Flags().StringVar(&targetSecretNamespace, "secret-namespace", "default", "Create the secret in this namespace") makeCaPoolCmd.Flags().StringVar(&targetSecretName, "name", "", "Create the secret with this name") + makeCaPoolCmd.Flags().StringVar(&caKeyType, "key-type", string(localca.KeyTypeED25519), + fmt.Sprintf("Signing key algorithm, %q or %q. Prefer %s for a CA whose certificates are validated by clients outside substrate, where Ed25519 support cannot be assumed.", + localca.KeyTypeED25519, localca.KeyTypeECDSAP256, localca.KeyTypeECDSAP256)) + makeCaPoolCmd.Flags().StringVar(&caCommonName, "common-name", "", "Subject common name of the CA certificate. Cosmetic; nothing authenticates on it.") + makeCaPoolCmd.Flags().StringArrayVar(&caPermittedDNSDomains, "permitted-dns-domain", nil, + "Constrain the CA to issuing for names beneath this DNS domain. Repeatable. Strongly recommended for any CA that signs for names substrate does not own.") + makeCaPoolCmd.Flags().IntVar(&caMaxPathLen, "max-path-len", -1, + "Maximum number of intermediate CAs beneath this one. 0 forbids intermediates; 1 permits a delegated signing intermediate. -1 leaves it unconstrained.") makeCaPoolCmd.MarkFlagRequired("name") } diff --git a/internal/localca/localca.go b/internal/localca/localca.go index 3078435ed..14c6e6b91 100644 --- a/internal/localca/localca.go +++ b/internal/localca/localca.go @@ -18,9 +18,12 @@ package localca import ( "crypto" + "crypto/ecdsa" "crypto/ed25519" + "crypto/elliptic" "crypto/rand" "crypto/x509" + "crypto/x509/pkix" "encoding/json" "encoding/pem" "fmt" @@ -32,8 +35,19 @@ type Pool struct { } type CA struct { - ID string - SigningKey crypto.PrivateKey + ID string + // SigningKey is a crypto.Signer rather than a concrete key type so that a + // signer whose private key lives outside this process -- a KMS, an HSM, a + // PKCS#11 token -- can be substituted for a parsed one. Everything that + // consumes a CA here passes SigningKey to x509.CreateCertificate, which + // only ever calls Public and Sign, so nothing downstream has to change. + // + // Such a signer cannot round-trip through Marshal: there is no key material + // to serialize. Marshal says so explicitly rather than emitting a pool that + // silently loses the key. Substrate ships no external signer itself, since + // picking one would mean picking a cloud; this type is the seam an operator + // implements against. + SigningKey crypto.Signer RootCertificate *x509.Certificate IntermediateCertificates []*x509.Certificate } @@ -58,9 +72,15 @@ func Marshal(ca *Pool) ([]byte, error) { caWire.ID = ca.ID + // An external signer has no exportable key material, so this is the + // point where "the key lives in a KMS" stops being marshalable. Name + // that case, because x509's own error ("unknown key type") reads like a + // bug in this code rather than a deliberate property of the signer. signingKeyPKCS8, err := x509.MarshalPKCS8PrivateKey(ca.SigningKey) if err != nil { - return nil, fmt.Errorf("while serializing signing key to PKCS#8: %w", err) + return nil, fmt.Errorf("while serializing signing key for CA %q to PKCS#8: %w "+ + "(a signer that holds no exportable key material, such as a KMS or HSM signer, "+ + "cannot be written to a pool file; keep it in its own store)", ca.ID, err) } caWire.SigningKeyPKCS8 = signingKeyPKCS8 @@ -119,9 +139,13 @@ func Unmarshal(wireBytes []byte) (*Pool, error) { return pool, nil } -func parsePrivateKey(pkcs8 []byte, pemData string) (crypto.PrivateKey, error) { +func parsePrivateKey(pkcs8 []byte, pemData string) (crypto.Signer, error) { if len(pkcs8) != 0 { - return x509.ParsePKCS8PrivateKey(pkcs8) + key, err := x509.ParsePKCS8PrivateKey(pkcs8) + if err != nil { + return nil, err + } + return asSigner(key) } block, _ := pem.Decode([]byte(pemData)) @@ -130,17 +154,28 @@ func parsePrivateKey(pkcs8 []byte, pemData string) (crypto.PrivateKey, error) { } if key, err := x509.ParsePKCS8PrivateKey(block.Bytes); err == nil { - return key, nil + return asSigner(key) } if key, err := x509.ParseECPrivateKey(block.Bytes); err == nil { - return key, nil + return asSigner(key) } if key, err := x509.ParsePKCS1PrivateKey(block.Bytes); err == nil { - return key, nil + return asSigner(key) } return nil, fmt.Errorf("unsupported private key PEM type %q", block.Type) } +// asSigner narrows a parsed key to the signing interface the CAs actually use. +// X25519 keys parse cleanly out of PKCS#8 and cannot sign, so this is a real +// case and not a defensive assertion. +func asSigner(key any) (crypto.Signer, error) { + signer, ok := key.(crypto.Signer) + if !ok { + return nil, fmt.Errorf("private key of type %T cannot sign", key) + } + return signer, nil +} + func parseCertificate(der []byte, pemData string) (*x509.Certificate, error) { if len(der) != 0 { return x509.ParseCertificate(der) @@ -156,14 +191,69 @@ func parseCertificate(der []byte, pemData string) (*x509.Certificate, error) { return x509.ParseCertificate(block.Bytes) } +// KeyType selects the algorithm of a generated CA's signing key. +type KeyType string + +const ( + // KeyTypeED25519 is the default. It is the smallest and fastest option and + // is what substrate's internal CAs have always used. + KeyTypeED25519 KeyType = "ed25519" + // KeyTypeECDSAP256 exists for CAs whose certificates are validated by + // clients outside substrate's control. Ed25519 in a chain needs OpenSSL + // 1.1.1+ or Go 1.13+, which is fine for anything substrate ships and not + // something to assume of an arbitrary process running inside an actor. + KeyTypeECDSAP256 KeyType = "ecdsa-p256" +) + +// GenerateOptions configures GenerateCA. The zero value, apart from ID, +// reproduces what GenerateED25519CA has always produced. +type GenerateOptions struct { + // ID names the CA within its Pool. + ID string + // CommonName is the subject CN. Empty leaves the subject empty, which is + // what the internal CAs do -- nothing authenticates on their name. + CommonName string + // KeyType defaults to KeyTypeED25519. + KeyType KeyType + // Lifetime defaults to 365 days. + Lifetime time.Duration + // PermittedDNSDomains, if non-empty, sets a critical dNSName name + // constraint. A constrained CA that leaks can still forge certificates, + // but only for names beneath these domains -- which for a CA that + // intercepts egress traffic is the difference between forging one vendor's + // API and forging anyone's bank. Constraining is worth more than any + // amount of care about where the key file sits. + PermittedDNSDomains []string + // MaxPathLen bounds how many intermediate CAs may appear beneath this one. + // nil leaves the constraint absent, meaning unlimited, which is the + // historical behavior. 0 means the CA may only issue end-entity + // certificates; 1 permits one intermediate. + MaxPathLen *int +} + +// GenerateED25519CA creates an unconstrained 365-day Ed25519 CA. It is the +// long-standing shape of substrate's internal CAs, kept as its own function +// because every existing caller wants exactly this. func GenerateED25519CA(id string) (*CA, error) { - rootPubKey, rootPrivKey, err := ed25519.GenerateKey(rand.Reader) + return GenerateCA(GenerateOptions{ID: id}) +} + +// GenerateCA creates a self-signed CA with its own freshly generated key. +func GenerateCA(opts GenerateOptions) (*CA, error) { + if opts.Lifetime == 0 { + opts.Lifetime = 365 * 24 * time.Hour + } + if opts.KeyType == "" { + opts.KeyType = KeyTypeED25519 + } + + rootPrivKey, err := generateKey(opts.KeyType) if err != nil { - return nil, fmt.Errorf("while generating root key: %w", err) + return nil, err } notBefore := time.Now() - notAfter := notBefore.Add(365 * 24 * time.Hour) + notAfter := notBefore.Add(opts.Lifetime) rootTemplate := &x509.Certificate{ NotBefore: notBefore, @@ -172,8 +262,23 @@ func GenerateED25519CA(id string) (*CA, error) { BasicConstraintsValid: true, KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, } + if opts.CommonName != "" { + rootTemplate.Subject = pkix.Name{CommonName: opts.CommonName} + } + if len(opts.PermittedDNSDomains) > 0 { + rootTemplate.PermittedDNSDomains = opts.PermittedDNSDomains + // Critical, so a client that does not understand name constraints + // rejects the chain instead of ignoring the limit it was given. + rootTemplate.PermittedDNSDomainsCritical = true + } + if opts.MaxPathLen != nil { + rootTemplate.MaxPathLen = *opts.MaxPathLen + // x509 encodes "path length zero" and "no path length given" the same + // way in the struct; MaxPathLenZero is what disambiguates them. + rootTemplate.MaxPathLenZero = *opts.MaxPathLen == 0 + } - rootDER, err := x509.CreateCertificate(rand.Reader, rootTemplate, rootTemplate, rootPubKey, rootPrivKey) + rootDER, err := x509.CreateCertificate(rand.Reader, rootTemplate, rootTemplate, rootPrivKey.Public(), rootPrivKey) if err != nil { return nil, fmt.Errorf("while generating root certificate: %w", err) } @@ -184,9 +289,28 @@ func GenerateED25519CA(id string) (*CA, error) { } return &CA{ - ID: id, + ID: opts.ID, SigningKey: rootPrivKey, RootCertificate: rootCert, // No intermediates. }, nil } + +func generateKey(kt KeyType) (crypto.Signer, error) { + switch kt { + case KeyTypeED25519: + _, key, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + return nil, fmt.Errorf("while generating root key: %w", err) + } + return key, nil + case KeyTypeECDSAP256: + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + return nil, fmt.Errorf("while generating root key: %w", err) + } + return key, nil + default: + return nil, fmt.Errorf("unsupported key type %q, want one of %q or %q", kt, KeyTypeED25519, KeyTypeECDSAP256) + } +} diff --git a/internal/localca/localca_test.go b/internal/localca/localca_test.go index 0cca4ef6f..cfff6dbb2 100644 --- a/internal/localca/localca_test.go +++ b/internal/localca/localca_test.go @@ -16,13 +16,17 @@ package localca import ( "bytes" + "crypto" + "crypto/ecdsa" "crypto/ed25519" + "crypto/elliptic" "crypto/rand" "crypto/rsa" "crypto/x509" "crypto/x509/pkix" "encoding/json" "encoding/pem" + "io" "math/big" "strings" "testing" @@ -298,3 +302,204 @@ func TestUnmarshalErrors(t *testing.T) { }) } } + +// externalSigner stands in for a KMS or HSM signer: it can sign, but it holds +// no exportable key material. +type externalSigner struct{ inner ed25519.PrivateKey } + +func (e externalSigner) Public() crypto.PublicKey { return e.inner.Public() } +func (e externalSigner) Sign(r io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { + return e.inner.Sign(r, digest, opts) +} + +// The point of typing SigningKey as crypto.Signer is that a key living outside +// the process can be substituted. Verify that actually works end to end: such +// a signer can issue certificates, and Marshal refuses it with an explanation +// rather than x509's "unknown key type". +func TestExternalSignerCanIssueButCannotBeMarshalled(t *testing.T) { + base, err := GenerateED25519CA("external") + if err != nil { + t.Fatalf("GenerateED25519CA: %v", err) + } + ca := &CA{ + ID: base.ID, + SigningKey: externalSigner{inner: base.SigningKey.(ed25519.PrivateKey)}, + RootCertificate: base.RootCertificate, + } + + // Issuing works: x509.CreateCertificate only needs Public and Sign. + leafPub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generating leaf key: %v", err) + } + leafDER, err := x509.CreateCertificate(rand.Reader, &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "leaf"}, + NotBefore: time.Now().Add(-time.Minute), + NotAfter: time.Now().Add(time.Hour), + }, ca.RootCertificate, leafPub, ca.SigningKey) + if err != nil { + t.Fatalf("signing with an external signer: %v", err) + } + if _, err := x509.ParseCertificate(leafDER); err != nil { + t.Fatalf("parsing the issued leaf: %v", err) + } + + // Serializing does not, and must say why. + _, err = Marshal(&Pool{CAs: []*CA{ca}}) + if err == nil { + t.Fatal("Marshal serialized a signer with no exportable key material") + } + if !strings.Contains(err.Error(), "KMS") { + t.Errorf("error does not explain the external-signer case: %v", err) + } +} + +func TestGenerateCAKeyTypes(t *testing.T) { + for _, tc := range []struct { + keyType KeyType + check func(*testing.T, crypto.Signer) + }{ + {KeyTypeED25519, func(t *testing.T, k crypto.Signer) { + if _, ok := k.(ed25519.PrivateKey); !ok { + t.Errorf("key type = %T, want ed25519.PrivateKey", k) + } + }}, + {KeyTypeECDSAP256, func(t *testing.T, k crypto.Signer) { + ec, ok := k.(*ecdsa.PrivateKey) + if !ok { + t.Fatalf("key type = %T, want *ecdsa.PrivateKey", k) + } + if ec.Curve != elliptic.P256() { + t.Errorf("curve = %v, want P-256", ec.Curve.Params().Name) + } + }}, + } { + t.Run(string(tc.keyType), func(t *testing.T) { + ca, err := GenerateCA(GenerateOptions{ID: "k", KeyType: tc.keyType}) + if err != nil { + t.Fatalf("GenerateCA: %v", err) + } + tc.check(t, ca.SigningKey) + + // Whatever the algorithm, the result has to survive the pool. + data, err := Marshal(&Pool{CAs: []*CA{ca}}) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + restored, err := Unmarshal(data) + if err != nil { + t.Fatalf("Unmarshal: %v", err) + } + tc.check(t, restored.CAs[0].SigningKey) + }) + } + + if _, err := GenerateCA(GenerateOptions{ID: "k", KeyType: "rsa-8192"}); err == nil { + t.Error("GenerateCA accepted an unknown key type") + } +} + +func TestGenerateCANameConstraintsAreEnforced(t *testing.T) { + ca, err := GenerateCA(GenerateOptions{ + ID: "constrained", + CommonName: "constrained CA", + PermittedDNSDomains: []string{"permitted.example"}, + }) + if err != nil { + t.Fatalf("GenerateCA: %v", err) + } + if !ca.RootCertificate.PermittedDNSDomainsCritical { + t.Error("the name constraint is not marked critical, so a client that ignores it still trusts the chain") + } + + roots := x509.NewCertPool() + roots.AddCert(ca.RootCertificate) + + for name, host := range map[string]string{ + "inside": "api.permitted.example", + "outside": "api.forbidden.example", + } { + t.Run(name, func(t *testing.T) { + leafPub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generating leaf key: %v", err) + } + leafDER, err := x509.CreateCertificate(rand.Reader, &x509.Certificate{ + SerialNumber: big.NewInt(2), + DNSNames: []string{host}, + NotBefore: time.Now().Add(-time.Minute), + NotAfter: time.Now().Add(time.Hour), + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }, ca.RootCertificate, leafPub, ca.SigningKey) + if err != nil { + t.Fatalf("signing leaf: %v", err) + } + leaf, err := x509.ParseCertificate(leafDER) + if err != nil { + t.Fatalf("parsing leaf: %v", err) + } + + // Signing always succeeds -- CreateCertificate does not check + // constraints. Verification is where the constraint bites, which + // is the property that makes it a compromise backstop. + _, err = leaf.Verify(x509.VerifyOptions{ + DNSName: host, Roots: roots, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + }) + if name == "inside" && err != nil { + t.Errorf("in-constraint leaf failed to verify: %v", err) + } + if name == "outside" && err == nil { + t.Error("out-of-constraint leaf verified; the constraint is not enforced") + } + }) + } +} + +func TestGenerateCAMaxPathLen(t *testing.T) { + zero, one := 0, 1 + + unset, err := GenerateCA(GenerateOptions{ID: "unset"}) + if err != nil { + t.Fatalf("GenerateCA: %v", err) + } + // Historical behavior: no path length stated at all. A parsed certificate + // reports that as -1, which is how it differs from an explicit 0. + if unset.RootCertificate.MaxPathLenZero || unset.RootCertificate.MaxPathLen != -1 { + t.Errorf("unset: MaxPathLen=%d MaxPathLenZero=%v, want the constraint absent", + unset.RootCertificate.MaxPathLen, unset.RootCertificate.MaxPathLenZero) + } + + leafOnly, err := GenerateCA(GenerateOptions{ID: "leaf-only", MaxPathLen: &zero}) + if err != nil { + t.Fatalf("GenerateCA: %v", err) + } + // 0 and "absent" encode identically in the struct; MaxPathLenZero is what + // separates them, and getting it wrong silently permits intermediates. + if !leafOnly.RootCertificate.MaxPathLenZero { + t.Error("MaxPathLen 0 did not set MaxPathLenZero, so the CA still permits intermediates") + } + + delegating, err := GenerateCA(GenerateOptions{ID: "delegating", MaxPathLen: &one}) + if err != nil { + t.Fatalf("GenerateCA: %v", err) + } + if delegating.RootCertificate.MaxPathLen != 1 || delegating.RootCertificate.MaxPathLenZero { + t.Errorf("MaxPathLen=%d MaxPathLenZero=%v, want 1 and false", + delegating.RootCertificate.MaxPathLen, delegating.RootCertificate.MaxPathLenZero) + } +} + +func TestGenerateCALifetimeAndCommonName(t *testing.T) { + ca, err := GenerateCA(GenerateOptions{ID: "x", CommonName: "my ca", Lifetime: 2 * time.Hour}) + if err != nil { + t.Fatalf("GenerateCA: %v", err) + } + if got := ca.RootCertificate.Subject.CommonName; got != "my ca" { + t.Errorf("CN = %q, want %q", got, "my ca") + } + if got := ca.RootCertificate.NotAfter.Sub(ca.RootCertificate.NotBefore); got != 2*time.Hour { + t.Errorf("lifetime = %v, want 2h", got) + } +}