Skip to content

Commit

Permalink
ssh: add MultiAlgorithmSigner
Browse files Browse the repository at this point in the history
MultiAlgorithmSigner allows to restrict client-side, server-side and
certificate signing algorithms.

Fixes golang/go#52132
Fixes golang/go#36261

Change-Id: I295092f1bba647327aaaf294f110e9157d294159
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/508398
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
  • Loading branch information
drakkan authored and gopherbot committed Sep 20, 2023
1 parent 3f0842a commit 28c53ff
Show file tree
Hide file tree
Showing 9 changed files with 554 additions and 66 deletions.
38 changes: 30 additions & 8 deletions ssh/certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ import (

// Certificate algorithm names from [PROTOCOL.certkeys]. These values can appear
// in Certificate.Type, PublicKey.Type, and ClientConfig.HostKeyAlgorithms.
// Unlike key algorithm names, these are not passed to AlgorithmSigner and don't
// appear in the Signature.Format field.
// Unlike key algorithm names, these are not passed to AlgorithmSigner nor
// returned by MultiAlgorithmSigner and don't appear in the Signature.Format
// field.
const (
CertAlgoRSAv01 = "ssh-rsa-cert-v01@openssh.com"
CertAlgoDSAv01 = "ssh-dss-cert-v01@openssh.com"
Expand Down Expand Up @@ -255,10 +256,17 @@ func NewCertSigner(cert *Certificate, signer Signer) (Signer, error) {
return nil, errors.New("ssh: signer and cert have different public key")
}

if algorithmSigner, ok := signer.(AlgorithmSigner); ok {
switch s := signer.(type) {
case MultiAlgorithmSigner:
return &multiAlgorithmSigner{
AlgorithmSigner: &algorithmOpenSSHCertSigner{
&openSSHCertSigner{cert, signer}, s},
supportedAlgorithms: s.Algorithms(),
}, nil
case AlgorithmSigner:
return &algorithmOpenSSHCertSigner{
&openSSHCertSigner{cert, signer}, algorithmSigner}, nil
} else {
&openSSHCertSigner{cert, signer}, s}, nil
default:
return &openSSHCertSigner{cert, signer}, nil
}
}
Expand Down Expand Up @@ -432,16 +440,30 @@ func (c *CertChecker) CheckCert(principal string, cert *Certificate) error {
}

// SignCert signs the certificate with an authority, setting the Nonce,
// SignatureKey, and Signature fields.
// SignatureKey, and Signature fields. If the authority implements the
// MultiAlgorithmSigner interface the first algorithm in the list is used. This
// is useful if you want to sign with a specific algorithm.
func (c *Certificate) SignCert(rand io.Reader, authority Signer) error {
c.Nonce = make([]byte, 32)
if _, err := io.ReadFull(rand, c.Nonce); err != nil {
return err
}
c.SignatureKey = authority.PublicKey()

// Default to KeyAlgoRSASHA512 for ssh-rsa signers.
if v, ok := authority.(AlgorithmSigner); ok && v.PublicKey().Type() == KeyAlgoRSA {
if v, ok := authority.(MultiAlgorithmSigner); ok {
if len(v.Algorithms()) == 0 {
return errors.New("the provided authority has no signature algorithm")
}
// Use the first algorithm in the list.
sig, err := v.SignWithAlgorithm(rand, c.bytesForSigning(), v.Algorithms()[0])
if err != nil {
return err
}
c.Signature = sig
return nil
} else if v, ok := authority.(AlgorithmSigner); ok && v.PublicKey().Type() == KeyAlgoRSA {
// Default to KeyAlgoRSASHA512 for ssh-rsa signers.
// TODO: consider using KeyAlgoRSASHA256 as default.
sig, err := v.SignWithAlgorithm(rand, c.bytesForSigning(), KeyAlgoRSASHA512)
if err != nil {
return err
Expand Down
100 changes: 94 additions & 6 deletions ssh/certs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,30 @@ func TestHostKeyCert(t *testing.T) {
}

for _, test := range []struct {
addr string
succeed bool
addr string
succeed bool
certSignerAlgorithms []string // Empty means no algorithm restrictions.
clientHostKeyAlgorithms []string
}{
{addr: "hostname:22", succeed: true},
{
addr: "hostname:22",
succeed: true,
certSignerAlgorithms: []string{KeyAlgoRSASHA256, KeyAlgoRSASHA512},
clientHostKeyAlgorithms: []string{CertAlgoRSASHA512v01},
},
{
addr: "hostname:22",
succeed: false,
certSignerAlgorithms: []string{KeyAlgoRSASHA256, KeyAlgoRSASHA512},
clientHostKeyAlgorithms: []string{CertAlgoRSAv01},
},
{
addr: "hostname:22",
succeed: false,
certSignerAlgorithms: []string{KeyAlgoRSASHA256, KeyAlgoRSASHA512},
clientHostKeyAlgorithms: []string{KeyAlgoRSASHA512}, // Not a certificate algorithm.
},
{addr: "otherhost:22", succeed: false}, // The certificate is valid for 'otherhost' as hostname, but we only recognize the authority of the signer for the address 'hostname:22'
{addr: "lasthost:22", succeed: false},
} {
Expand All @@ -207,14 +227,24 @@ func TestHostKeyCert(t *testing.T) {
conf := ServerConfig{
NoClientAuth: true,
}
conf.AddHostKey(certSigner)
if len(test.certSignerAlgorithms) > 0 {
mas, err := NewSignerWithAlgorithms(certSigner.(AlgorithmSigner), test.certSignerAlgorithms)
if err != nil {
errc <- err
return
}
conf.AddHostKey(mas)
} else {
conf.AddHostKey(certSigner)
}
_, _, _, err := NewServerConn(c1, &conf)
errc <- err
}()

config := &ClientConfig{
User: "user",
HostKeyCallback: checker.CheckHostKey,
User: "user",
HostKeyCallback: checker.CheckHostKey,
HostKeyAlgorithms: test.clientHostKeyAlgorithms,
}
_, _, _, err = NewClientConn(c2, test.addr, config)

Expand Down Expand Up @@ -242,6 +272,20 @@ func (s *legacyRSASigner) Sign(rand io.Reader, data []byte) (*Signature, error)
}

func TestCertTypes(t *testing.T) {
algorithmSigner, ok := testSigners["rsa"].(AlgorithmSigner)
if !ok {
t.Fatal("rsa test signer does not implement the AlgorithmSigner interface")
}
multiAlgoSignerSHA256, err := NewSignerWithAlgorithms(algorithmSigner, []string{KeyAlgoRSASHA256})
if err != nil {
t.Fatalf("unable to create multi algorithm signer SHA256: %v", err)
}
// Algorithms are in order of preference, we expect rsa-sha2-512 to be used.
multiAlgoSignerSHA512, err := NewSignerWithAlgorithms(algorithmSigner, []string{KeyAlgoRSASHA512, KeyAlgoRSASHA256})
if err != nil {
t.Fatalf("unable to create multi algorithm signer SHA512: %v", err)
}

var testVars = []struct {
name string
signer Signer
Expand All @@ -251,8 +295,10 @@ func TestCertTypes(t *testing.T) {
{CertAlgoECDSA384v01, testSigners["ecdsap384"], ""},
{CertAlgoECDSA521v01, testSigners["ecdsap521"], ""},
{CertAlgoED25519v01, testSigners["ed25519"], ""},
{CertAlgoRSAv01, testSigners["rsa"], KeyAlgoRSASHA512},
{CertAlgoRSAv01, testSigners["rsa"], KeyAlgoRSASHA256},
{"legacyRSASigner", &legacyRSASigner{testSigners["rsa"]}, KeyAlgoRSA},
{"multiAlgoRSASignerSHA256", multiAlgoSignerSHA256, KeyAlgoRSASHA256},
{"multiAlgoRSASignerSHA512", multiAlgoSignerSHA512, KeyAlgoRSASHA512},
{CertAlgoDSAv01, testSigners["dsa"], ""},
}

Expand Down Expand Up @@ -318,3 +364,45 @@ func TestCertTypes(t *testing.T) {
})
}
}

func TestCertSignWithMultiAlgorithmSigner(t *testing.T) {
type testcase struct {
sigAlgo string
algoritms []string
}
cases := []testcase{
{
sigAlgo: KeyAlgoRSA,
algoritms: []string{KeyAlgoRSA, KeyAlgoRSASHA512},
},
{
sigAlgo: KeyAlgoRSASHA256,
algoritms: []string{KeyAlgoRSASHA256, KeyAlgoRSA, KeyAlgoRSASHA512},
},
{
sigAlgo: KeyAlgoRSASHA512,
algoritms: []string{KeyAlgoRSASHA512, KeyAlgoRSASHA256},
},
}

cert := &Certificate{
Key: testPublicKeys["rsa"],
ValidBefore: CertTimeInfinity,
CertType: UserCert,
}

for _, c := range cases {
t.Run(c.sigAlgo, func(t *testing.T) {
signer, err := NewSignerWithAlgorithms(testSigners["rsa"].(AlgorithmSigner), c.algoritms)
if err != nil {
t.Fatalf("NewSignerWithAlgorithms error: %v", err)
}
if err := cert.SignCert(rand.Reader, signer); err != nil {
t.Fatalf("SignCert error: %v", err)
}
if cert.Signature.Format != c.sigAlgo {
t.Fatalf("got signature format %q, want %q", cert.Signature.Format, c.sigAlgo)
}
})
}
}
96 changes: 66 additions & 30 deletions ssh/client_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ func (c *connection) clientAuthenticate(config *ClientConfig) error {
for auth := AuthMethod(new(noneAuth)); auth != nil; {
ok, methods, err := auth.auth(sessionID, config.User, c.transport, config.Rand, extensions)
if err != nil {
return err
// We return the error later if there is no other method left to
// try.
ok = authFailure
}
if ok == authSuccess {
// success
Expand Down Expand Up @@ -101,6 +103,12 @@ func (c *connection) clientAuthenticate(config *ClientConfig) error {
}
}
}

if auth == nil && err != nil {
// We have an error and there are no other authentication methods to
// try, so we return it.
return err
}
}
return fmt.Errorf("ssh: unable to authenticate, attempted methods %v, no supported methods remain", tried)
}
Expand Down Expand Up @@ -217,21 +225,45 @@ func (cb publicKeyCallback) method() string {
return "publickey"
}

func pickSignatureAlgorithm(signer Signer, extensions map[string][]byte) (as AlgorithmSigner, algo string) {
func pickSignatureAlgorithm(signer Signer, extensions map[string][]byte) (MultiAlgorithmSigner, string, error) {
var as MultiAlgorithmSigner
keyFormat := signer.PublicKey().Type()

// Like in sendKexInit, if the public key implements AlgorithmSigner we
// assume it supports all algorithms, otherwise only the key format one.
as, ok := signer.(AlgorithmSigner)
if !ok {
return algorithmSignerWrapper{signer}, keyFormat
// If the signer implements MultiAlgorithmSigner we use the algorithms it
// support, if it implements AlgorithmSigner we assume it supports all
// algorithms, otherwise only the key format one.
switch s := signer.(type) {
case MultiAlgorithmSigner:
as = s
case AlgorithmSigner:
as = &multiAlgorithmSigner{
AlgorithmSigner: s,
supportedAlgorithms: algorithmsForKeyFormat(underlyingAlgo(keyFormat)),
}
default:
as = &multiAlgorithmSigner{
AlgorithmSigner: algorithmSignerWrapper{signer},
supportedAlgorithms: []string{underlyingAlgo(keyFormat)},
}
}

getFallbackAlgo := func() (string, error) {
// Fallback to use if there is no "server-sig-algs" extension or a
// common algorithm cannot be found. We use the public key format if the
// MultiAlgorithmSigner supports it, otherwise we return an error.
if !contains(as.Algorithms(), underlyingAlgo(keyFormat)) {
return "", fmt.Errorf("ssh: no common public key signature algorithm, server only supports %q for key type %q, signer only supports %v",
underlyingAlgo(keyFormat), keyFormat, as.Algorithms())
}
return keyFormat, nil
}

extPayload, ok := extensions["server-sig-algs"]
if !ok {
// If there is no "server-sig-algs" extension, fall back to the key
// format algorithm.
return as, keyFormat
// If there is no "server-sig-algs" extension use the fallback
// algorithm.
algo, err := getFallbackAlgo()
return as, algo, err
}

// The server-sig-algs extension only carries underlying signature
Expand All @@ -245,15 +277,22 @@ func pickSignatureAlgorithm(signer Signer, extensions map[string][]byte) (as Alg
}
}

keyAlgos := algorithmsForKeyFormat(keyFormat)
// Filter algorithms based on those supported by MultiAlgorithmSigner.
var keyAlgos []string
for _, algo := range algorithmsForKeyFormat(keyFormat) {
if contains(as.Algorithms(), underlyingAlgo(algo)) {
keyAlgos = append(keyAlgos, algo)
}
}

algo, err := findCommon("public key signature algorithm", keyAlgos, serverAlgos)
if err != nil {
// If there is no overlap, try the key anyway with the key format
// algorithm, to support servers that fail to list all supported
// algorithms.
return as, keyFormat
// If there is no overlap, return the fallback algorithm to support
// servers that fail to list all supported algorithms.
algo, err := getFallbackAlgo()
return as, algo, err
}
return as, algo
return as, algo, nil
}

func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand io.Reader, extensions map[string][]byte) (authResult, []string, error) {
Expand All @@ -267,10 +306,17 @@ func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand
return authFailure, nil, err
}
var methods []string
var errSigAlgo error
for _, signer := range signers {
pub := signer.PublicKey()
as, algo := pickSignatureAlgorithm(signer, extensions)

as, algo, err := pickSignatureAlgorithm(signer, extensions)
if err != nil && errSigAlgo == nil {
// If we cannot negotiate a signature algorithm store the first
// error so we can return it to provide a more meaningful message if
// no other signers work.
errSigAlgo = err
continue
}
ok, err := validateKey(pub, algo, user, c)
if err != nil {
return authFailure, nil, err
Expand Down Expand Up @@ -317,22 +363,12 @@ func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand
// contain the "publickey" method, do not attempt to authenticate with any
// other keys. According to RFC 4252 Section 7, the latter can occur when
// additional authentication methods are required.
if success == authSuccess || !containsMethod(methods, cb.method()) {
if success == authSuccess || !contains(methods, cb.method()) {
return success, methods, err
}
}

return authFailure, methods, nil
}

func containsMethod(methods []string, method string) bool {
for _, m := range methods {
if m == method {
return true
}
}

return false
return authFailure, methods, errSigAlgo
}

// validateKey validates the key provided is acceptable to the server.
Expand Down
Loading

0 comments on commit 28c53ff

Please sign in to comment.