From 21f0acca104f3ac6cd2719f92cf72f656fbe9180 Mon Sep 17 00:00:00 2001 From: Russell Jones Date: Thu, 25 Jul 2019 21:36:11 +0000 Subject: [PATCH] Only check certificate algorithms in FIPS mode. Update utils.CertChecker to only check key and certificate algorithms when in FIPS mode. Otherwise accept keys and certificates generated with any algorithm. --- lib/client/api.go | 7 +++++ lib/client/interfaces.go | 5 +++- lib/client/keyagent.go | 1 + lib/client/keystore_test.go | 36 +++++++++++++++++----- lib/reversetunnel/remotesite.go | 1 + lib/reversetunnel/srv.go | 10 ++++++- lib/service/service.go | 3 ++ lib/srv/authhandlers.go | 6 ++++ lib/srv/forward/sshserver.go | 5 ++++ lib/srv/regular/sshserver.go | 16 +++++++++- lib/sshutils/server.go | 19 ++++++++++-- lib/sshutils/server_test.go | 53 ++++++++++++++++++++++++++------- lib/utils/checker.go | 22 +++++++++----- lib/utils/checker_test.go | 47 +++++++++++++++++++++++++---- 14 files changed, 196 insertions(+), 35 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index 719f8d93ceb20..c4ef1d41cb7f4 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -49,6 +49,7 @@ import ( "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/events" + "github.com/gravitational/teleport/lib/modules" "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/session" "github.com/gravitational/teleport/lib/shell" @@ -2145,3 +2146,9 @@ func ParseDynamicPortForwardSpec(spec []string) (DynamicForwardedPorts, error) { func InsecureSkipHostKeyChecking(host string, remote net.Addr, key ssh.PublicKey) error { return nil } + +// isFIPS returns if the binary was build with BoringCrypto, which implies +// FedRAMP/FIPS 140-2 mode for tsh. +func isFIPS() bool { + return modules.GetModules().IsBoringBinary() +} diff --git a/lib/client/interfaces.go b/lib/client/interfaces.go index a5d416466421f..e0f8193e54d68 100644 --- a/lib/client/interfaces.go +++ b/lib/client/interfaces.go @@ -245,9 +245,12 @@ func (k *Key) CheckCert() error { if err != nil { return trace.Wrap(err) } + // A valid principal is always passed in because the principals are not being // checked here, but rather the validity period, signature, and algorithms. - certChecker := utils.CertChecker{} + certChecker := utils.CertChecker{ + FIPS: isFIPS(), + } err = certChecker.CheckCert(cert.ValidPrincipals[0], cert) if err != nil { return trace.Wrap(err) diff --git a/lib/client/keyagent.go b/lib/client/keyagent.go index 9687cd75273ba..8e13b98ccaf41 100644 --- a/lib/client/keyagent.go +++ b/lib/client/keyagent.go @@ -269,6 +269,7 @@ func (a *LocalKeyAgent) CheckHostSignature(addr string, remote net.Addr, key ssh IsHostAuthority: a.checkHostCertificate, HostKeyFallback: a.checkHostKey, }, + FIPS: isFIPS(), } err := certChecker.CheckHostKey(addr, remote, key) if err != nil { diff --git a/lib/client/keystore_test.go b/lib/client/keystore_test.go index ee499de40c27e..c1ee7ebaef4fb 100644 --- a/lib/client/keystore_test.go +++ b/lib/client/keystore_test.go @@ -227,11 +227,11 @@ func (s *KeyStoreTestSuite) makeSignedKey(c *check.C, makeExpired bool) *Key { c.Assert(err, check.IsNil) cert, err = s.keygen.GenerateUserCert(services.UserCertParams{ - PrivateCASigningKey: CAPriv, - PublicUserKey: pub, - Username: username, - AllowedLogins: allowedLogins, - TTL: ttl, + PrivateCASigningKey: CAPriv, + PublicUserKey: pub, + Username: username, + AllowedLogins: allowedLogins, + TTL: ttl, PermitAgentForwarding: false, PermitPortForwarding: true, }) @@ -244,8 +244,8 @@ func (s *KeyStoreTestSuite) makeSignedKey(c *check.C, makeExpired bool) *Key { } } -// TestCheckKey make sure Teleport clients don't load invalid user -// certificates. The main check is the certificate algorithms. +// TestCheckKey make sure Teleport clients can load non-RSA algorithms in +// normal operating mode. func (s *KeyStoreTestSuite) TestCheckKey(c *check.C) { key := s.makeSignedKey(c, false) @@ -257,6 +257,28 @@ func (s *KeyStoreTestSuite) TestCheckKey(c *check.C) { err = s.store.AddKey("host.a", "bob", key) c.Assert(err, check.IsNil) + _, err = s.store.GetKey("host.a", "bob") + c.Assert(err, check.IsNil) +} + +// TestCheckKey make sure Teleport clients don't load invalid +// certificates while in FIPS mode. +func (s *KeyStoreTestSuite) TestCheckKeyFIPS(c *check.C) { + // This test only runs in FIPS mode. + if !isFIPS() { + return + } + + key := s.makeSignedKey(c, false) + + // Swap out the key with a ECDSA SSH key. + ellipticCertificate, _, err := utils.CreateEllipticCertificate("foo", ssh.UserCert) + c.Assert(err, check.IsNil) + key.Cert = ssh.MarshalAuthorizedKey(ellipticCertificate) + + err = s.store.AddKey("host.a", "bob", key) + c.Assert(err, check.IsNil) + _, err = s.store.GetKey("host.a", "bob") c.Assert(err, check.NotNil) } diff --git a/lib/reversetunnel/remotesite.go b/lib/reversetunnel/remotesite.go index a2566f1b3080d..1a219561c97c4 100644 --- a/lib/reversetunnel/remotesite.go +++ b/lib/reversetunnel/remotesite.go @@ -561,6 +561,7 @@ func (s *remoteSite) dialWithAgent(params DialParams) (net.Conn, error) { DataDir: s.srv.Config.DataDir, Address: params.Address, UseTunnel: targetConn.UseTunnel(), + FIPS: s.srv.FIPS, } remoteServer, err := forward.New(serverConfig) if err != nil { diff --git a/lib/reversetunnel/srv.go b/lib/reversetunnel/srv.go index 0786e7af8c18f..cdf076aeabfb7 100644 --- a/lib/reversetunnel/srv.go +++ b/lib/reversetunnel/srv.go @@ -172,12 +172,17 @@ type Config struct { // DataDir is a local server data directory DataDir string + // PollingPeriod specifies polling period for internal sync // goroutines, used to speed up sync-ups in tests. PollingPeriod time.Duration // Component is a component used in logs Component string + + // FIPS means Teleport was started in a FedRAMP/FIPS 140-2 compliant + // configuration. + FIPS bool } // CheckAndSetDefaults checks parameters and sets default values @@ -288,6 +293,7 @@ func NewServer(cfg Config) (Server, error) { sshutils.SetCiphers(cfg.Ciphers), sshutils.SetKEXAlgorithms(cfg.KEXAlgorithms), sshutils.SetMACAlgorithms(cfg.MACAlgorithms), + sshutils.SetFIPS(cfg.FIPS), ) if err != nil { return nil, err @@ -778,7 +784,9 @@ func (s *server) checkHostCert(logger *log.Entry, user string, clusterName strin return trace.NotFound("cluster %v has no matching CA keys", clusterName) } - checker := utils.CertChecker{} + checker := utils.CertChecker{ + FIPS: s.FIPS, + } if err := checker.CheckCert(user, cert); err != nil { return trace.BadParameter(err.Error()) } diff --git a/lib/service/service.go b/lib/service/service.go index 811037ea2b559..acf717e4e1ae1 100644 --- a/lib/service/service.go +++ b/lib/service/service.go @@ -1428,6 +1428,7 @@ func (process *TeleportProcess) initSSH() error { regular.SetPAMConfig(cfg.SSH.PAM), regular.SetRotationGetter(process.getRotation), regular.SetUseTunnel(conn.UseTunnel()), + regular.SetFIPS(cfg.FIPS), ) if err != nil { return trace.Wrap(err) @@ -1972,6 +1973,7 @@ func (process *TeleportProcess) initProxyEndpoint(conn *Connector) error { MACAlgorithms: cfg.MACAlgorithms, DataDir: process.Config.DataDir, PollingPeriod: process.Config.PollingPeriod, + FIPS: cfg.FIPS, }) if err != nil { return trace.Wrap(err) @@ -2081,6 +2083,7 @@ func (process *TeleportProcess) initProxyEndpoint(conn *Connector) error { regular.SetMACAlgorithms(cfg.MACAlgorithms), regular.SetNamespace(defaults.Namespace), regular.SetRotationGetter(process.getRotation), + regular.SetFIPS(cfg.FIPS), ) if err != nil { return trace.Wrap(err) diff --git a/lib/srv/authhandlers.go b/lib/srv/authhandlers.go index 53f598e0ae079..c4355322280e3 100644 --- a/lib/srv/authhandlers.go +++ b/lib/srv/authhandlers.go @@ -52,6 +52,10 @@ type AuthHandlers struct { // AccessPoint is used to access the Auth Server. AccessPoint auth.AccessPoint + + // FIPS mode means Teleport started in a FedRAMP/FIPS 140-2 compliant + // configuration. + FIPS bool } // BuildIdentityContext returns an IdentityContext populated with information @@ -178,6 +182,7 @@ func (h *AuthHandlers) UserKeyAuth(conn ssh.ConnMetadata, key ssh.PublicKey) (*s CertChecker: ssh.CertChecker{ IsUserAuthority: h.IsUserAuthority, }, + FIPS: h.FIPS, } permissions, err := certChecker.Authenticate(conn, key) if err != nil { @@ -254,6 +259,7 @@ func (h *AuthHandlers) HostKeyAuth(addr string, remote net.Addr, key ssh.PublicK IsHostAuthority: h.IsHostAuthority, HostKeyFallback: h.hostKeyCallback, }, + FIPS: h.FIPS, } err := certChecker.CheckHostKey(addr, remote, key) if err != nil { diff --git a/lib/srv/forward/sshserver.go b/lib/srv/forward/sshserver.go index 9f388c932b5e0..737dd135d2d03 100644 --- a/lib/srv/forward/sshserver.go +++ b/lib/srv/forward/sshserver.go @@ -168,6 +168,10 @@ type ServerConfig struct { // Clock is an optoinal clock to override default real time clock Clock clockwork.Clock + + // FIPS mode means Teleport started in a FedRAMP/FIPS 140-2 compliant + // configuration. + FIPS bool } // CheckDefaults makes sure all required parameters are passed in. @@ -261,6 +265,7 @@ func New(c ServerConfig) (*Server, error) { Component: teleport.ComponentForwardingNode, AuditLog: c.AuthClient, AccessPoint: c.AuthClient, + FIPS: c.FIPS, } // Common term handlers. diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go index d51daae564a7c..9746ea705e4a5 100644 --- a/lib/srv/regular/sshserver.go +++ b/lib/srv/regular/sshserver.go @@ -144,6 +144,10 @@ type Server struct { // useTunnel is used to inform other components that this server is // requesting connections to it come over a reverse tunnel. useTunnel bool + + // fips means Teleport started in a FedRAMP/FIPS 140-2 compliant + // configuration. + fips bool } // GetClock returns server clock implementation @@ -408,6 +412,13 @@ func SetUseTunnel(useTunnel bool) ServerOption { } } +func SetFIPS(fips bool) ServerOption { + return func(s *Server) error { + s.fips = fips + return nil + } +} + // New returns an unstarted server func New(addr utils.NetAddr, hostname string, @@ -485,6 +496,7 @@ func New(addr utils.NetAddr, Component: component, AuditLog: s.alog, AccessPoint: s.authService, + FIPS: s.fips, } // common term handlers @@ -500,7 +512,9 @@ func New(addr utils.NetAddr, sshutils.SetRequestHandler(s), sshutils.SetCiphers(s.ciphers), sshutils.SetKEXAlgorithms(s.kexAlgorithms), - sshutils.SetMACAlgorithms(s.macAlgorithms)) + sshutils.SetMACAlgorithms(s.macAlgorithms), + sshutils.SetFIPS(s.fips), + ) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/sshutils/server.go b/lib/sshutils/server.go index 7c68c64965f5b..c144be213dbbf 100644 --- a/lib/sshutils/server.go +++ b/lib/sshutils/server.go @@ -76,6 +76,10 @@ type Server struct { // insecureSkipHostValidation does not validate the host signers to make sure // they are a valid certificate. Used in tests. insecureSkipHostValidation bool + + // fips means Teleport started in a FedRAMP/FIPS 140-2 compliant + // configuration. + fips bool } const ( @@ -222,6 +226,13 @@ func SetMACAlgorithms(macAlgorithms []string) ServerOption { } } +func SetFIPS(fips bool) ServerOption { + return func(s *Server) error { + s.fips = fips + return nil + } +} + func (s *Server) Addr() string { return s.listener.Addr().String() } @@ -493,7 +504,7 @@ func (s *Server) checkArguments(a utils.NetAddr, h NewChanHandler, hostSigners [ return trace.BadParameter("host signer can not be nil") } if !s.insecureSkipHostValidation { - err := validateHostSigner(signer) + err := validateHostSigner(s.fips, signer) if err != nil { return trace.Wrap(err) } @@ -506,7 +517,7 @@ func (s *Server) checkArguments(a utils.NetAddr, h NewChanHandler, hostSigners [ } // validateHostSigner make sure the signer is a valid certificate. -func validateHostSigner(signer ssh.Signer) error { +func validateHostSigner(fips bool, signer ssh.Signer) error { cert, ok := signer.PublicKey().(*ssh.Certificate) if !ok { return trace.BadParameter("only host certificates supported") @@ -515,7 +526,9 @@ func validateHostSigner(signer ssh.Signer) error { return trace.BadParameter("at least one valid principal is required in host certificate") } - certChecker := utils.CertChecker{} + certChecker := utils.CertChecker{ + FIPS: fips, + } err := certChecker.CheckCert(cert.ValidPrincipals[0], cert) if err != nil { return trace.Wrap(err) diff --git a/lib/sshutils/server_test.go b/lib/sshutils/server_test.go index ea2b92a11d0c7..1b75daaea6546 100644 --- a/lib/sshutils/server_test.go +++ b/lib/sshutils/server_test.go @@ -178,7 +178,7 @@ func (s *ServerSuite) TestConfigureCiphers(c *check.C) { // TestHostSigner makes sure Teleport can not be started with a invalid host // certificate. The main check is the certificate algorithms. -func (s *ServerSuite) TestHostSigner(c *check.C) { +func (s *ServerSuite) TestHostSignerFIPS(c *check.C) { _, ellipticSigner, err := utils.CreateEllipticCertificate("foo", ssh.HostCert) c.Assert(err, check.IsNil) @@ -186,15 +186,48 @@ func (s *ServerSuite) TestHostSigner(c *check.C) { nch.Reject(ssh.Prohibited, "nothing to see here") }) - _, err = NewServer( - "test", - utils.NetAddr{AddrNetwork: "tcp", Addr: "localhost:0"}, - newChanHandler, - []ssh.Signer{ellipticSigner}, - AuthMethods{Password: pass("abc123")}, - SetCiphers([]string{"aes128-ctr"}), - ) - c.Assert(err, check.NotNil) + var tests = []struct { + inSigner ssh.Signer + inFIPS bool + outError bool + }{ + // ECDSA when in FIPS mode should fail. + { + inSigner: ellipticSigner, + inFIPS: true, + outError: true, + }, + // RSA when in FIPS mode is okay. + { + inSigner: s.signer, + inFIPS: true, + outError: false, + }, + // ECDSA when in not FIPS mode should succeed. + { + inSigner: ellipticSigner, + inFIPS: false, + outError: false, + }, + // RSA when in not FIPS mode should succeed. + { + inSigner: s.signer, + inFIPS: false, + outError: false, + }, + } + for _, tt := range tests { + _, err := NewServer( + "test", + utils.NetAddr{AddrNetwork: "tcp", Addr: "localhost:0"}, + newChanHandler, + []ssh.Signer{tt.inSigner}, + AuthMethods{Password: pass("abc123")}, + SetCiphers([]string{"aes128-ctr"}), + SetFIPS(tt.inFIPS), + ) + c.Assert(err != nil, check.Equals, tt.outError) + } } func wait(c *check.C, srv *Server) { diff --git a/lib/utils/checker.go b/lib/utils/checker.go index e3f110344b598..f213151a47d21 100644 --- a/lib/utils/checker.go +++ b/lib/utils/checker.go @@ -32,16 +32,19 @@ import ( "github.com/gravitational/trace" ) -// CertChecker is a drop-in replacement for ssh.CertChecker that also checks -// if the certificate (or key) were generated with a valid algorithm. +// CertChecker is a drop-in replacement for ssh.CertChecker. In FIPS mode, +// checks if the certificate (or key) were generated with a supported algorithm. type CertChecker struct { ssh.CertChecker + + // FIPS means in addition to checking the validity of the key or + // certificate, also check that FIPS 140-2 algorithms were used. + FIPS bool } // Authenticate checks the validity of a user certificate. -// a value for ServerConfig.PublicKeyCallback. func (c *CertChecker) Authenticate(conn ssh.ConnMetadata, key ssh.PublicKey) (*ssh.Permissions, error) { - err := validate(key) + err := c.validate(key) if err != nil { return nil, trace.Wrap(err) } @@ -56,7 +59,7 @@ func (c *CertChecker) Authenticate(conn ssh.ConnMetadata, key ssh.PublicKey) (*s // CheckCert checks certificate metadata and signature. func (c *CertChecker) CheckCert(principal string, cert *ssh.Certificate) error { - err := validate(cert) + err := c.validate(cert) if err != nil { return trace.Wrap(err) } @@ -66,7 +69,7 @@ func (c *CertChecker) CheckCert(principal string, cert *ssh.Certificate) error { // CheckHostKey checks the validity of a host certificate. func (c *CertChecker) CheckHostKey(addr string, remote net.Addr, key ssh.PublicKey) error { - err := validate(key) + err := c.validate(key) if err != nil { return trace.Wrap(err) } @@ -74,7 +77,12 @@ func (c *CertChecker) CheckHostKey(addr string, remote net.Addr, key ssh.PublicK return c.CertChecker.CheckHostKey(addr, remote, key) } -func validate(key ssh.PublicKey) error { +func (c *CertChecker) validate(key ssh.PublicKey) error { + // When not in FIPS mode, accept all algorithms and key sizes. + if !c.FIPS { + return nil + } + switch cert := key.(type) { case *ssh.Certificate: err := validateAlgorithm(cert.Key) diff --git a/lib/utils/checker_test.go b/lib/utils/checker_test.go index 4445228528bee..fa58f26f8a734 100644 --- a/lib/utils/checker_test.go +++ b/lib/utils/checker_test.go @@ -35,9 +35,46 @@ type CheckerSuite struct{} var _ = fmt.Printf var _ = check.Suite(&CheckerSuite{}) -// TestValidate makes sure the public key is a valid algorithm -// that Teleport supports. +// TestValidate checks what algorithm are supported in regular (non-FIPS) mode. func (s *CheckerSuite) TestValidate(c *check.C) { + checker := CertChecker{} + + rsaKey, err := rsa.GenerateKey(rand.Reader, teleport.RSAKeySize) + c.Assert(err, check.IsNil) + smallRSAKey, err := rsa.GenerateKey(rand.Reader, 1024) + c.Assert(err, check.IsNil) + ellipticKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + c.Assert(err, check.IsNil) + + // 2048-bit RSA keys are valid. + cryptoKey := rsaKey.Public() + sshKey, err := ssh.NewPublicKey(cryptoKey) + c.Assert(err, check.IsNil) + err = checker.validate(sshKey) + c.Assert(err, check.IsNil) + + // 1024-bit RSA keys are valid. + cryptoKey = smallRSAKey.Public() + sshKey, err = ssh.NewPublicKey(cryptoKey) + c.Assert(err, check.IsNil) + err = checker.validate(sshKey) + c.Assert(err, check.IsNil) + + // ECDSA keys are valid. + cryptoKey = ellipticKey.Public() + sshKey, err = ssh.NewPublicKey(cryptoKey) + c.Assert(err, check.IsNil) + err = checker.validate(sshKey) + c.Assert(err, check.IsNil) +} + +// TestValidate makes sure the public key is a valid algorithm +// that Teleport supports while in FIPS mode. +func (s *CheckerSuite) TestValidateFIPS(c *check.C) { + checker := CertChecker{ + FIPS: true, + } + rsaKey, err := rsa.GenerateKey(rand.Reader, teleport.RSAKeySize) c.Assert(err, check.IsNil) smallRSAKey, err := rsa.GenerateKey(rand.Reader, 1024) @@ -49,20 +86,20 @@ func (s *CheckerSuite) TestValidate(c *check.C) { cryptoKey := rsaKey.Public() sshKey, err := ssh.NewPublicKey(cryptoKey) c.Assert(err, check.IsNil) - err = validate(sshKey) + err = checker.validate(sshKey) c.Assert(err, check.IsNil) // 1024-bit RSA keys are not valid. cryptoKey = smallRSAKey.Public() sshKey, err = ssh.NewPublicKey(cryptoKey) c.Assert(err, check.IsNil) - err = validate(sshKey) + err = checker.validate(sshKey) c.Assert(err, check.NotNil) // ECDSA keys are not valid. cryptoKey = ellipticKey.Public() sshKey, err = ssh.NewPublicKey(cryptoKey) c.Assert(err, check.IsNil) - err = validate(sshKey) + err = checker.validate(sshKey) c.Assert(err, check.NotNil) }