From 477da44127d1f45a8cb5fb2594614ff5934d4639 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 29 Jul 2024 15:12:24 -0700 Subject: [PATCH 1/6] split user keys for k8s access --- api/utils/keypaths/keypaths.go | 27 ++++++--- api/utils/keys/privatekey.go | 1 + lib/benchmark/kube.go | 6 +- lib/client/api.go | 5 +- lib/client/client_store.go | 6 +- lib/client/cluster_client.go | 16 +++-- lib/client/cluster_client_test.go | 77 ++++++++++++++++++++---- lib/client/identityfile/identity.go | 7 ++- lib/client/identityfile/identity_test.go | 4 +- lib/client/interfaces.go | 29 +++++---- lib/client/keyagent.go | 2 +- lib/client/keystore.go | 47 ++++++--------- lib/client/kube/kube.go | 4 +- lib/client/profile.go | 14 ----- lib/tbot/output_utils.go | 4 +- tool/tsh/common/kube.go | 13 ++-- 16 files changed, 161 insertions(+), 101 deletions(-) diff --git a/api/utils/keypaths/keypaths.go b/api/utils/keypaths/keypaths.go index 62f1e2b9381a7..fb83adc09d7e1 100644 --- a/api/utils/keypaths/keypaths.go +++ b/api/utils/keypaths/keypaths.go @@ -111,13 +111,16 @@ const ( // │ ├── foo-kube --> Kubernetes certs for user "foo" // │ | ├── root --> Kubernetes certs for Teleport cluster "root" // │ | │ ├── kubeA-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeA" -// │ | │ ├── kubeA-x509.pem --> TLS cert for Kubernetes cluster "kubeA" +// │ | │ ├── kubeA.crt --> TLS cert for Kubernetes cluster "kubeA" +// │ | │ ├── kubeA.key --> private key for Kubernetes cluster "kubeA" // │ | │ ├── kubeB-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeB" -// │ | │ ├── kubeB-x509.pem --> TLS cert for Kubernetes cluster "kubeB" +// │ | │ ├── kubeB.crt --> TLS cert for Kubernetes cluster "kubeB" +// │ | │ ├── kubeB.key --> private key for Kubernetes cluster "kubeB" // │ | │ └── localca.pem --> Self-signed localhost CA cert for Teleport cluster "root" // │ | └── leaf --> Kubernetes certs for Teleport cluster "leaf" // │ | ├── kubeC-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeC" -// │ | └── kubeC-x509.pem --> TLS cert for Kubernetes cluster "kubeC" +// │ | └── kubeC.crt --> TLS cert for Kubernetes cluster "kubeC" +// │ | └── kubeC.key --> private key for Kubernetes cluster "kubeC" // | └── cas --> Trusted clusters certificates // | ├── root.pem --> TLS CA for teleport cluster "root" // | ├── leaf1.pem --> TLS CA for teleport cluster "leaf1" @@ -316,20 +319,28 @@ func KubeDir(baseDir, proxy, username string) string { return filepath.Join(ProxyKeyDir(baseDir, proxy), username+kubeDirSuffix) } -// KubeCertDir returns the path to the user's kube cert directory +// KubeCredentialDir returns the path to the user's kube credential directory // for the given proxy and cluster. // // /keys//-kube/ -func KubeCertDir(baseDir, proxy, username, cluster string) string { +func KubeCredentialDir(baseDir, proxy, username, cluster string) string { return filepath.Join(KubeDir(baseDir, proxy, username), cluster) } // KubeCertPath returns the path to the user's TLS certificate // for the given proxy, cluster, and kube cluster. // -// /keys//-kube//-x509.pem +// /keys//-kube//.crt func KubeCertPath(baseDir, proxy, username, cluster, kubename string) string { - return filepath.Join(KubeCertDir(baseDir, proxy, username, cluster), kubename+fileExtTLSCertLegacy) + return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+fileExtTLSCert) +} + +// KubeKeyPath returns the path to the user's TLS private key +// for the given proxy, cluster, and kube cluster. +// +// /keys//-kube//.key +func KubeKeyPath(baseDir, proxy, username, cluster, kubename string) string { + return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+fileExtTLSKey) } // KubeConfigPath returns the path to the user's standalone kubeconfig @@ -337,7 +348,7 @@ func KubeCertPath(baseDir, proxy, username, cluster, kubename string) string { // // /keys//-kube//-kubeconfig func KubeConfigPath(baseDir, proxy, username, cluster, kubename string) string { - return filepath.Join(KubeCertDir(baseDir, proxy, username, cluster), kubename+kubeConfigSuffix) + return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+kubeConfigSuffix) } // KubeCredLockfilePath returns the kube credentials lock file for given proxy diff --git a/api/utils/keys/privatekey.go b/api/utils/keys/privatekey.go index acf71c41609e2..187ff002058b2 100644 --- a/api/utils/keys/privatekey.go +++ b/api/utils/keys/privatekey.go @@ -110,6 +110,7 @@ func TLSCertificateForSigner(signer crypto.Signer, certPEMBlock []byte) (tls.Cer return tls.Certificate{}, trace.Wrap(err) } cert.Certificate = rawCerts + cert.Leaf = x509Cert // Check that the certificate's public key matches this private key. if keyPub, ok := signer.Public().(cryptoPublicKeyI); !ok { diff --git a/lib/benchmark/kube.go b/lib/benchmark/kube.go index 824584617b29b..e5bc048bd76ee 100644 --- a/lib/benchmark/kube.go +++ b/lib/benchmark/kube.go @@ -99,9 +99,9 @@ func getKubeTLSClientConfig(ctx context.Context, tc *client.TeleportClient) (res return rest.TLSClientConfig{}, trace.Wrap(err) } - certPem := k.KubeTLSCerts[tc.KubernetesCluster] + cred := k.KubeTLSCredentials[tc.KubernetesCluster] - keyPEM, err := k.PrivateKey.SoftwarePrivateKeyPEM() + keyPEM, err := cred.PrivateKey.SoftwarePrivateKeyPEM() if err != nil { return rest.TLSClientConfig{}, trace.Wrap(err) } @@ -132,7 +132,7 @@ func getKubeTLSClientConfig(ctx context.Context, tc *client.TeleportClient) (res return rest.TLSClientConfig{ CAData: bytes.Join(clusterCAs, []byte("\n")), - CertData: certPem, + CertData: cred.Cert, KeyData: keyPEM, ServerName: tlsServerName, }, nil diff --git a/lib/client/api.go b/lib/client/api.go index 3e2fcb2b6e999..40dc2d5ef7b70 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -3724,7 +3724,10 @@ func (tc *TeleportClient) SSHLogin(ctx context.Context, sshLoginFunc SSHLoginFun keyRing.ProxyHost = tc.WebProxyHost() if tc.KubernetesCluster != "" { - keyRing.KubeTLSCerts[tc.KubernetesCluster] = response.TLSCert + keyRing.KubeTLSCredentials[tc.KubernetesCluster] = TLSCredential{ + Cert: response.TLSCert, + PrivateKey: priv, + } } if tc.DatabaseService != "" { keyRing.DBTLSCredentials[tc.DatabaseService] = TLSCredential{ diff --git a/lib/client/client_store.go b/lib/client/client_store.go index d9fa274f6cb7c..9ab97d2c31272 100644 --- a/lib/client/client_store.go +++ b/lib/client/client_store.go @@ -260,8 +260,8 @@ func (s *Store) FullProfileStatus() (*ProfileStatus, []*ProfileStatus, error) { // parallel. // Although this function speeds up the process since it removes all transversals, // it still has to read 2 different files: -// - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER-x509.pem -// - $TSH_HOME/keys/$PROXY/$USER +// - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.crt +// - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.key func LoadKeysToKubeFromStore(profile *profile.Profile, dirPath, teleportCluster, kubeCluster string) ([]byte, []byte, error) { fsKeyStore := NewFSKeyStore(dirPath) @@ -271,7 +271,7 @@ func LoadKeysToKubeFromStore(profile *profile.Profile, dirPath, teleportCluster, return nil, nil, trace.Wrap(err) } - privKeyPath := fsKeyStore.userKeyPath(KeyRingIndex{ProxyHost: profile.SiteName, Username: profile.Username}) + privKeyPath := fsKeyStore.kubeKeyPath(KeyRingIndex{ProxyHost: profile.SiteName, ClusterName: teleportCluster, Username: profile.Username}, kubeCluster) privKey, err := os.ReadFile(privKeyPath) if err != nil { return nil, nil, trace.Wrap(err) diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index 7fb8d839621bf..5e7da5178dac2 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -241,7 +241,10 @@ func (c *ClusterClient) generateUserCerts(ctx context.Context, cachePolicy CertC PrivateKey: privKey, } case proto.UserCertsRequest_Kubernetes: - keyRing.KubeTLSCerts[params.KubernetesCluster] = certs.TLS + keyRing.KubeTLSCredentials[params.KubernetesCluster] = TLSCredential{ + PrivateKey: privKey, + Cert: certs.TLS, + } case proto.UserCertsRequest_WindowsDesktop: keyRing.WindowsDesktopCerts[params.RouteToWindowsDesktop.WindowsDesktop] = certs.TLS } @@ -353,7 +356,7 @@ func (c *ClusterClient) prepareUserCertsRequest(ctx context.Context, params Reis var sshPublicKey, tlsPublicKey []byte var privateKey *keys.PrivateKey switch params.usage() { - case proto.UserCertsRequest_App: + case proto.UserCertsRequest_App, proto.UserCertsRequest_Kubernetes: privateKey, err = keyRing.GenerateKey(ctx, c.tc, cryptosuites.UserTLS) if err != nil { return nil, nil, trace.Wrap(err) @@ -670,10 +673,13 @@ func PerformMFACeremony(ctx context.Context, params PerformMFACeremonyParams) (* case len(newCerts.TLS) > 0: switch certsReq.Usage { case proto.UserCertsRequest_Kubernetes: - if keyRing.KubeTLSCerts == nil { - keyRing.KubeTLSCerts = make(map[string][]byte) + if keyRing.KubeTLSCredentials == nil { + keyRing.KubeTLSCredentials = make(map[string]TLSCredential) + } + keyRing.KubeTLSCredentials[certsReq.KubernetesCluster] = TLSCredential{ + Cert: newCerts.TLS, + PrivateKey: params.PrivateKey, } - keyRing.KubeTLSCerts[certsReq.KubernetesCluster] = newCerts.TLS case proto.UserCertsRequest_Database: dbCert, err := makeDatabaseClientPEM(certsReq.RouteToDatabase.Protocol, newCerts.TLS, params.PrivateKey) diff --git a/lib/client/cluster_client_test.go b/lib/client/cluster_client_test.go index d72b644bbffcb..1079cd0d97cf0 100644 --- a/lib/client/cluster_client_test.go +++ b/lib/client/cluster_client_test.go @@ -18,6 +18,7 @@ package client import ( "context" + "crypto/x509" "errors" "testing" @@ -28,7 +29,9 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/client/proxy" + "github.com/gravitational/teleport/api/client/webclient" "github.com/gravitational/teleport/api/mfa" + "github.com/gravitational/teleport/api/types" webauthnpb "github.com/gravitational/teleport/api/types/webauthn" "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/lib/auth/authclient" @@ -124,12 +127,13 @@ func TestIssueUserCertsWithMFA(t *testing.T) { failedPrompt := fakePrompt{err: errors.New("prompt failed intentionally")} tests := []struct { - name string - mfaRequired proto.MFARequired - agent *LocalKeyAgent - params ReissueParams - prompt fakePrompt - assertion func(t *testing.T, keyRing *KeyRing, mfaRequired proto.MFARequired, err error) + name string + mfaRequired proto.MFARequired + agent *LocalKeyAgent + params ReissueParams + prompt fakePrompt + signatureAlgorithmSuite types.SignatureAlgorithmSuite + assertion func(t *testing.T, keyRing *KeyRing, mfaRequired proto.MFARequired, err error) }{ { name: "ssh no mfa", @@ -139,6 +143,7 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_NO, mfaRequired) + require.NotEmpty(t, keyRing.Cert) }, }, { @@ -149,6 +154,7 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) + require.NotEmpty(t, keyRing.Cert) }, }, { @@ -170,6 +176,7 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_NO, mfaRequired) + require.NotEmpty(t, keyRing.KubeTLSCredentials["test"]) }, }, { @@ -180,6 +187,27 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) + cred := keyRing.KubeTLSCredentials["test"] + require.NotEmpty(t, cred) + cert, err := cred.TLSCertificate() + require.NoError(t, err) + require.Equal(t, x509.ECDSA, cert.Leaf.PublicKeyAlgorithm) + }, + }, + { + name: "kube legacy", + mfaRequired: proto.MFARequired_MFA_REQUIRED_YES, + params: ReissueParams{KubernetesCluster: "test"}, + signatureAlgorithmSuite: types.SignatureAlgorithmSuite_SIGNATURE_ALGORITHM_SUITE_LEGACY, + assertion: func(t *testing.T, keyRing *KeyRing, mfaRequired proto.MFARequired, err error) { + require.NoError(t, err) + require.NotNil(t, keyRing) + require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) + cred := keyRing.KubeTLSCredentials["test"] + require.NotEmpty(t, cred) + cert, err := cred.TLSCertificate() + require.NoError(t, err) + require.Equal(t, x509.RSA, cert.Leaf.PublicKeyAlgorithm) }, }, { @@ -197,14 +225,16 @@ func TestIssueUserCertsWithMFA(t *testing.T) { mfaRequired: proto.MFARequired_MFA_REQUIRED_NO, params: ReissueParams{ RouteToDatabase: proto.RouteToDatabase{ - Username: "test", - Database: "test", + ServiceName: "test", + Username: "test", + Database: "test", }, }, assertion: func(t *testing.T, keyRing *KeyRing, mfaRequired proto.MFARequired, err error) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_NO, mfaRequired) + require.NotEmpty(t, keyRing.DBTLSCredentials["test"]) }, }, { @@ -212,14 +242,20 @@ func TestIssueUserCertsWithMFA(t *testing.T) { mfaRequired: proto.MFARequired_MFA_REQUIRED_YES, params: ReissueParams{ RouteToDatabase: proto.RouteToDatabase{ - Username: "test", - Database: "test", + ServiceName: "test", + Username: "test", + Database: "test", }, }, assertion: func(t *testing.T, keyRing *KeyRing, mfaRequired proto.MFARequired, err error) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) + cred := keyRing.DBTLSCredentials["test"] + require.NotEmpty(t, cred) + cert, err := cred.TLSCertificate() + require.NoError(t, err) + require.Equal(t, x509.RSA, cert.Leaf.PublicKeyAlgorithm) }, }, { @@ -284,6 +320,7 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_NO, mfaRequired) + require.NotEmpty(t, keyRing.Cert) }, }, { @@ -302,6 +339,7 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.NoError(t, err) require.NotNil(t, keyRing) require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) + require.NotEmpty(t, keyRing.Cert) }, }, } @@ -313,10 +351,23 @@ func TestIssueUserCertsWithMFA(t *testing.T) { agent = test.agent } + suite := test.signatureAlgorithmSuite + if suite == types.SignatureAlgorithmSuite_SIGNATURE_ALGORITHM_SUITE_UNSPECIFIED { + suite = types.SignatureAlgorithmSuite_SIGNATURE_ALGORITHM_SUITE_BALANCED_V1 + } + clt := &ClusterClient{ tc: &TeleportClient{ localAgent: agent, - Config: Config{SiteName: "test"}, + Config: Config{ + SiteName: "test", + Tracer: tracing.NoopTracer("test"), + }, + lastPing: &webclient.PingResponse{ + Auth: webclient.AuthenticationSettings{ + SignatureAlgorithmSuite: suite, + }, + }, }, ProxyClient: &proxy.Client{}, AuthClient: fakeAuthClient{ @@ -373,8 +424,8 @@ func TestIssueUserCertsWithMFA(t *testing.T) { ctx := context.Background() - key, mfaRequired, err := clt.IssueUserCertsWithMFA(ctx, test.params, test.prompt) - test.assertion(t, key, mfaRequired, err) + keyRing, mfaRequired, err := clt.IssueUserCertsWithMFA(ctx, test.params, test.prompt) + test.assertion(t, keyRing, mfaRequired, err) }) } } diff --git a/lib/client/identityfile/identity.go b/lib/client/identityfile/identity.go index 39137c872fc7f..48bb1ce4ddcce 100644 --- a/lib/client/identityfile/identity.go +++ b/lib/client/identityfile/identity.go @@ -787,7 +787,12 @@ func KeyRingFromIdentityFile(identityPath, proxyHost, clusterName string) (*clie // If this identity file has any kubernetes certs, copy it into the // KubeTLSCerts map. if parsedIdent.KubernetesCluster != "" { - keyRing.KubeTLSCerts[parsedIdent.KubernetesCluster] = ident.Certs.TLS + keyRing.KubeTLSCredentials[parsedIdent.KubernetesCluster] = client.TLSCredential{ + // Identity files only have room for one private key, it must + // match the kube cert. + PrivateKey: priv, + Cert: ident.Certs.TLS, + } } } else { keyRing.Username, err = keyRing.CertUsername() diff --git a/lib/client/identityfile/identity_test.go b/lib/client/identityfile/identity_test.go index d218143efa29e..57e5585a94c1e 100644 --- a/lib/client/identityfile/identity_test.go +++ b/lib/client/identityfile/identity_test.go @@ -416,8 +416,8 @@ func TestKeyFromIdentityFile(t *testing.T) { require.NoError(t, err) parsedKeyRing, err := KeyRingFromIdentityFile(identityFilePath, proxyHost, cluster) require.NoError(t, err) - require.NotNil(t, parsedKeyRing.KubeTLSCerts[k8sCluster]) - require.Equal(t, keyRing.TLSCert, parsedKeyRing.KubeTLSCerts[k8sCluster]) + require.NotNil(t, parsedKeyRing.KubeTLSCredentials[k8sCluster].PrivateKey) + require.Equal(t, keyRing.TLSCert, parsedKeyRing.KubeTLSCredentials[k8sCluster].Cert) }) } diff --git a/lib/client/interfaces.go b/lib/client/interfaces.go index 05846ab276b56..80bf653072216 100644 --- a/lib/client/interfaces.go +++ b/lib/client/interfaces.go @@ -110,9 +110,9 @@ type KeyRing struct { // TLSCert is a PEM encoded client TLS x509 certificate. // It's used to authenticate to the Teleport APIs. TLSCert []byte `json:"TLSCert,omitempty"` - // KubeTLSCerts are TLS certificates (PEM-encoded) for individual + // KubeTLSCredentials are TLS credentials for individual // kubernetes clusters. Map key is a kubernetes cluster name. - KubeTLSCerts map[string][]byte `json:"KubeCerts,omitempty"` + KubeTLSCredentials map[string]TLSCredential // DBTLSCredentials are TLS credentials for database access. // Map key is the database service name. DBTLSCredentials map[string]TLSCredential @@ -177,7 +177,7 @@ func GenerateRSAKeyRing() (*KeyRing, error) { func NewKeyRing(priv *keys.PrivateKey) *KeyRing { return &KeyRing{ PrivateKey: priv, - KubeTLSCerts: make(map[string][]byte), + KubeTLSCredentials: make(map[string]TLSCredential), DBTLSCredentials: make(map[string]TLSCredential), AppTLSCredentials: make(map[string]TLSCredential), WindowsDesktopCerts: make(map[string][]byte), @@ -221,12 +221,12 @@ func (k *KeyRing) KubeClientTLSConfig(cipherSuites []uint16, kubeClusterName str if err != nil { return nil, trace.Wrap(err) } - tlsCert, ok := k.KubeTLSCerts[kubeClusterName] + cred, ok := k.KubeTLSCredentials[kubeClusterName] if !ok { return nil, trace.NotFound("TLS certificate for kubernetes cluster %q not found", kubeClusterName) } - tlsConfig, err := k.clientTLSConfig(cipherSuites, tlsCert, []string{rootCluster}) + tlsConfig, err := k.clientTLSConfig(cipherSuites, cred, []string{rootCluster}) if err != nil { return nil, trace.Wrap(err) } @@ -275,11 +275,14 @@ func (k *KeyRing) TeleportClientTLSConfig(cipherSuites []uint16, clusters []stri if len(k.TLSCert) == 0 { return nil, trace.NotFound("TLS certificate not found") } - return k.clientTLSConfig(cipherSuites, k.TLSCert, clusters) + return k.clientTLSConfig(cipherSuites, TLSCredential{ + PrivateKey: k.PrivateKey, + Cert: k.TLSCert, + }, clusters) } -func (k *KeyRing) clientTLSConfig(cipherSuites []uint16, tlsCertRaw []byte, clusters []string) (*tls.Config, error) { - tlsCert, err := k.PrivateKey.TLSCertificate(tlsCertRaw) +func (k *KeyRing) clientTLSConfig(cipherSuites []uint16, cred TLSCredential, clusters []string) (*tls.Config, error) { + tlsCert, err := cred.TLSCertificate() if err != nil { return nil, trace.Wrap(err) } @@ -460,21 +463,21 @@ func (k *KeyRing) TeleportTLSCertificate() (*x509.Certificate, error) { // KubeX509Cert returns the parsed x509 certificate for authentication against // a named kubernetes cluster. func (k *KeyRing) KubeX509Cert(kubeClusterName string) (*x509.Certificate, error) { - tlsCert, ok := k.KubeTLSCerts[kubeClusterName] + cred, ok := k.KubeTLSCredentials[kubeClusterName] if !ok { - return nil, trace.NotFound("TLS certificate for kubernetes cluster %q not found", kubeClusterName) + return nil, trace.NotFound("TLS credential for kubernetes cluster %q not found", kubeClusterName) } - return tlsca.ParseCertificatePEM(tlsCert) + return tlsca.ParseCertificatePEM(cred.Cert) } // KubeTLSCert returns the tls.Certificate for authentication against a named // kubernetes cluster. func (k *KeyRing) KubeTLSCert(kubeClusterName string) (tls.Certificate, error) { - certPem, ok := k.KubeTLSCerts[kubeClusterName] + cred, ok := k.KubeTLSCredentials[kubeClusterName] if !ok { return tls.Certificate{}, trace.NotFound("TLS certificate for kubernetes cluster %q not found", kubeClusterName) } - tlsCert, err := k.PrivateKey.TLSCertificate(certPem) + tlsCert, err := cred.TLSCertificate() if err != nil { return tls.Certificate{}, trace.Wrap(err) } diff --git a/lib/client/keyagent.go b/lib/client/keyagent.go index b9a8107988550..89181fee0ad02 100644 --- a/lib/client/keyagent.go +++ b/lib/client/keyagent.go @@ -515,7 +515,7 @@ func (a *LocalKeyAgent) AddDatabaseKeyRing(keyRing *KeyRing) error { // AddKubeKeyRing activates a new signed Kubernetes key by adding it into the keystore. // key must contain at least one Kubernetes cert. ssh cert is not required. func (a *LocalKeyAgent) AddKubeKeyRing(keyRing *KeyRing) error { - if len(keyRing.KubeTLSCerts) == 0 { + if len(keyRing.KubeTLSCredentials) == 0 { return trace.BadParameter("key ring must contain at least one Kubernetes access certificate") } return a.addKeyRing(keyRing) diff --git a/lib/client/keystore.go b/lib/client/keystore.go index bd968fba10735..9d266ab5386cd 100644 --- a/lib/client/keystore.go +++ b/lib/client/keystore.go @@ -165,6 +165,11 @@ func (fs *FSKeyStore) kubeCertPath(idx KeyRingIndex, kubename string) string { return keypaths.KubeCertPath(fs.KeyDir, idx.ProxyHost, idx.Username, idx.ClusterName, kubename) } +// kubeKeyPath returns the private key path for the given KeyRingIndex and kube cluster name. +func (fs *FSKeyStore) kubeKeyPath(idx KeyRingIndex, kubename string) string { + return keypaths.KubeKeyPath(fs.KeyDir, idx.ProxyHost, idx.Username, idx.ClusterName, kubename) +} + // AddKeyRing adds the given key ring to the store. func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { if err := keyRing.KeyRingIndex.Check(); err != nil { @@ -205,8 +210,7 @@ func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { } } - // TODO(awly): unit test this. - for kubeCluster, cert := range keyRing.KubeTLSCerts { + for kubeCluster, cred := range keyRing.KubeTLSCredentials { // Prevent directory traversal via a crafted kubernetes cluster name. // // This will confuse cluster cert loading (GetKeyRing will return @@ -214,8 +218,12 @@ func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { // don't expect any well-meaning user to create bad names. kubeCluster = filepath.Clean(kubeCluster) - path := fs.kubeCertPath(keyRing.KeyRingIndex, kubeCluster) - if err := fs.writeBytes(cert, path); err != nil { + certPath := fs.kubeCertPath(keyRing.KeyRingIndex, kubeCluster) + if err := fs.writeBytes(cred.Cert, certPath); err != nil { + return trace.Wrap(err) + } + keyPath := fs.kubeKeyPath(keyRing.KeyRingIndex, kubeCluster) + if err := fs.writeBytes(cred.PrivateKey.PrivateKeyPEM(), keyPath); err != nil { return trace.Wrap(err) } } @@ -401,25 +409,6 @@ func (fs *FSKeyStore) GetSSHCertificates(proxyHost, username string) ([]*ssh.Cer return sshCerts, nil } -func getCertsByName(certDir string) (map[string][]byte, error) { - certsByName := make(map[string][]byte) - certFiles, err := os.ReadDir(certDir) - if err != nil { - return nil, trace.ConvertSystemError(err) - } - for _, certFile := range certFiles { - name := keypaths.TrimCertPathSuffix(certFile.Name()) - if isCert := name != certFile.Name(); isCert { - data, err := os.ReadFile(filepath.Join(certDir, certFile.Name())) - if err != nil { - return nil, trace.ConvertSystemError(err) - } - certsByName[name] = data - } - } - return certsByName, nil -} - func getCredentialsByName(credentialDir string) (map[string]TLSCredential, error) { credsByName := make(map[string]TLSCredential) files, err := os.ReadDir(credentialDir) @@ -504,12 +493,12 @@ func (o WithSSHCerts) deleteFromKeyRing(keyRing *KeyRing) { type WithKubeCerts struct{} func (o WithKubeCerts) updateKeyRing(keyDir string, idx KeyRingIndex, keyRing *KeyRing) error { - certDir := keypaths.KubeCertDir(keyDir, idx.ProxyHost, idx.Username, idx.ClusterName) - certsByName, err := getCertsByName(certDir) + credentialDir := keypaths.KubeCredentialDir(keyDir, idx.ProxyHost, idx.Username, idx.ClusterName) + credsByName, err := getCredentialsByName(credentialDir) if err != nil { return trace.Wrap(err) } - keyRing.KubeTLSCerts = certsByName + keyRing.KubeTLSCredentials = credsByName return nil } @@ -517,11 +506,11 @@ func (o WithKubeCerts) pathsToDelete(keyDir string, idx KeyRingIndex) []string { if idx.ClusterName == "" { return []string{keypaths.KubeDir(keyDir, idx.ProxyHost, idx.Username)} } - return []string{keypaths.KubeCertDir(keyDir, idx.ProxyHost, idx.Username, idx.ClusterName)} + return []string{keypaths.KubeCredentialDir(keyDir, idx.ProxyHost, idx.Username, idx.ClusterName)} } func (o WithKubeCerts) deleteFromKeyRing(keyRing *KeyRing) { - keyRing.KubeTLSCerts = make(map[string][]byte) + keyRing.KubeTLSCredentials = make(map[string]TLSCredential) } // WithDBCerts is a CertOption for handling database access certificates. @@ -661,7 +650,7 @@ func (ms *MemKeyStore) GetKeyRing(idx KeyRingIndex, opts ...CertOption) (*KeyRin case WithSSHCerts: retKeyRing.Cert = keyRing.Cert case WithKubeCerts: - retKeyRing.KubeTLSCerts = keyRing.KubeTLSCerts + retKeyRing.KubeTLSCredentials = keyRing.KubeTLSCredentials case WithDBCerts: retKeyRing.DBTLSCredentials = keyRing.DBTLSCredentials case WithAppCerts: diff --git a/lib/client/kube/kube.go b/lib/client/kube/kube.go index 80e6e83ad5014..cb02b8ab1dfe8 100644 --- a/lib/client/kube/kube.go +++ b/lib/client/kube/kube.go @@ -42,12 +42,12 @@ func CheckIfCertsAreAllowedToAccessCluster(k *client.KeyRing, rootCluster, telep if rootCluster != teleportCluster { return nil } - for k8sCluster, cert := range k.KubeTLSCerts { + for k8sCluster, cred := range k.KubeTLSCredentials { if k8sCluster != kubeCluster { continue } log.Debugf("Got TLS cert for Kubernetes cluster %q", k8sCluster) - exist, err := checkIfCertHasKubeGroupsAndUsers(cert) + exist, err := checkIfCertHasKubeGroupsAndUsers(cred.Cert) if err != nil { return trace.Wrap(err) } else if exist { diff --git a/lib/client/profile.go b/lib/client/profile.go index 48f526bd5cfc3..08618f0ebbb35 100644 --- a/lib/client/profile.go +++ b/lib/client/profile.go @@ -561,20 +561,6 @@ func (p *ProfileStatus) KubeConfigPath(name string) string { return keypaths.KubeConfigPath(p.Dir, p.Name, p.Username, p.Cluster, name) } -// KubeCertPathForCluster returns path to the specified kube access certificate -// for this profile, for the specified cluster name. -// -// It's kept in /keys//-kube//-x509.pem -func (p *ProfileStatus) KubeCertPathForCluster(teleportCluster, kubeCluster string) string { - if teleportCluster == "" { - teleportCluster = p.Cluster - } - if path, ok := p.virtualPathFromEnv(VirtualPathKubernetes, VirtualPathKubernetesParams(kubeCluster)); ok { - return path - } - return keypaths.KubeCertPath(p.Dir, p.Name, p.Username, teleportCluster, kubeCluster) -} - // DatabaseServices returns a list of database service names for this profile. func (p *ProfileStatus) DatabaseServices() (result []string) { for _, db := range p.Databases { diff --git a/lib/tbot/output_utils.go b/lib/tbot/output_utils.go index 351decdb0b0e6..52f58526d2b1b 100644 --- a/lib/tbot/output_utils.go +++ b/lib/tbot/output_utils.go @@ -122,8 +122,8 @@ func NewClientKeyRing(ident *identity.Identity, hostCAs []types.CertAuthority) ( // Note: these fields are never used or persisted with identity files, // so we won't bother to set them. (They may need to be reconstituted // on tsh's end based on cert fields, though.) - KubeTLSCerts: make(map[string][]byte), - DBTLSCredentials: make(map[string]client.TLSCredential), + KubeTLSCredentials: make(map[string]client.TLSCredential), + DBTLSCredentials: make(map[string]client.TLSCredential), }, nil } diff --git a/tool/tsh/common/kube.go b/tool/tsh/common/kube.go index 630e06fbb0038..987ae3b01f20b 100644 --- a/tool/tsh/common/kube.go +++ b/tool/tsh/common/kube.go @@ -632,8 +632,8 @@ func (c *kubeCredentialsCommand) run(cf *CLIConf) error { // we call the function multiple times in parallel. // Although client.LoadKeysToKubeFromStore function speeds up the process since // it removes all transversals, it still has to read 2 different files from the disk: - // - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER-x509.pem - // - $TSH_HOME/keys/$PROXY/$USER + // - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.crt + // - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.key // // In addition to these files, $TSH_HOME/$profile.yaml is also read from // cf.GetProfile call above. @@ -803,14 +803,19 @@ func (c *kubeCredentialsCommand) writeKeyResponse(output io.Writer, keyRing *cli expiry = expiry.Add(-1 * time.Minute) } + cred, ok := keyRing.KubeTLSCredentials[kubeClusterName] + if !ok { + return trace.NotFound("TLS credential for kubernetes cluster %q not found", kubeClusterName) + } + // TODO (Joerger): Create a custom k8s Auth Provider or Exec Provider to use // hardware private keys for kube credentials (if possible) - keyPEM, err := keyRing.PrivateKey.SoftwarePrivateKeyPEM() + keyPEM, err := cred.PrivateKey.SoftwarePrivateKeyPEM() if err != nil { return trace.Wrap(err) } - return trace.Wrap(c.writeResponse(output, keyRing.KubeTLSCerts[kubeClusterName], keyPEM, expiry)) + return trace.Wrap(c.writeResponse(output, cred.Cert, keyPEM, expiry)) } // writeByteResponse writes the exec credential response to the output stream. From 84e0c96bf75e245303b40e44ee07d6107cbe9da4 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Tue, 6 Aug 2024 10:00:16 -0700 Subject: [PATCH 2/6] remove unnecessary loop --- lib/client/kube/kube.go | 8 +++----- tool/tsh/common/kube_proxy.go | 8 ++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/client/kube/kube.go b/lib/client/kube/kube.go index cb02b8ab1dfe8..ff70bdb8fc375 100644 --- a/lib/client/kube/kube.go +++ b/lib/client/kube/kube.go @@ -42,15 +42,13 @@ func CheckIfCertsAreAllowedToAccessCluster(k *client.KeyRing, rootCluster, telep if rootCluster != teleportCluster { return nil } - for k8sCluster, cred := range k.KubeTLSCredentials { - if k8sCluster != kubeCluster { - continue - } + if cred, ok := k.KubeTLSCredentials[kubeCluster]; ok { log.Debugf("Got TLS cert for Kubernetes cluster %q", k8sCluster) exist, err := checkIfCertHasKubeGroupsAndUsers(cred.Cert) if err != nil { return trace.Wrap(err) - } else if exist { + } + if exist { return nil } } diff --git a/tool/tsh/common/kube_proxy.go b/tool/tsh/common/kube_proxy.go index cfd73cb1b075f..4800f88e54a5c 100644 --- a/tool/tsh/common/kube_proxy.go +++ b/tool/tsh/common/kube_proxy.go @@ -576,7 +576,7 @@ func issueKubeCert(ctx context.Context, tc *client.TeleportClient, clusterClient requesterName = proto.UserCertsRequest_TSH_KUBE_LOCAL_PROXY_HEADLESS } - key, mfaRequired, err := clusterClient.IssueUserCertsWithMFA( + keyRing, mfaRequired, err := clusterClient.IssueUserCertsWithMFA( ctx, client.ReissueParams{ RouteToCluster: teleportCluster, @@ -599,7 +599,7 @@ func issueKubeCert(ctx context.Context, tc *client.TeleportClient, clusterClient return tls.Certificate{}, trace.Wrap(err) } if err := kubeclient.CheckIfCertsAreAllowedToAccessCluster( - key, + keyRing, rootClusterName, teleportCluster, kubeCluster); err != nil { @@ -608,12 +608,12 @@ func issueKubeCert(ctx context.Context, tc *client.TeleportClient, clusterClient // Save it if MFA was not required. if mfaRequired == proto.MFARequired_MFA_REQUIRED_NO { - if err := tc.LocalAgent().AddKubeKeyRing(key); err != nil { + if err := tc.LocalAgent().AddKubeKeyRing(keyRing); err != nil { return tls.Certificate{}, trace.Wrap(err) } } - cert, err := key.KubeTLSCert(kubeCluster) + cert, err := keyRing.KubeTLSCert(kubeCluster) if err != nil { return tls.Certificate{}, trace.Wrap(err) } From a09f67283c82f5bc0c62e7eab786255f43745f24 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Thu, 8 Aug 2024 14:37:52 -0700 Subject: [PATCH 3/6] prevent race when reading/writing key/cert pairs --- api/utils/keypaths/keypaths.go | 12 +-- lib/client/client_store.go | 16 +--- lib/client/client_store_test.go | 104 ++++++++++++++++++++ lib/client/keystore.go | 163 +++++++++++++++++++++++--------- lib/client/kube/kube.go | 2 +- tool/tsh/common/kube.go | 3 +- 6 files changed, 234 insertions(+), 66 deletions(-) diff --git a/api/utils/keypaths/keypaths.go b/api/utils/keypaths/keypaths.go index fb83adc09d7e1..34500f23ef358 100644 --- a/api/utils/keypaths/keypaths.go +++ b/api/utils/keypaths/keypaths.go @@ -34,8 +34,8 @@ const ( fileNameKnownHosts = "known_hosts" // fileExtTLSCertLegacy is the legacy suffix/extension of a file where a TLS cert is stored. fileExtTLSCertLegacy = "-x509.pem" - // fileExtTLSCert is the suffix/extension of a file where a TLS cert is stored. - fileExtTLSCert = ".crt" + // FileExtTLSCert is the suffix/extension of a file where a TLS cert is stored. + FileExtTLSCert = ".crt" // fileExtTLSKey is the suffix/extension of a file where a TLS private key is stored. fileExtTLSKey = ".key" // fileNameTLSCerts is a file where TLS Cert Authorities are stored. @@ -253,7 +253,7 @@ func AppCredentialDir(baseDir, proxy, username, cluster string) string { // // /keys//-app//.crt func AppCertPath(baseDir, proxy, username, cluster, appname string) string { - return filepath.Join(AppCredentialDir(baseDir, proxy, username, cluster), appname+fileExtTLSCert) + return filepath.Join(AppCredentialDir(baseDir, proxy, username, cluster), appname+FileExtTLSCert) } // AppKeyPath returns the path to the user's private key for the given proxy, @@ -293,7 +293,7 @@ func DatabaseCredentialDir(baseDir, proxy, username, cluster string) string { // // /keys//-db//.crt func DatabaseCertPath(baseDir, proxy, username, cluster, dbname string) string { - return filepath.Join(DatabaseCredentialDir(baseDir, proxy, username, cluster), dbname+fileExtTLSCert) + return filepath.Join(DatabaseCredentialDir(baseDir, proxy, username, cluster), dbname+FileExtTLSCert) } // DatabaseKeyPath returns the path to the user's TLS private key @@ -332,7 +332,7 @@ func KubeCredentialDir(baseDir, proxy, username, cluster string) string { // // /keys//-kube//.crt func KubeCertPath(baseDir, proxy, username, cluster, kubename string) string { - return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+fileExtTLSCert) + return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+FileExtTLSCert) } // KubeKeyPath returns the path to the user's TLS private key @@ -380,7 +380,7 @@ func IdentitySSHCertPath(path string) string { // TrimCertPathSuffix returns the given path with any cert suffix/extension trimmed off. func TrimCertPathSuffix(path string) string { trimmedPath := strings.TrimSuffix(path, fileExtTLSCertLegacy) - trimmedPath = strings.TrimSuffix(trimmedPath, fileExtTLSCert) + trimmedPath = strings.TrimSuffix(trimmedPath, FileExtTLSCert) trimmedPath = strings.TrimSuffix(trimmedPath, fileExtSSHCert) return trimmedPath } diff --git a/lib/client/client_store.go b/lib/client/client_store.go index 9ab97d2c31272..cd1031afba563 100644 --- a/lib/client/client_store.go +++ b/lib/client/client_store.go @@ -21,7 +21,6 @@ package client import ( "errors" "net/url" - "os" "time" "github.com/gravitational/trace" @@ -259,26 +258,21 @@ func (s *Store) FullProfileStatus() (*ProfileStatus, []*ProfileStatus, error) { // when the store has a lot of keys and when we call the function multiple times in // parallel. // Although this function speeds up the process since it removes all transversals, -// it still has to read 2 different files: +// it still has to read 2 different files and acquire a read lock on the key file: // - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.crt // - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.key func LoadKeysToKubeFromStore(profile *profile.Profile, dirPath, teleportCluster, kubeCluster string) ([]byte, []byte, error) { fsKeyStore := NewFSKeyStore(dirPath) + keyPath := fsKeyStore.kubeKeyPath(KeyRingIndex{ProxyHost: profile.SiteName, ClusterName: teleportCluster, Username: profile.Username}, kubeCluster) certPath := fsKeyStore.kubeCertPath(KeyRingIndex{ProxyHost: profile.SiteName, ClusterName: teleportCluster, Username: profile.Username}, kubeCluster) - kubeCert, err := os.ReadFile(certPath) + keyPEM, certPEM, err := readTLSCredentialFiles(keyPath, certPath) if err != nil { return nil, nil, trace.Wrap(err) } - privKeyPath := fsKeyStore.kubeKeyPath(KeyRingIndex{ProxyHost: profile.SiteName, ClusterName: teleportCluster, Username: profile.Username}, kubeCluster) - privKey, err := os.ReadFile(privKeyPath) - if err != nil { - return nil, nil, trace.Wrap(err) - } - - if err := keys.AssertSoftwarePrivateKey(privKey); err != nil { + if err := keys.AssertSoftwarePrivateKey(keyPEM); err != nil { return nil, nil, trace.Wrap(err, "unsupported private key type") } - return kubeCert, privKey, nil + return certPEM, keyPEM, nil } diff --git a/lib/client/client_store_test.go b/lib/client/client_store_test.go index cd9de2f65d354..199b0658cbcee 100644 --- a/lib/client/client_store_test.go +++ b/lib/client/client_store_test.go @@ -20,7 +20,13 @@ package client import ( "context" + "crypto/rand" + "crypto/x509" "crypto/x509/pkix" + "encoding/pem" + "fmt" + "math/big" + "sync" "sync/atomic" "testing" "time" @@ -36,6 +42,7 @@ import ( apisshutils "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/auth/testauthority" + "github.com/gravitational/teleport/lib/cryptosuites" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/sshutils" @@ -360,6 +367,103 @@ func TestProxySSHConfig(t *testing.T) { }) } +// BenchmarkLoadKeysToKubeFromStore benchmarks the namesake function used in the +// `tsh kube credentials` command called by kubectl. It should be reasonably +// fast to avoid adding latency to all kubectl calls. It should tolerate being +// called many times in parallel. +func BenchmarkLoadKeysToKubeFromStore(b *testing.B) { + key, err := cryptosuites.GenerateKeyWithAlgorithm(cryptosuites.ECDSAP256) + require.NoError(b, err) + + template := x509.Certificate{ + Subject: pkix.Name{ + CommonName: "k8scluster", + }, + SerialNumber: big.NewInt(1), + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + } + certDER, err := x509.CreateCertificate(rand.Reader, &template, &template, key.Public(), key) + require.NoError(b, err) + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}) + require.NotEmpty(b, certPEM) + + keyPEM, err := keys.MarshalPrivateKey(key) + require.NoError(b, err) + privateKey, err := keys.NewPrivateKey(key, keyPEM) + require.NoError(b, err) + + kubeCred := TLSCredential{ + PrivateKey: privateKey, + Cert: certPEM, + } + + dir := b.TempDir() + fsKeyStore := NewFSKeyStore(dir) + + keyRing := &KeyRing{ + KeyRingIndex: KeyRingIndex{ + ProxyHost: "teleport.example.com", + Username: "tester", + ClusterName: "teleportcluster", + }, + PrivateKey: privateKey, + TLSCert: certPEM, + KubeTLSCredentials: make(map[string]TLSCredential, 10), + } + + kubeClusterNames := make([]string, 0, 10) + for i := 0; i < 10; i++ { + kubeClusterName := fmt.Sprintf("kubecluster-%d", i) + keyRing.KubeTLSCredentials[kubeClusterName] = kubeCred + kubeClusterNames = append(kubeClusterNames, kubeClusterName) + } + + err = fsKeyStore.AddKeyRing(keyRing) + require.NoError(b, err) + + b.Run("LoadKeysToKubeFromStore", func(b *testing.B) { + for i := 0; i < b.N; i++ { + var wg sync.WaitGroup + wg.Add(len(kubeClusterNames)) + for _, kubeClusterName := range kubeClusterNames { + kubeClusterName = kubeClusterName + go func() { + defer wg.Done() + certPEM, keyPEM, err := LoadKeysToKubeFromStore(&profile.Profile{ + SiteName: "teleport.example.com", + Username: "tester", + }, dir, "teleportcluster", kubeClusterName) + require.NoError(b, err) + require.NotEmpty(b, certPEM) + require.NotEmpty(b, keyPEM) + }() + } + wg.Wait() + } + }) + + // Compare against a naive GetKeyRing call which loads the key and cert for + // all active kube clusters, not just the one requested. + b.Run("GetKeyRing", func(b *testing.B) { + for i := 0; i < b.N; i++ { + var wg sync.WaitGroup + wg.Add(len(kubeClusterNames)) + for _, kubeClusterName := range kubeClusterNames { + go func() { + defer wg.Done() + keyRing, err := fsKeyStore.GetKeyRing(keyRing.KeyRingIndex, WithKubeCerts{}) + require.NoError(b, err) + require.NotNil(b, keyRing.KubeTLSCredentials[kubeClusterName].PrivateKey) + require.NotEmpty(b, keyRing.KubeTLSCredentials[kubeClusterName].Cert) + }() + } + wg.Wait() + } + }) +} + var ( CAPriv = []byte(`-----BEGIN RSA PRIVATE KEY----- MIIEowIBAAKCAQEAwBgwn+vkjCcKEr2fbX1mLN555B9amVYfD/fUZBNbXKpHaqYn diff --git a/lib/client/keystore.go b/lib/client/keystore.go index 9d266ab5386cd..4bf73d9678cfe 100644 --- a/lib/client/keystore.go +++ b/lib/client/keystore.go @@ -19,11 +19,13 @@ package client import ( + "context" "errors" iofs "io/fs" "os" "path/filepath" "runtime" + "time" "github.com/gravitational/trace" "github.com/sirupsen/logrus" @@ -176,19 +178,19 @@ func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { return trace.Wrap(err) } - if err := fs.writeBytes(keyRing.PrivateKey.PrivateKeyPEM(), fs.userKeyPath(keyRing.KeyRingIndex)); err != nil { + // Store TLS key and cert. + if err := fs.writeTLSCredential(TLSCredential{ + PrivateKey: keyRing.PrivateKey, + Cert: keyRing.TLSCert, + }, fs.userKeyPath(keyRing.KeyRingIndex), fs.tlsCertPath(keyRing.KeyRingIndex)); err != nil { return trace.Wrap(err) } + // Store SSH public key (it currently matches the same private key as the TLS cert). if err := fs.writeBytes(keyRing.PrivateKey.MarshalSSHPublicKey(), fs.publicKeyPath(keyRing.KeyRingIndex)); err != nil { return trace.Wrap(err) } - // Store TLS cert - if err := fs.writeBytes(keyRing.TLSCert, fs.tlsCertPath(keyRing.KeyRingIndex)); err != nil { - return trace.Wrap(err) - } - // We only generate PPK files for use by PuTTY when running tsh on Windows. if runtime.GOOS == constants.WindowsOS { ppkFile, err := keyRing.PrivateKey.PPKFile() @@ -203,7 +205,7 @@ func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { } } - // Store per-cluster key data. + // Store per-cluster SSH cert. if len(keyRing.Cert) > 0 { if err := fs.writeBytes(keyRing.Cert, fs.sshCertPath(keyRing.KeyRingIndex)); err != nil { return trace.Wrap(err) @@ -218,34 +220,25 @@ func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { // don't expect any well-meaning user to create bad names. kubeCluster = filepath.Clean(kubeCluster) - certPath := fs.kubeCertPath(keyRing.KeyRingIndex, kubeCluster) - if err := fs.writeBytes(cred.Cert, certPath); err != nil { - return trace.Wrap(err) - } keyPath := fs.kubeKeyPath(keyRing.KeyRingIndex, kubeCluster) - if err := fs.writeBytes(cred.PrivateKey.PrivateKeyPEM(), keyPath); err != nil { + certPath := fs.kubeCertPath(keyRing.KeyRingIndex, kubeCluster) + if err := fs.writeTLSCredential(cred, keyPath, certPath); err != nil { return trace.Wrap(err) } } for db, cred := range keyRing.DBTLSCredentials { db = filepath.Clean(db) - certPath := fs.databaseCertPath(keyRing.KeyRingIndex, db) - if err := fs.writeBytes(cred.Cert, certPath); err != nil { - return trace.Wrap(err) - } keyPath := fs.databaseKeyPath(keyRing.KeyRingIndex, db) - if err := fs.writeBytes(cred.PrivateKey.PrivateKeyPEM(), keyPath); err != nil { + certPath := fs.databaseCertPath(keyRing.KeyRingIndex, db) + if err := fs.writeTLSCredential(cred, keyPath, certPath); err != nil { return trace.Wrap(err) } } for app, cred := range keyRing.AppTLSCredentials { app = filepath.Clean(app) - certPath := fs.appCertPath(keyRing.KeyRingIndex, app) - if err := fs.writeBytes(cred.Cert, certPath); err != nil { - return trace.Wrap(err) - } keyPath := fs.appKeyPath(keyRing.KeyRingIndex, app) - if err := fs.writeBytes(cred.PrivateKey.PrivateKeyPEM(), keyPath); err != nil { + certPath := fs.appCertPath(keyRing.KeyRingIndex, app) + if err := fs.writeTLSCredential(cred, keyPath, certPath); err != nil { return trace.Wrap(err) } } @@ -253,6 +246,98 @@ func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { return nil } +func (fs *FSKeyStore) writeTLSCredential(cred TLSCredential, keyPath, certPath string) error { + if err := os.MkdirAll(filepath.Dir(keyPath), os.ModeDir|profileDirPerms); err != nil { + return trace.ConvertSystemError(err) + } + + // Always lock the key file when reading or writing a TLS credential so + // that we can't end up with non-matching key/cert in a race condition. + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + unlock, err := tryWriteLockFile(ctx, keyPath) + if err != nil { + return trace.Wrap(err) + } + defer unlock() + + if err := fs.writeBytes(cred.PrivateKey.PrivateKeyPEM(), keyPath); err != nil { + return trace.Wrap(err) + } + if err := fs.writeBytes(cred.Cert, certPath); err != nil { + return trace.Wrap(err) + } + return nil +} + +func readTLSCredential(keyPath, certPath string) (TLSCredential, error) { + keyPEM, certPEM, err := readTLSCredentialFiles(keyPath, certPath) + if err != nil { + return TLSCredential{}, trace.Wrap(err) + } + key, err := keys.ParsePrivateKey(keyPEM) + if err != nil { + return TLSCredential{}, trace.Wrap(err) + } + return TLSCredential{ + PrivateKey: key, + Cert: certPEM, + }, nil +} + +func readTLSCredentialFiles(keyPath, certPath string) ([]byte, []byte, error) { + // Always lock the key file when reading or writing a TLS credential so + // that we can't end up with non-matching key/cert in a race condition. + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + unlock, err := tryReadLockFile(ctx, keyPath) + if err != nil { + return nil, nil, trace.Wrap(err) + } + defer unlock() + + keyPEM, err := os.ReadFile(keyPath) + if err != nil { + return nil, nil, trace.ConvertSystemError(err) + } + certPEM, err := os.ReadFile(certPath) + if err != nil { + return nil, nil, trace.ConvertSystemError(err) + } + return keyPEM, certPEM, nil +} + +func tryWriteLockFile(ctx context.Context, path string) (func(), error) { + return tryLockFile(ctx, path, utils.FSTryWriteLock) +} + +func tryReadLockFile(ctx context.Context, path string) (func(), error) { + return tryLockFile(ctx, path, utils.FSTryReadLock) +} + +func tryLockFile(ctx context.Context, path string, lockFn func(string) (func() error, error)) (func(), error) { + unlock, err := lockFn(path) + for { + switch { + case err == nil: + return func() { + if err := unlock(); err != nil { + log.Errorf("failed to unlock TLS credential at %s: %s", path, err) + } + }, nil + case errors.Is(err, utils.ErrUnsuccessfulLockTry): + select { + case <-ctx.Done(): + return nil, trace.Wrap(ctx.Err(), "timed out trying to acquire lock for TLS credential at %s", path) + case <-time.After(50 * time.Millisecond): + } + unlock, err = lockFn(path) + default: + return nil, trace.Wrap(err, "could not acquire lock for TLS credential at %s", path) + } + } +} + func (fs *FSKeyStore) writeBytes(bytes []byte, fp string) error { if err := os.MkdirAll(filepath.Dir(fp), os.ModeDir|profileDirPerms); err != nil { return trace.ConvertSystemError(err) @@ -410,43 +495,27 @@ func (fs *FSKeyStore) GetSSHCertificates(proxyHost, username string) ([]*ssh.Cer } func getCredentialsByName(credentialDir string) (map[string]TLSCredential, error) { - credsByName := make(map[string]TLSCredential) files, err := os.ReadDir(credentialDir) if err != nil { return nil, trace.ConvertSystemError(err) } + credsByName := make(map[string]TLSCredential, len(files)/2) for _, file := range files { - if certName := keypaths.TrimCertPathSuffix(file.Name()); certName != file.Name() { - data, err := os.ReadFile(filepath.Join(credentialDir, file.Name())) - if err != nil { - return nil, trace.ConvertSystemError(err) - } - cred := credsByName[certName] - cred.Cert = data - credsByName[certName] = cred - } if keyName := keypaths.TrimKeyPathSuffix(file.Name()); keyName != file.Name() { - data, err := os.ReadFile(filepath.Join(credentialDir, file.Name())) - if err != nil { - return nil, trace.ConvertSystemError(err) - } - privKey, err := keys.ParsePrivateKey(data) + keyPath := filepath.Join(credentialDir, file.Name()) + certPath := filepath.Join(credentialDir, keyName+keypaths.FileExtTLSCert) + cred, err := readTLSCredential(keyPath, certPath) if err != nil { + if trace.IsNotFound(err) { + // Somehow we have a key with no cert, skip it. This should + // cause a cert re-issue which should solve the problem. + continue + } return nil, trace.Wrap(err) } - cred := credsByName[keyName] - cred.PrivateKey = privKey credsByName[keyName] = cred } } - for name, creds := range credsByName { - if creds.Cert == nil || creds.PrivateKey == nil { - // Found a cert with no key or vice-versa, this may have been - // written by an older version or partially deleted, treat it as - // missing and a cert re-issue should solve it. - delete(credsByName, name) - } - } return credsByName, nil } diff --git a/lib/client/kube/kube.go b/lib/client/kube/kube.go index ff70bdb8fc375..dd26866d0938a 100644 --- a/lib/client/kube/kube.go +++ b/lib/client/kube/kube.go @@ -43,7 +43,7 @@ func CheckIfCertsAreAllowedToAccessCluster(k *client.KeyRing, rootCluster, telep return nil } if cred, ok := k.KubeTLSCredentials[kubeCluster]; ok { - log.Debugf("Got TLS cert for Kubernetes cluster %q", k8sCluster) + log.Debugf("Got TLS cert for Kubernetes cluster %q", kubeCluster) exist, err := checkIfCertHasKubeGroupsAndUsers(cred.Cert) if err != nil { return trace.Wrap(err) diff --git a/tool/tsh/common/kube.go b/tool/tsh/common/kube.go index 987ae3b01f20b..d36f4f9b553a8 100644 --- a/tool/tsh/common/kube.go +++ b/tool/tsh/common/kube.go @@ -631,7 +631,8 @@ func (c *kubeCredentialsCommand) run(cf *CLIConf) error { // This operation takes a long time when the store has a lot of keys and when // we call the function multiple times in parallel. // Although client.LoadKeysToKubeFromStore function speeds up the process since - // it removes all transversals, it still has to read 2 different files from the disk: + // it removes all transversals, it still has to read 2 different files from + // the disk and acquire a read lock on the key file: // - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.crt // - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.key // From 4eaafb2abce398624d0166ecb3a5d2d7d545c93b Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Fri, 9 Aug 2024 19:00:34 -0700 Subject: [PATCH 4/6] reuse RSA keys --- api/utils/keys/privatekey.go | 1 - api/utils/keys/privatekey_test.go | 4 +++- lib/client/client_store_test.go | 11 +++++++---- lib/client/cluster_client.go | 4 ++-- lib/client/cluster_client_test.go | 15 ++++++++------- lib/client/interfaces.go | 17 ++++++++++++----- 6 files changed, 32 insertions(+), 20 deletions(-) diff --git a/api/utils/keys/privatekey.go b/api/utils/keys/privatekey.go index 187ff002058b2..acf71c41609e2 100644 --- a/api/utils/keys/privatekey.go +++ b/api/utils/keys/privatekey.go @@ -110,7 +110,6 @@ func TLSCertificateForSigner(signer crypto.Signer, certPEMBlock []byte) (tls.Cer return tls.Certificate{}, trace.Wrap(err) } cert.Certificate = rawCerts - cert.Leaf = x509Cert // Check that the certificate's public key matches this private key. if keyPub, ok := signer.Public().(cryptoPublicKeyI); !ok { diff --git a/api/utils/keys/privatekey_test.go b/api/utils/keys/privatekey_test.go index ce84ff8141580..41b52b169c739 100644 --- a/api/utils/keys/privatekey_test.go +++ b/api/utils/keys/privatekey_test.go @@ -30,6 +30,8 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/require" ) @@ -87,7 +89,7 @@ func TestX509KeyPair(t *testing.T) { tlsCert, err := X509KeyPair(tc.certPEM, tc.keyPEM) require.NoError(t, err) - require.Equal(t, expectCert, tlsCert) + require.Empty(t, cmp.Diff(expectCert, tlsCert, cmpopts.IgnoreFields(tls.Certificate{}, "Leaf"))) }) } } diff --git a/lib/client/client_store_test.go b/lib/client/client_store_test.go index 199b0658cbcee..4c03418d60fb5 100644 --- a/lib/client/client_store_test.go +++ b/lib/client/client_store_test.go @@ -69,7 +69,11 @@ func newTestAuthority(t *testing.T) testAuthority { // makeSignedKeyRing helper returns a new user key ring signed by CAPriv key. func (s *testAuthority) makeSignedKeyRing(t *testing.T, idx KeyRingIndex, makeExpired bool) *KeyRing { - priv, err := s.keygen.GeneratePrivateKey() + signer, err := cryptosuites.GenerateKeyWithAlgorithm(cryptosuites.ECDSAP256) + require.NoError(t, err) + keyPEM, err := keys.MarshalPrivateKey(signer) + require.NoError(t, err) + priv, err := keys.NewPrivateKey(signer, keyPEM) require.NoError(t, err) allowedLogins := []string{idx.Username, "root"} @@ -78,8 +82,8 @@ func (s *testAuthority) makeSignedKeyRing(t *testing.T, idx KeyRingIndex, makeEx ttl = -ttl } - // reuse the same RSA keys for SSH and TLS keys - // TODO(nklaassen): don't + // reuse the same keys for SSH and TLS keys + // TODO(nklaassen): don't reuse these keys. clock := clockwork.NewRealClock() identity := tlsca.Identity{ Username: idx.Username, @@ -428,7 +432,6 @@ func BenchmarkLoadKeysToKubeFromStore(b *testing.B) { var wg sync.WaitGroup wg.Add(len(kubeClusterNames)) for _, kubeClusterName := range kubeClusterNames { - kubeClusterName = kubeClusterName go func() { defer wg.Done() certPEM, keyPEM, err := LoadKeysToKubeFromStore(&profile.Profile{ diff --git a/lib/client/cluster_client.go b/lib/client/cluster_client.go index 5e7da5178dac2..80958b88539e4 100644 --- a/lib/client/cluster_client.go +++ b/lib/client/cluster_client.go @@ -357,7 +357,7 @@ func (c *ClusterClient) prepareUserCertsRequest(ctx context.Context, params Reis var privateKey *keys.PrivateKey switch params.usage() { case proto.UserCertsRequest_App, proto.UserCertsRequest_Kubernetes: - privateKey, err = keyRing.GenerateKey(ctx, c.tc, cryptosuites.UserTLS) + privateKey, err = keyRing.generateSubjectTLSKey(ctx, c.tc, cryptosuites.UserTLS) if err != nil { return nil, nil, trace.Wrap(err) } @@ -366,7 +366,7 @@ func (c *ClusterClient) prepareUserCertsRequest(ctx context.Context, params Reis return nil, nil, trace.Wrap(err) } case proto.UserCertsRequest_Database: - privateKey, err = keyRing.GenerateKey(ctx, c.tc, cryptosuites.UserDatabase) + privateKey, err = keyRing.generateSubjectTLSKey(ctx, c.tc, cryptosuites.UserDatabase) if err != nil { return nil, nil, trace.Wrap(err) } diff --git a/lib/client/cluster_client_test.go b/lib/client/cluster_client_test.go index 1079cd0d97cf0..3defeb2844029 100644 --- a/lib/client/cluster_client_test.go +++ b/lib/client/cluster_client_test.go @@ -18,7 +18,8 @@ package client import ( "context" - "crypto/x509" + "crypto/ecdsa" + "crypto/rsa" "errors" "testing" @@ -189,9 +190,9 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) cred := keyRing.KubeTLSCredentials["test"] require.NotEmpty(t, cred) - cert, err := cred.TLSCertificate() + _, err = cred.TLSCertificate() require.NoError(t, err) - require.Equal(t, x509.ECDSA, cert.Leaf.PublicKeyAlgorithm) + require.IsType(t, (*ecdsa.PrivateKey)(nil), cred.PrivateKey.Signer) }, }, { @@ -205,9 +206,9 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) cred := keyRing.KubeTLSCredentials["test"] require.NotEmpty(t, cred) - cert, err := cred.TLSCertificate() + _, err = cred.TLSCertificate() require.NoError(t, err) - require.Equal(t, x509.RSA, cert.Leaf.PublicKeyAlgorithm) + require.IsType(t, (*rsa.PrivateKey)(nil), cred.PrivateKey.Signer) }, }, { @@ -253,9 +254,9 @@ func TestIssueUserCertsWithMFA(t *testing.T) { require.Equal(t, proto.MFARequired_MFA_REQUIRED_YES, mfaRequired) cred := keyRing.DBTLSCredentials["test"] require.NotEmpty(t, cred) - cert, err := cred.TLSCertificate() + _, err = cred.TLSCertificate() require.NoError(t, err) - require.Equal(t, x509.RSA, cert.Leaf.PublicKeyAlgorithm) + require.IsType(t, (*rsa.PrivateKey)(nil), cred.PrivateKey.Signer) }, }, { diff --git a/lib/client/interfaces.go b/lib/client/interfaces.go index 80bf653072216..6aa29f1ec6c18 100644 --- a/lib/client/interfaces.go +++ b/lib/client/interfaces.go @@ -135,12 +135,19 @@ func (k *KeyRing) Copy() *KeyRing { return © } -// GenerateKey returns a new private key with the appropriate algorithm for -// [purpose]. If this KeyRing uses a PIV/hardware key, it will always return -// that hardware key. -func (k *KeyRing) GenerateKey(ctx context.Context, tc *TeleportClient, purpose cryptosuites.KeyPurpose) (*keys.PrivateKey, error) { +// generateSubjectTLSKey returns a new private key with the appropriate algorithm for +// [purpose], meant to be used as the subject key for a new user cert request. +// If [k.PrivateKey] is a PIV/hardware key or an RSA key, it will be re-used. +func (k *KeyRing) generateSubjectTLSKey(ctx context.Context, tc *TeleportClient, purpose cryptosuites.KeyPurpose) (*keys.PrivateKey, error) { if k.PrivateKey.IsHardware() { - // We always use the same hardware key. + // We always re-use the root TLS key if it is a hardware key. + return k.PrivateKey, nil + } + if _, isRSA := k.PrivateKey.Public().(*rsa.PublicKey); isRSA { + // We always re-use the root TLS key if it is RSA (it would be expensive + // to always generate new RSA keys). If [k.PrivateKey] is RSA we must be + // using the `legacy` signature algorithm suitei and the subject keys + // should be RSA as well. return k.PrivateKey, nil } From ad0da39eb1035b752d3ffab6cc303234eb5f7a33 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 12 Aug 2024 23:14:55 -0700 Subject: [PATCH 5/6] single file for kube key and cert --- api/utils/keypaths/keypaths.go | 21 +++--- api/utils/keypaths/keypaths_test.go | 2 +- lib/client/client_store.go | 17 ++--- lib/client/client_store_test.go | 2 +- lib/client/interfaces.go | 4 +- lib/client/keystore.go | 86 +++++++++++++++++++++---- rfd/0136-modern-signature-algorithms.md | 28 ++++++-- tool/tsh/common/kube.go | 6 +- 8 files changed, 119 insertions(+), 47 deletions(-) diff --git a/api/utils/keypaths/keypaths.go b/api/utils/keypaths/keypaths.go index 34500f23ef358..84321aa589b37 100644 --- a/api/utils/keypaths/keypaths.go +++ b/api/utils/keypaths/keypaths.go @@ -36,6 +36,9 @@ const ( fileExtTLSCertLegacy = "-x509.pem" // FileExtTLSCert is the suffix/extension of a file where a TLS cert is stored. FileExtTLSCert = ".crt" + // FileExtKubeCred is the suffix/extension of a file where a kubernetes + // credential is stored (TLS key and cert combined in a single file). + FileExtKubeCred = ".cred" // fileExtTLSKey is the suffix/extension of a file where a TLS private key is stored. fileExtTLSKey = ".key" // fileNameTLSCerts is a file where TLS Cert Authorities are stored. @@ -327,20 +330,12 @@ func KubeCredentialDir(baseDir, proxy, username, cluster string) string { return filepath.Join(KubeDir(baseDir, proxy, username), cluster) } -// KubeCertPath returns the path to the user's TLS certificate -// for the given proxy, cluster, and kube cluster. -// -// /keys//-kube//.crt -func KubeCertPath(baseDir, proxy, username, cluster, kubename string) string { - return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+FileExtTLSCert) -} - -// KubeKeyPath returns the path to the user's TLS private key -// for the given proxy, cluster, and kube cluster. +// KubeCredPath returns the path to the user's TLS credential for the given +// proxy, cluster, and kube cluster. // -// /keys//-kube//.key -func KubeKeyPath(baseDir, proxy, username, cluster, kubename string) string { - return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+fileExtTLSKey) +// /keys//-kube//.cred +func KubeCredPath(baseDir, proxy, username, cluster, kubename string) string { + return filepath.Join(KubeCredentialDir(baseDir, proxy, username, cluster), kubename+FileExtKubeCred) } // KubeConfigPath returns the path to the user's standalone kubeconfig diff --git a/api/utils/keypaths/keypaths_test.go b/api/utils/keypaths/keypaths_test.go index 89f3f3c7bc950..0dcff1de898b0 100644 --- a/api/utils/keypaths/keypaths_test.go +++ b/api/utils/keypaths/keypaths_test.go @@ -31,7 +31,7 @@ func TestIsProfileKubeConfigPath(t *testing.T) { require.NoError(t, err) require.False(t, isKubeConfig) - path = keypaths.KubeCertPath("~/tsh", "proxy", "user", "cluster", "kube") + path = keypaths.KubeCredPath("~/tsh", "proxy", "user", "cluster", "kube") isKubeConfig, err = keypaths.IsProfileKubeConfigPath(path) require.NoError(t, err) require.False(t, isKubeConfig) diff --git a/lib/client/client_store.go b/lib/client/client_store.go index cd1031afba563..4d65be16d0dc8 100644 --- a/lib/client/client_store.go +++ b/lib/client/client_store.go @@ -257,22 +257,19 @@ func (s *Store) FullProfileStatus() (*ProfileStatus, []*ProfileStatus, error) { // Store transverses the entire store to find the keys. This operation takes a long time // when the store has a lot of keys and when we call the function multiple times in // parallel. -// Although this function speeds up the process since it removes all transversals, -// it still has to read 2 different files and acquire a read lock on the key file: -// - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.crt -// - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.key -func LoadKeysToKubeFromStore(profile *profile.Profile, dirPath, teleportCluster, kubeCluster string) ([]byte, []byte, error) { +// This function speeds up the process since it removes all transversals, and +// only reads 1 file: +// - $TSH_HOME/keys/$PROXY/$USER-kube/$TELEPORT_CLUSTER/$KUBE_CLUSTER.cred +func LoadKeysToKubeFromStore(profile *profile.Profile, dirPath, teleportCluster, kubeCluster string) (keyPEM, certPEM []byte, err error) { fsKeyStore := NewFSKeyStore(dirPath) - keyPath := fsKeyStore.kubeKeyPath(KeyRingIndex{ProxyHost: profile.SiteName, ClusterName: teleportCluster, Username: profile.Username}, kubeCluster) - certPath := fsKeyStore.kubeCertPath(KeyRingIndex{ProxyHost: profile.SiteName, ClusterName: teleportCluster, Username: profile.Username}, kubeCluster) - keyPEM, certPEM, err := readTLSCredentialFiles(keyPath, certPath) + credPath := fsKeyStore.kubeCredPath(KeyRingIndex{ProxyHost: profile.SiteName, ClusterName: teleportCluster, Username: profile.Username}, kubeCluster) + keyPEM, certPEM, err = readKubeCredentialFile(credPath) if err != nil { return nil, nil, trace.Wrap(err) } - if err := keys.AssertSoftwarePrivateKey(keyPEM); err != nil { return nil, nil, trace.Wrap(err, "unsupported private key type") } - return certPEM, keyPEM, nil + return keyPEM, certPEM, nil } diff --git a/lib/client/client_store_test.go b/lib/client/client_store_test.go index 4c03418d60fb5..3d8861d9a6f0b 100644 --- a/lib/client/client_store_test.go +++ b/lib/client/client_store_test.go @@ -434,7 +434,7 @@ func BenchmarkLoadKeysToKubeFromStore(b *testing.B) { for _, kubeClusterName := range kubeClusterNames { go func() { defer wg.Done() - certPEM, keyPEM, err := LoadKeysToKubeFromStore(&profile.Profile{ + keyPEM, certPEM, err := LoadKeysToKubeFromStore(&profile.Profile{ SiteName: "teleport.example.com", Username: "tester", }, dir, "teleportcluster", kubeClusterName) diff --git a/lib/client/interfaces.go b/lib/client/interfaces.go index 6aa29f1ec6c18..c9498afbea12f 100644 --- a/lib/client/interfaces.go +++ b/lib/client/interfaces.go @@ -84,8 +84,10 @@ func (idx KeyRingIndex) Match(matchKeyRing KeyRingIndex) bool { // TLSCredential holds a signed TLS certificate and matching private key. type TLSCredential struct { + // PrivateKey is the private key of the credential. PrivateKey *keys.PrivateKey - Cert []byte + // Cert is a PEM-encoded signed X509 certificate. + Cert []byte } // TLSCertificate returns a valid [tls.Certificate] ready to be used in a TLS diff --git a/lib/client/keystore.go b/lib/client/keystore.go index 4bf73d9678cfe..ba1d1eaeda315 100644 --- a/lib/client/keystore.go +++ b/lib/client/keystore.go @@ -19,12 +19,14 @@ package client import ( + "bytes" "context" "errors" iofs "io/fs" "os" "path/filepath" "runtime" + "strings" "time" "github.com/gravitational/trace" @@ -162,14 +164,9 @@ func (fs *FSKeyStore) databaseKeyPath(idx KeyRingIndex, dbname string) string { return keypaths.DatabaseKeyPath(fs.KeyDir, idx.ProxyHost, idx.Username, idx.ClusterName, dbname) } -// kubeCertPath returns the TLS certificate path for the given KeyRingIndex and kube cluster name. -func (fs *FSKeyStore) kubeCertPath(idx KeyRingIndex, kubename string) string { - return keypaths.KubeCertPath(fs.KeyDir, idx.ProxyHost, idx.Username, idx.ClusterName, kubename) -} - -// kubeKeyPath returns the private key path for the given KeyRingIndex and kube cluster name. -func (fs *FSKeyStore) kubeKeyPath(idx KeyRingIndex, kubename string) string { - return keypaths.KubeKeyPath(fs.KeyDir, idx.ProxyHost, idx.Username, idx.ClusterName, kubename) +// kubeCredPath returns the TLS credential path for the given KeyRingIndex and kube cluster name. +func (fs *FSKeyStore) kubeCredPath(idx KeyRingIndex, kubename string) string { + return keypaths.KubeCredPath(fs.KeyDir, idx.ProxyHost, idx.Username, idx.ClusterName, kubename) } // AddKeyRing adds the given key ring to the store. @@ -220,9 +217,8 @@ func (fs *FSKeyStore) AddKeyRing(keyRing *KeyRing) error { // don't expect any well-meaning user to create bad names. kubeCluster = filepath.Clean(kubeCluster) - keyPath := fs.kubeKeyPath(keyRing.KeyRingIndex, kubeCluster) - certPath := fs.kubeCertPath(keyRing.KeyRingIndex, kubeCluster) - if err := fs.writeTLSCredential(cred, keyPath, certPath); err != nil { + credPath := fs.kubeCredPath(keyRing.KeyRingIndex, kubeCluster) + if err := fs.writeKubeCredential(cred, credPath); err != nil { return trace.Wrap(err) } } @@ -338,6 +334,53 @@ func tryLockFile(ctx context.Context, path string, lockFn func(string) (func() e } } +// writeKubeCredential writes a kube key and cert to a single file, both +// PEM-encoded, with exactly 2 newlines between them. Compared to using separate files +// it is more efficient for reading/writing and avoids file locks. +func (fs *FSKeyStore) writeKubeCredential(cred TLSCredential, path string) error { + credFileContents := bytes.Join([][]byte{ + bytes.TrimRight(cred.PrivateKey.PrivateKeyPEM(), "\n"), + bytes.TrimLeft(cred.Cert, "\n"), + }, []byte("\n\n")) + return trace.Wrap(fs.writeBytes(credFileContents, path)) +} + +// readKubeCredential reads a kube key and cert from a single file written by +// [(*FSKeyStore).writeKubeCredential]. Compared to using separate files it is +// more efficient for reading/writing and avoids file locks. +func readKubeCredential(path string) (TLSCredential, error) { + keyPEM, certPEM, err := readKubeCredentialFile(path) + if err != nil { + return TLSCredential{}, trace.Wrap(err) + } + privateKey, err := keys.ParsePrivateKey(keyPEM) + if err != nil { + return TLSCredential{}, trace.Wrap(err) + } + return TLSCredential{ + PrivateKey: privateKey, + Cert: certPEM, + }, nil +} + +// readKubeCredentialFile reads kube key and cert PEM blocks from a single +// file written by [(*FSKeyStore).writeKubeCredential]. Compared to using +// separate files it is more efficient for reading/writing and avoids file +// locks. +func readKubeCredentialFile(path string) (keyPEM, certPEM []byte, err error) { + credFileContents, err := os.ReadFile(path) + if err != nil { + return nil, nil, trace.ConvertSystemError(err) + } + + pems := bytes.Split(credFileContents, []byte("\n\n")) + if len(pems) != 2 { + return nil, nil, trace.BadParameter("expect key and cert PEM blocks separated by two newlines") + } + keyPEM, certPEM = pems[0], pems[1] + return keyPEM, certPEM, nil +} + func (fs *FSKeyStore) writeBytes(bytes []byte, fp string) error { if err := os.MkdirAll(filepath.Dir(fp), os.ModeDir|profileDirPerms); err != nil { return trace.ConvertSystemError(err) @@ -519,6 +562,25 @@ func getCredentialsByName(credentialDir string) (map[string]TLSCredential, error return credsByName, nil } +func getKubeCredentialsByName(credentialDir string) (map[string]TLSCredential, error) { + files, err := os.ReadDir(credentialDir) + if err != nil { + return nil, trace.ConvertSystemError(err) + } + credsByName := make(map[string]TLSCredential, len(files)) + for _, file := range files { + if credName := strings.TrimSuffix(file.Name(), keypaths.FileExtKubeCred); credName != file.Name() { + credPath := filepath.Join(credentialDir, file.Name()) + cred, err := readKubeCredential(credPath) + if err != nil { + return nil, trace.Wrap(err) + } + credsByName[credName] = cred + } + } + return credsByName, nil +} + // CertOption is an additional step to run when loading/deleting user certificates. type CertOption interface { // updateKeyRing is used by [FSKeyStore] to add the relevant credentials @@ -563,7 +625,7 @@ type WithKubeCerts struct{} func (o WithKubeCerts) updateKeyRing(keyDir string, idx KeyRingIndex, keyRing *KeyRing) error { credentialDir := keypaths.KubeCredentialDir(keyDir, idx.ProxyHost, idx.Username, idx.ClusterName) - credsByName, err := getCredentialsByName(credentialDir) + credsByName, err := getKubeCredentialsByName(credentialDir) if err != nil { return trace.Wrap(err) } diff --git a/rfd/0136-modern-signature-algorithms.md b/rfd/0136-modern-signature-algorithms.md index ee58739acff39..9075ef9b4c953 100644 --- a/rfd/0136-modern-signature-algorithms.md +++ b/rfd/0136-modern-signature-algorithms.md @@ -484,14 +484,12 @@ We will also have to change the disk layout of the ~/.tsh directory: │ | ├── root --> Kubernetes certs for Teleport cluster "root" │ | │ ├── kubeA-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeA" - │ | │ ├── kubeA-x509.pem --> TLS cert for Kubernetes cluster "kubeA" -+ │ | │ ├── kubeA.key --> TLS private key for Kubernetes cluster "kubeA" -+ │ | │ ├── kubeA.crt --> TLS cert for Kubernetes cluster "kubeA" ++ │ | │ ├── kubeA.cred --> TLS private key and certificate for Kubernetes cluster "kubeA" │ | │ └── localca.pem --> Self-signed localhost CA cert for Teleport cluster "root" │ | └── leaf --> Kubernetes certs for Teleport cluster "leaf" │ | ├── kubeC-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeC" - │ | ├── kubeC-x509.pem --> TLS cert for Kubernetes cluster "kubeC" -+ │ | ├── kubeC.key --> TLS cert for Kubernetes cluster "kubeC" -+ │ | └── kubeC.crt --> TLS cert for Kubernetes cluster "kubeC" ++ │ | └── kubeC.cred --> TLS private key and cert for Kubernetes cluster "kubeC" | └── cas --> Trusted clusters certificates | ├── root.pem --> TLS CA for teleport cluster "root" | ├── leaf1.pem --> TLS CA for teleport cluster "leaf1" @@ -540,13 +538,31 @@ re-running the command so that it prints the correct updated paths. The main TLS private key used to authenticate to cluster APIs will now be held in `~/.tsh/keys/one.example.com/foo.key`. -All TLS x509 cert files will be renamed from `-x509.pem` to -`.crt`. +All TLS x509 cert files (except for k8s) will be renamed from `-x509.pem` +to `.crt`. This may seem like an unnecessary breaking change, but it has a benefit that any software trying to use the old `-x509.pem` along with the outdated private key location will fail to load both files, instead of successfully opening the cert but failing with some confusing error when the private key does not match. +Because app and db logins will now cause the private key AND cert to be +overwritten, we will use an OS file lock on the key file whenever reading or +writing the pair of files, to avoid a race condition that could cause key/cert +pair that don't match to be read or written. + +#### Kubernetes + +In the interest of keeping `tsh kube credentials` performant for cases where it +is called tens of times per second, instead of storing the key and cert in +separate files, they are both combined into a single file `.cred`. +The requires only a single file read, instead of 2 file reads plus an OS file +lock/unlock. + +We don't do this for app and db key/certs because those are often read by third +party clients. +For kubectl, we don't have to worry about this, because it always gets the +current credentials by calling `tsh kube credentials`. + ### HSMs and KMS Admins should configure the `hsm-v1` suite when using any HSM or KMS. diff --git a/tool/tsh/common/kube.go b/tool/tsh/common/kube.go index d36f4f9b553a8..9ba2606260cfe 100644 --- a/tool/tsh/common/kube.go +++ b/tool/tsh/common/kube.go @@ -638,16 +638,16 @@ func (c *kubeCredentialsCommand) run(cf *CLIConf) error { // // In addition to these files, $TSH_HOME/$profile.yaml is also read from // cf.GetProfile call above. - if kubeCert, privKey, err := client.LoadKeysToKubeFromStore( + if keyPEM, certPEM, err := client.LoadKeysToKubeFromStore( profile, cf.HomePath, c.teleportCluster, c.kubeCluster, ); err == nil { - crt, _ := tlsca.ParseCertificatePEM(kubeCert) + crt, _ := tlsca.ParseCertificatePEM(certPEM) if crt != nil && time.Until(crt.NotAfter) > time.Minute { log.Debugf("Re-using existing TLS cert for Kubernetes cluster %q", c.kubeCluster) - return c.writeByteResponse(cf.Stdout(), kubeCert, privKey, crt.NotAfter) + return c.writeByteResponse(cf.Stdout(), certPEM, keyPEM, crt.NotAfter) } } From 779f22e5d45522f020dcba7a2e177ac9e833b3d9 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Tue, 13 Aug 2024 08:55:04 -0700 Subject: [PATCH 6/6] update comment with file layout --- api/utils/keypaths/keypaths.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/api/utils/keypaths/keypaths.go b/api/utils/keypaths/keypaths.go index 84321aa589b37..3226a86ad512d 100644 --- a/api/utils/keypaths/keypaths.go +++ b/api/utils/keypaths/keypaths.go @@ -114,16 +114,13 @@ const ( // │ ├── foo-kube --> Kubernetes certs for user "foo" // │ | ├── root --> Kubernetes certs for Teleport cluster "root" // │ | │ ├── kubeA-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeA" -// │ | │ ├── kubeA.crt --> TLS cert for Kubernetes cluster "kubeA" -// │ | │ ├── kubeA.key --> private key for Kubernetes cluster "kubeA" +// │ | │ ├── kubeA.cred --> TLS private key and cert for Kubernetes cluster "kubeA" // │ | │ ├── kubeB-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeB" -// │ | │ ├── kubeB.crt --> TLS cert for Kubernetes cluster "kubeB" -// │ | │ ├── kubeB.key --> private key for Kubernetes cluster "kubeB" +// │ | │ ├── kubeB.cred --> TLS private key and cert for Kubernetes cluster "kubeB" // │ | │ └── localca.pem --> Self-signed localhost CA cert for Teleport cluster "root" // │ | └── leaf --> Kubernetes certs for Teleport cluster "leaf" // │ | ├── kubeC-kubeconfig --> standalone kubeconfig for Kubernetes cluster "kubeC" -// │ | └── kubeC.crt --> TLS cert for Kubernetes cluster "kubeC" -// │ | └── kubeC.key --> private key for Kubernetes cluster "kubeC" +// │ | └── kubeC.cred --> TLS private key and cert for Kubernetes cluster "kubeC" // | └── cas --> Trusted clusters certificates // | ├── root.pem --> TLS CA for teleport cluster "root" // | ├── leaf1.pem --> TLS CA for teleport cluster "leaf1"