From 07a2150d38d61e11a43ded326881d81ff09854ec Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Wed, 27 May 2020 15:18:55 -0700 Subject: [PATCH] Enforce SHA-512 for RSA SSH signatures Motivation: x/crypto/ssh defaults to using SHA-1 for signatures: https://github.com/golang/crypto/blob/master/ssh/keys.go#L963-L982 Because Teleport uses RSA for user, host and CA keys, we end up with SHA-1 by default. SHA-1 is now considered weak and OpenSSH plans to deprecate it: https://www.openssh.com/txt/release-8.3 Fix: Wrap all RSA `ssh.Signer`s and override `SignWithAlgorithm` to provide `SigAlgoRSASHA2512` if not otherwise specified. This will only affect new certs, existing certs will use `SigAlgoRSA` until rotated. For CA certs (e.g. exported with `tctl auth export`) users might need to manually rotate. Limited local testing with openssh 8.2 client and `-oHostKeyAlgorithms=-ssh-rsa` confirms that this works with a new cluster and fails with an old one. --- lib/auth/init.go | 1 + lib/auth/native/native.go | 3 ++ lib/auth/rotate.go | 2 + lib/client/interfaces.go | 2 + lib/reversetunnel/cache.go | 2 + lib/services/authority.go | 2 + lib/sshutils/fingerprint.go | 1 + lib/sshutils/signer.go | 46 +++++++++++++++++++++++ lib/sshutils/signer_test.go | 74 +++++++++++++++++++++++++++++++++++++ tool/tsh/common/identity.go | 1 + 10 files changed, 134 insertions(+) create mode 100644 lib/sshutils/signer_test.go diff --git a/lib/auth/init.go b/lib/auth/init.go index 4059eee3a0a0d..e523e500046e6 100644 --- a/lib/auth/init.go +++ b/lib/auth/init.go @@ -796,6 +796,7 @@ func ReadSSHIdentityFromKeyPair(keyBytes, certBytes []byte) (*Identity, error) { if err != nil { return nil, trace.BadParameter("failed to parse private key: %v", err) } + signer = sshutils.CompatSigner(signer) // this signer authenticates using certificate signed by the cert authority // not only by the public key certSigner, err := ssh.NewCertSigner(cert, signer) diff --git a/lib/auth/native/native.go b/lib/auth/native/native.go index 5af0ce1f473b0..b2fb61f8db146 100644 --- a/lib/auth/native/native.go +++ b/lib/auth/native/native.go @@ -30,6 +30,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/services" + "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/wrappers" @@ -193,6 +194,7 @@ func (k *Keygen) GenerateHostCert(c services.HostCertParams) ([]byte, error) { if err != nil { return nil, trace.Wrap(err) } + signer = sshutils.CompatSigner(signer) // Build a valid list of principals from the HostID and NodeName and then // add in any additional principals passed in. @@ -304,6 +306,7 @@ func (k *Keygen) GenerateUserCert(c services.UserCertParams) ([]byte, error) { if err != nil { return nil, trace.Wrap(err) } + signer = sshutils.CompatSigner(signer) if err := cert.SignCert(rand.Reader, signer); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/rotate.go b/lib/auth/rotate.go index 0f856e91d2c9b..fc5dfb11afe99 100644 --- a/lib/auth/rotate.go +++ b/lib/auth/rotate.go @@ -24,6 +24,7 @@ import ( "github.com/gravitational/teleport/lib/auth/native" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/services" + "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/trace" @@ -476,6 +477,7 @@ func startNewRotation(req rotationReq, ca services.CertAuthority) error { if err != nil { return trace.Wrap(err) } + signer = sshutils.CompatSigner(signer) sshPubPEM = ssh.MarshalAuthorizedKey(signer.PublicKey()) sshPrivPEM = req.privateKey diff --git a/lib/client/interfaces.go b/lib/client/interfaces.go index 918c3f4cefee6..65021aedc3414 100644 --- a/lib/client/interfaces.go +++ b/lib/client/interfaces.go @@ -27,6 +27,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/auth/native" + "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" @@ -245,6 +246,7 @@ func (k *Key) AsAuthMethod() (ssh.AuthMethod, error) { if signer, err = ssh.NewCertSigner(keys[0].Certificate, signer); err != nil { return nil, trace.Wrap(err) } + signer = sshutils.CompatSigner(signer) return NewAuthMethodForCert(signer), nil } diff --git a/lib/reversetunnel/cache.go b/lib/reversetunnel/cache.go index 5e82ec85e3577..ebc68ddb65b92 100644 --- a/lib/reversetunnel/cache.go +++ b/lib/reversetunnel/cache.go @@ -27,6 +27,7 @@ import ( "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/sshca" + "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/trace" "github.com/gravitational/ttlmap" @@ -154,6 +155,7 @@ func (c *certificateCache) generateHostCert(principals []string) (ssh.Signer, er if err != nil { return nil, trace.Wrap(err) } + privateKey = sshutils.CompatSigner(privateKey) publicKey, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) if err != nil { return nil, err diff --git a/lib/services/authority.go b/lib/services/authority.go index 5b17f25f57fce..5eaae995ef988 100644 --- a/lib/services/authority.go +++ b/lib/services/authority.go @@ -24,6 +24,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/lib/defaults" + "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/wrappers" @@ -553,6 +554,7 @@ func (ca *CertAuthorityV2) Signers() ([]ssh.Signer, error) { if err != nil { return nil, trace.Wrap(err) } + signer = sshutils.CompatSigner(signer) out = append(out, signer) } return out, nil diff --git a/lib/sshutils/fingerprint.go b/lib/sshutils/fingerprint.go index 6eff2c1fcca93..e7cc89e060585 100644 --- a/lib/sshutils/fingerprint.go +++ b/lib/sshutils/fingerprint.go @@ -27,5 +27,6 @@ func PrivateKeyFingerprint(keyBytes []byte) (string, error) { if err != nil { return "", trace.Wrap(err) } + signer = CompatSigner(signer) return Fingerprint(signer.PublicKey()), nil } diff --git a/lib/sshutils/signer.go b/lib/sshutils/signer.go index 1746cd4df766d..71e76dfcf0c20 100644 --- a/lib/sshutils/signer.go +++ b/lib/sshutils/signer.go @@ -18,6 +18,7 @@ package sshutils import ( "crypto" + "io" "golang.org/x/crypto/ssh" @@ -32,6 +33,7 @@ func NewSigner(keyBytes, certBytes []byte) (ssh.Signer, error) { if err != nil { return nil, trace.Wrap(err, "failed to parse SSH private key") } + keySigner = CompatSigner(keySigner) pubkey, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) if err != nil { @@ -59,3 +61,47 @@ func CryptoPublicKey(publicKey []byte) (crypto.PublicKey, error) { } return cryptoPubKey.CryptoPublicKey(), nil } + +// CompatSigner wraps a provided ssh.Signer to ensure algorithm compatibility +// with OpenSSH. +// +// Right now it means forcing SHA-2 signatures with RSA keys, instead of the +// default SHA-1 used by x/crypto/ssh. +// See https://www.openssh.com/txt/release-8.2 for context. +// This will be obsolete once https://github.com/golang/go/issues/37278 is +// fixed upstream. +// +// If provided Signer is not an RSA key or does not implement +// ssh.AlgorithmSigner, it's returned as is. +// +// DELETE IN 5.0: assuming https://github.com/golang/go/issues/37278 is fixed +// by then and we pull in the fix. Also delete all call sites. +func CompatSigner(s ssh.Signer) ssh.Signer { + if s.PublicKey().Type() != ssh.KeyAlgoRSA { + return s + } + as, ok := s.(ssh.AlgorithmSigner) + if !ok { + return s + } + return fixedAlgorithmSigner{ + AlgorithmSigner: as, + alg: ssh.SigAlgoRSASHA2512, + } +} + +type fixedAlgorithmSigner struct { + ssh.AlgorithmSigner + alg string +} + +func (s fixedAlgorithmSigner) SignWithAlgorithm(rand io.Reader, data []byte, alg string) (*ssh.Signature, error) { + if alg == "" { + alg = s.alg + } + return s.AlgorithmSigner.SignWithAlgorithm(rand, data, alg) +} + +func (s fixedAlgorithmSigner) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) { + return s.AlgorithmSigner.SignWithAlgorithm(rand, data, s.alg) +} diff --git a/lib/sshutils/signer_test.go b/lib/sshutils/signer_test.go new file mode 100644 index 0000000000000..cc0b139099519 --- /dev/null +++ b/lib/sshutils/signer_test.go @@ -0,0 +1,74 @@ +package sshutils + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "io" + + "golang.org/x/crypto/ssh" + "gopkg.in/check.v1" +) + +type CompatSignerSuite struct{} + +var _ = check.Suite(&CompatSignerSuite{}) + +func (s *CompatSignerSuite) TestCompatSignerNoop(c *check.C) { + ecdsaKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + c.Assert(err, check.IsNil) + ecdsaSigner, err := ssh.NewSignerFromSigner(ecdsaKey) + c.Assert(err, check.IsNil) + + // ECDSA key should be returned as-is, not wrapped. + c.Assert(CompatSigner(ecdsaSigner), check.Equals, ecdsaSigner) +} + +func (s *CompatSignerSuite) TestCompatSigner(c *check.C) { + rsaSigner, err := newMockRSASigner() + c.Assert(err, check.IsNil) + + wrapped := CompatSigner(rsaSigner) + // RSA key should get wrapped. + c.Assert(wrapped, check.Not(check.Equals), rsaSigner) + wrappedAS := wrapped.(ssh.AlgorithmSigner) + + // Simple Sign call should use the enforced SHA-2 algorithm. + wrapped.Sign(nil, nil) + c.Assert(rsaSigner.lastAlg, check.Equals, ssh.SigAlgoRSASHA2512) + rsaSigner.lastAlg = "" + + // SignWithAlgorithm without specifying an algorithm should use the + // enforced SHA-2 algorithm. + wrappedAS.SignWithAlgorithm(nil, nil, "") + c.Assert(rsaSigner.lastAlg, check.Equals, ssh.SigAlgoRSASHA2512) + rsaSigner.lastAlg = "" + + // SignWithAlgorithm *with* an algorithm should respect the provided + // algorithm. + wrappedAS.SignWithAlgorithm(nil, nil, "foo") + c.Assert(rsaSigner.lastAlg, check.Equals, "foo") +} + +type mockRSASigner struct { + ssh.Signer + lastAlg string +} + +func newMockRSASigner() (*mockRSASigner, error) { + rsaKey, err := rsa.GenerateKey(rand.Reader, 2056) + if err != nil { + return nil, err + } + rsaSigner, err := ssh.NewSignerFromSigner(rsaKey) + if err != nil { + return nil, err + } + return &mockRSASigner{Signer: rsaSigner}, nil +} + +func (s *mockRSASigner) SignWithAlgorithm(rand io.Reader, data []byte, alg string) (*ssh.Signature, error) { + s.lastAlg = alg + return nil, nil +} diff --git a/tool/tsh/common/identity.go b/tool/tsh/common/identity.go index a547a99c91911..2016c548f1137 100644 --- a/tool/tsh/common/identity.go +++ b/tool/tsh/common/identity.go @@ -72,6 +72,7 @@ func LoadIdentity(idFn string) (*client.Key, ssh.HostKeyCallback, error) { if err != nil { return nil, nil, trace.Wrap(err) } + signer = sshutils.CompatSigner(signer) // validate TLS Cert (if present): if len(ident.Certs.TLS) > 0 { _, err := tls.X509KeyPair(ident.Certs.TLS, ident.PrivateKey)