From 458cf4e9855401073223c5021d880dc3dd490353 Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Tue, 23 Jan 2024 17:27:39 +0100 Subject: [PATCH 1/4] Use the standard GetConfigForClient in sshGRPCServer --- lib/service/service.go | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/lib/service/service.go b/lib/service/service.go index 0562ffb81dd97..f0708360de602 100644 --- a/lib/service/service.go +++ b/lib/service/service.go @@ -4256,19 +4256,7 @@ func (process *TeleportProcess) initProxyEndpoint(conn *Connector) error { tlscfg.InsecureSkipVerify = true tlscfg.ClientAuth = tls.RequireAnyClientCert } - tlscfg.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { - tlsClone := tlscfg.Clone() - - // Build the client CA pool containing the cluster's user CA in - // order to be able to validate certificates provided by users. - var err error - tlsClone.ClientCAs, _, err = auth.DefaultClientCertPool(accessPoint, clusterName) - if err != nil { - return nil, trace.Wrap(err) - } - - return tlsClone, nil - } + tlscfg.GetConfigForClient = auth.WithClusterCAs(tlscfg, accessPoint, clusterName, log) creds, err := auth.NewTransportCredentials(auth.TransportCredentialsConfig{ TransportCredentials: credentials.NewTLS(tlscfg), From 4cfb69e2c64b0576c89f07f23cec1c3517a6d39a Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Wed, 24 Jan 2024 19:33:24 +0100 Subject: [PATCH 2/4] Exercise JumpHost meaningfully in TestSSH --- tool/tsh/common/proxy_test.go | 85 +++++++++++++++++++++++++++++++---- 1 file changed, 77 insertions(+), 8 deletions(-) diff --git a/tool/tsh/common/proxy_test.go b/tool/tsh/common/proxy_test.go index 7eb1267cd515e..19223c30e19de 100644 --- a/tool/tsh/common/proxy_test.go +++ b/tool/tsh/common/proxy_test.go @@ -21,8 +21,13 @@ package common import ( "bytes" "context" + "crypto/ecdsa" + "crypto/elliptic" "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" "encoding/json" + "encoding/pem" "errors" "fmt" "net" @@ -32,9 +37,11 @@ import ( "path" "path/filepath" "strconv" + "sync" "testing" "text/template" "time" + _ "unsafe" "github.com/gravitational/trace" "github.com/stretchr/testify/assert" @@ -56,17 +63,57 @@ import ( "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/service/servicecfg" "github.com/gravitational/teleport/lib/teleagent" + "github.com/gravitational/teleport/lib/tlsca" ) +//go:linkname x509_systemRootsMu crypto/x509.systemRootsMu +var x509_systemRootsMu sync.RWMutex + +//go:linkname x509_systemRoots crypto/x509.systemRoots +var x509_systemRoots *x509.CertPool + +func setSystemRoots(pool *x509.CertPool) (reset func()) { + // ensure that systemRoots was already set, or the next call to + // x509.SystemCertPool() will overwrite it + _, _ = x509.SystemCertPool() + + x509_systemRootsMu.Lock() + previousSystemRoots := x509_systemRoots + x509_systemRoots = pool + x509_systemRootsMu.Unlock() + + return func() { + x509_systemRootsMu.Lock() + x509_systemRoots = previousSystemRoots + x509_systemRootsMu.Unlock() + } +} + // TestSSH verifies "tsh ssh" command. func TestSSH(t *testing.T) { - lib.SetInsecureDevMode(true) - defer lib.SetInsecureDevMode(false) + t.Setenv("_TELEPORT_TEST_NO_PARALLEL", "1") + defer lib.SetInsecureDevMode(lib.IsInsecureDevMode()) + lib.SetInsecureDevMode(false) + + d := t.TempDir() + webCertPath := path.Join(d, "cert.pem") + webKeyPath := path.Join(d, "key.pem") + webCertPEM := generateWebPKICA(t, webCertPath, webKeyPath) + + certPool := x509.NewCertPool() + require.True(t, certPool.AppendCertsFromPEM(webCertPEM)) + + reset := setSystemRoots(certPool) + defer reset() s := newTestSuite(t, withRootConfigFunc(func(cfg *servicecfg.Config) { cfg.Version = defaults.TeleportConfigVersionV2 cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Multiplex) + cfg.Proxy.KeyPairs = []servicecfg.KeyPairPath{{ + PrivateKey: webKeyPath, + Certificate: webCertPath, + }} }), withLeafCluster(), withLeafConfigFunc(func(cfg *servicecfg.Config) { @@ -74,6 +121,9 @@ func TestSSH(t *testing.T) { }), ) + // just in case someone changes newTestSuite + require.False(t, lib.IsInsecureDevMode()) + tests := []struct { name string fn func(t *testing.T, s *suite) @@ -102,7 +152,6 @@ func testRootClusterSSHAccess(t *testing.T, s *suite) { identityFile := mustLoginIdentity(t, s) err = Run(context.Background(), []string{ "--proxy", s.root.Config.Proxy.WebAddr.String(), - "--insecure", "-i", identityFile, "ssh", "localhost", @@ -127,7 +176,6 @@ func testLeafClusterSSHAccess(t *testing.T, s *suite) { identityFile := mustLoginIdentity(t, s) err := Run(context.Background(), []string{ "--proxy", s.root.Config.Proxy.WebAddr.String(), - "--insecure", "-i", identityFile, "ssh", "--cluster", s.leaf.Config.Auth.ClusterName.GetClusterName(), @@ -144,7 +192,6 @@ func testJumpHostSSHAccess(t *testing.T, s *suite) { // Switch to leaf cluster err := Run(context.Background(), []string{ "login", - "--insecure", s.leaf.Config.Auth.ClusterName.GetClusterName(), }, setMockSSOLogin(t, s), setHomePath(tshHome)) require.NoError(t, err) @@ -152,7 +199,6 @@ func testJumpHostSSHAccess(t *testing.T, s *suite) { // Connect to leaf node though jump host set to leaf proxy SSH port. err = Run(context.Background(), []string{ "ssh", - "--insecure", "-J", s.leaf.Config.Proxy.SSHAddr.Addr, s.leaf.Config.Hostname, "echo", "hello", @@ -163,7 +209,6 @@ func testJumpHostSSHAccess(t *testing.T, s *suite) { // Connect to leaf node though jump host set to proxy web port where TLS Routing is enabled. err = Run(context.Background(), []string{ "ssh", - "--insecure", "-J", s.leaf.Config.Proxy.WebAddr.Addr, s.leaf.Config.Hostname, "echo", "hello", @@ -179,7 +224,6 @@ func testJumpHostSSHAccess(t *testing.T, s *suite) { // Check JumpHost flow when root cluster is offline. err = Run(context.Background(), []string{ "ssh", - "--insecure", "-J", s.leaf.Config.Proxy.WebAddr.Addr, s.leaf.Config.Hostname, "echo", "hello", @@ -188,6 +232,31 @@ func testJumpHostSSHAccess(t *testing.T, s *suite) { }) } +func generateWebPKICA(t *testing.T, certPath, keyPath string) (certPEM []byte) { + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + keyDER, err := x509.MarshalPKCS8PrivateKey(key) + require.NoError(t, err) + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: keyDER}) + require.NotNil(t, keyPEM) + + certPEM, err = tlsca.GenerateSelfSignedCAWithConfig(tlsca.GenerateCAConfig{ + Signer: key, + Entity: pkix.Name{CommonName: "webpki ca"}, + TTL: time.Hour * 24 * 365, + // not sure why the CA needs to self-certify a specific address, but + // something in the crypto/tls verification checks for this when + // connecting to 127.0.0.1 + IPAddresses: []net.IP{net.IPv4(127, 0, 0, 1)}, + }) + require.NoError(t, err) + + require.NoError(t, os.WriteFile(certPath, certPEM, 0o644)) + require.NoError(t, os.WriteFile(keyPath, keyPEM, 0o644)) + + return certPEM +} + // TestWithRsync tests that Teleport works with rsync. func TestWithRsync(t *testing.T) { disableAgent(t) From 9b495db8edaaa7a48e4b8586ff3053c5aea0656f Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Thu, 25 Jan 2024 15:29:34 +0100 Subject: [PATCH 3/4] Clarify use of system roots override helper --- integration/helpers/x509.go | 71 +++++++++++++++++++++++++++++++++++ tool/tsh/common/proxy_test.go | 29 +------------- 2 files changed, 73 insertions(+), 27 deletions(-) create mode 100644 integration/helpers/x509.go diff --git a/integration/helpers/x509.go b/integration/helpers/x509.go new file mode 100644 index 0000000000000..a386daef96dcc --- /dev/null +++ b/integration/helpers/x509.go @@ -0,0 +1,71 @@ +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package helpers + +import ( + "crypto/x509" + "sync" + "testing" + _ "unsafe" +) + +//go:linkname x509_systemRootsMu crypto/x509.systemRootsMu +//go:linkname x509_systemRoots crypto/x509.systemRoots + +// These variables are used in x509 to cache the system cert pool after +// calculating it once; this is not a public interface and it's liable to even +// crash if the details of crypto/x509 are changed. An alternative to this would +// be to use x509.SetFallbackRoots and GODEBUG=x509usefallbackroots=1, but that +// can only be done once and requires a go:debug directive, so it will require +// the cooperation of the entire package's tests. +var ( + x509_systemRootsMu sync.RWMutex + x509_systemRoots *x509.CertPool +) + +// OverrideSystemRoots overrides the system certificate pool for the remainder +// of the test. The test must not be parallel, and OverrideSystemRoots will +// panic if called outside of a test binary. +func OverrideSystemRoots(t *testing.T, pool *x509.CertPool) { + if !testing.Testing() { + panic("SetSystemRoots was called outside of a test binary") + } + // panic if the test is parallel or when the test is made parallel + t.Setenv("_OverrideSystemRoots_called", "1") + + // ensure that systemRoots is already primed, or the next call to + // x509.SystemCertPool() will overwrite it + _, _ = x509.SystemCertPool() + + // the pool we store in systemRoots must not be changed + pool = pool.Clone() + + x509_systemRootsMu.Lock() + previousSystemRoots := x509_systemRoots + x509_systemRoots = pool + x509_systemRootsMu.Unlock() + + t.Cleanup(func() { + x509_systemRootsMu.Lock() + if x509_systemRoots != pool { + t.Log("x509.systemRoots was left modified after OverrideSystemRoots") + t.Fail() // continue execution but fail the test + } + x509_systemRoots = previousSystemRoots + x509_systemRootsMu.Unlock() + }) +} diff --git a/tool/tsh/common/proxy_test.go b/tool/tsh/common/proxy_test.go index 19223c30e19de..ea79973ce5e28 100644 --- a/tool/tsh/common/proxy_test.go +++ b/tool/tsh/common/proxy_test.go @@ -37,11 +37,9 @@ import ( "path" "path/filepath" "strconv" - "sync" "testing" "text/template" "time" - _ "unsafe" "github.com/gravitational/trace" "github.com/stretchr/testify/assert" @@ -54,6 +52,7 @@ import ( "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/api/utils/retryutils" + "github.com/gravitational/teleport/integration/helpers" "github.com/gravitational/teleport/lib" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/auth/mocku2f" @@ -66,29 +65,6 @@ import ( "github.com/gravitational/teleport/lib/tlsca" ) -//go:linkname x509_systemRootsMu crypto/x509.systemRootsMu -var x509_systemRootsMu sync.RWMutex - -//go:linkname x509_systemRoots crypto/x509.systemRoots -var x509_systemRoots *x509.CertPool - -func setSystemRoots(pool *x509.CertPool) (reset func()) { - // ensure that systemRoots was already set, or the next call to - // x509.SystemCertPool() will overwrite it - _, _ = x509.SystemCertPool() - - x509_systemRootsMu.Lock() - previousSystemRoots := x509_systemRoots - x509_systemRoots = pool - x509_systemRootsMu.Unlock() - - return func() { - x509_systemRootsMu.Lock() - x509_systemRoots = previousSystemRoots - x509_systemRootsMu.Unlock() - } -} - // TestSSH verifies "tsh ssh" command. func TestSSH(t *testing.T) { t.Setenv("_TELEPORT_TEST_NO_PARALLEL", "1") @@ -103,8 +79,7 @@ func TestSSH(t *testing.T) { certPool := x509.NewCertPool() require.True(t, certPool.AppendCertsFromPEM(webCertPEM)) - reset := setSystemRoots(certPool) - defer reset() + helpers.OverrideSystemRoots(t, certPool) s := newTestSuite(t, withRootConfigFunc(func(cfg *servicecfg.Config) { From b986a46ef80cd3ffda7879b22f497a1292b123de Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Thu, 25 Jan 2024 20:26:45 +0100 Subject: [PATCH 4/4] Use x509usefallbackroots=1 and SetFallbackRoots instead of go:linkname --- integration/helpers/x509.go | 71 -------------------------- tool/tsh/common/proxy_test.go | 34 ++++++------- tool/tsh/common/webpki_test.go | 92 ++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 89 deletions(-) delete mode 100644 integration/helpers/x509.go create mode 100644 tool/tsh/common/webpki_test.go diff --git a/integration/helpers/x509.go b/integration/helpers/x509.go deleted file mode 100644 index a386daef96dcc..0000000000000 --- a/integration/helpers/x509.go +++ /dev/null @@ -1,71 +0,0 @@ -// Teleport -// Copyright (C) 2024 Gravitational, Inc. -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -package helpers - -import ( - "crypto/x509" - "sync" - "testing" - _ "unsafe" -) - -//go:linkname x509_systemRootsMu crypto/x509.systemRootsMu -//go:linkname x509_systemRoots crypto/x509.systemRoots - -// These variables are used in x509 to cache the system cert pool after -// calculating it once; this is not a public interface and it's liable to even -// crash if the details of crypto/x509 are changed. An alternative to this would -// be to use x509.SetFallbackRoots and GODEBUG=x509usefallbackroots=1, but that -// can only be done once and requires a go:debug directive, so it will require -// the cooperation of the entire package's tests. -var ( - x509_systemRootsMu sync.RWMutex - x509_systemRoots *x509.CertPool -) - -// OverrideSystemRoots overrides the system certificate pool for the remainder -// of the test. The test must not be parallel, and OverrideSystemRoots will -// panic if called outside of a test binary. -func OverrideSystemRoots(t *testing.T, pool *x509.CertPool) { - if !testing.Testing() { - panic("SetSystemRoots was called outside of a test binary") - } - // panic if the test is parallel or when the test is made parallel - t.Setenv("_OverrideSystemRoots_called", "1") - - // ensure that systemRoots is already primed, or the next call to - // x509.SystemCertPool() will overwrite it - _, _ = x509.SystemCertPool() - - // the pool we store in systemRoots must not be changed - pool = pool.Clone() - - x509_systemRootsMu.Lock() - previousSystemRoots := x509_systemRoots - x509_systemRoots = pool - x509_systemRootsMu.Unlock() - - t.Cleanup(func() { - x509_systemRootsMu.Lock() - if x509_systemRoots != pool { - t.Log("x509.systemRoots was left modified after OverrideSystemRoots") - t.Fail() // continue execution but fail the test - } - x509_systemRoots = previousSystemRoots - x509_systemRootsMu.Unlock() - }) -} diff --git a/tool/tsh/common/proxy_test.go b/tool/tsh/common/proxy_test.go index ea79973ce5e28..0febb429571dd 100644 --- a/tool/tsh/common/proxy_test.go +++ b/tool/tsh/common/proxy_test.go @@ -52,7 +52,6 @@ import ( "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/api/utils/retryutils" - "github.com/gravitational/teleport/integration/helpers" "github.com/gravitational/teleport/lib" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/auth/mocku2f" @@ -67,6 +66,10 @@ import ( // TestSSH verifies "tsh ssh" command. func TestSSH(t *testing.T) { + if webpkiCACert == nil { + t.Skip("the current platform doesn't support adding CAs to the system pool") + } + t.Setenv("_TELEPORT_TEST_NO_PARALLEL", "1") defer lib.SetInsecureDevMode(lib.IsInsecureDevMode()) lib.SetInsecureDevMode(false) @@ -74,12 +77,7 @@ func TestSSH(t *testing.T) { d := t.TempDir() webCertPath := path.Join(d, "cert.pem") webKeyPath := path.Join(d, "key.pem") - webCertPEM := generateWebPKICA(t, webCertPath, webKeyPath) - - certPool := x509.NewCertPool() - require.True(t, certPool.AppendCertsFromPEM(webCertPEM)) - - helpers.OverrideSystemRoots(t, certPool) + generateWebPKICert(t, webCertPath, webKeyPath) s := newTestSuite(t, withRootConfigFunc(func(cfg *servicecfg.Config) { @@ -207,7 +205,7 @@ func testJumpHostSSHAccess(t *testing.T, s *suite) { }) } -func generateWebPKICA(t *testing.T, certPath, keyPath string) (certPEM []byte) { +func generateWebPKICert(t *testing.T, certPath, keyPath string) { key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) keyDER, err := x509.MarshalPKCS8PrivateKey(key) @@ -215,21 +213,21 @@ func generateWebPKICA(t *testing.T, certPath, keyPath string) (certPEM []byte) { keyPEM := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: keyDER}) require.NotNil(t, keyPEM) - certPEM, err = tlsca.GenerateSelfSignedCAWithConfig(tlsca.GenerateCAConfig{ - Signer: key, - Entity: pkix.Name{CommonName: "webpki ca"}, - TTL: time.Hour * 24 * 365, - // not sure why the CA needs to self-certify a specific address, but - // something in the crypto/tls verification checks for this when - // connecting to 127.0.0.1 - IPAddresses: []net.IP{net.IPv4(127, 0, 0, 1)}, + ca := &tlsca.CertAuthority{ + Cert: webpkiCACert, + Signer: webpkiCAKey, + } + certPEM, err := ca.GenerateCertificate(tlsca.CertificateRequest{ + PublicKey: key.Public(), + Subject: pkix.Name{CommonName: "proxy web addr cert"}, + NotAfter: time.Now().Add(12 * time.Hour), + DNSNames: []string{"127.0.0.1"}, + KeyUsage: x509.KeyUsageDigitalSignature, }) require.NoError(t, err) require.NoError(t, os.WriteFile(certPath, certPEM, 0o644)) require.NoError(t, os.WriteFile(keyPath, keyPEM, 0o644)) - - return certPEM } // TestWithRsync tests that Teleport works with rsync. diff --git a/tool/tsh/common/webpki_test.go b/tool/tsh/common/webpki_test.go new file mode 100644 index 0000000000000..e71882b1d759a --- /dev/null +++ b/tool/tsh/common/webpki_test.go @@ -0,0 +1,92 @@ +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +// We use x509usefallbackroots to replace the system cert pool with a system +// cert pool with an extra CA. +//go:debug x509usefallbackroots=1 + +package common + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "math/big" + "runtime" + "time" +) + +// webpkiCACert and webpkiCAKey are a CA added to the system cert pool. +var ( + webpkiCACert *x509.Certificate + webpkiCAKey *ecdsa.PrivateKey +) + +func init() { + // we can't add to the system cert pool on platforms with a system verifier, + // because verifying against the default cert pool only uses the system + // verifier, so on those we just don't set up our additional CA + // + // TODO(espadolini): see if embedding the mozilla trust store could work + if runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios" { + return + } + + serialNumber, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + panic(err) + } + + template := &x509.Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{CommonName: "Teleport testing web PKI CA"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour * 24 * 365), + + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{ + x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth, + }, + IsCA: true, + BasicConstraintsValid: true, + } + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + panic(err) + } + + der, err := x509.CreateCertificate(rand.Reader, template, template, key.Public(), key) + if err != nil { + panic(err) + } + + cert, err := x509.ParseCertificate(der) + if err != nil { + panic(err) + } + + pool, err := x509.SystemCertPool() + if err != nil { + pool = x509.NewCertPool() + } + pool.AddCert(cert) + x509.SetFallbackRoots(pool) + + webpkiCACert, webpkiCAKey = cert, key +}