Skip to content

Commit 28c53ff

Browse files
drakkangopherbot
drakkan
authored and
gopherbot
committed
ssh: add MultiAlgorithmSigner
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 <[email protected]> Run-TryBot: Filippo Valsorda <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Auto-Submit: Filippo Valsorda <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]>
1 parent 3f0842a commit 28c53ff

File tree

9 files changed

+554
-66
lines changed

9 files changed

+554
-66
lines changed

‎ssh/certs.go‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,9 @@ import (
1616

1717
// Certificate algorithm names from [PROTOCOL.certkeys]. These values can appear
1818
// in Certificate.Type, PublicKey.Type, and ClientConfig.HostKeyAlgorithms.
19-
// Unlike key algorithm names, these are not passed to AlgorithmSigner and don't
20-
// appear in the Signature.Format field.
19+
// Unlike key algorithm names, these are not passed to AlgorithmSigner nor
20+
// returned by MultiAlgorithmSigner and don't appear in the Signature.Format
21+
// field.
2122
const (
2223
CertAlgoRSAv01 = "[email protected]"
2324
CertAlgoDSAv01 = "[email protected]"
@@ -255,10 +256,17 @@ func NewCertSigner(cert *Certificate, signer Signer) (Signer, error) {
255256
return nil, errors.New("ssh: signer and cert have different public key")
256257
}
257258

258-
if algorithmSigner, ok := signer.(AlgorithmSigner); ok {
259+
switch s := signer.(type) {
260+
case MultiAlgorithmSigner:
261+
return &multiAlgorithmSigner{
262+
AlgorithmSigner: &algorithmOpenSSHCertSigner{
263+
&openSSHCertSigner{cert, signer}, s},
264+
supportedAlgorithms: s.Algorithms(),
265+
}, nil
266+
case AlgorithmSigner:
259267
return &algorithmOpenSSHCertSigner{
260-
&openSSHCertSigner{cert, signer}, algorithmSigner}, nil
261-
} else {
268+
&openSSHCertSigner{cert, signer}, s}, nil
269+
default:
262270
return &openSSHCertSigner{cert, signer}, nil
263271
}
264272
}
@@ -432,16 +440,30 @@ func (c *CertChecker) CheckCert(principal string, cert *Certificate) error {
432440
}
433441

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

443-
// Default to KeyAlgoRSASHA512 for ssh-rsa signers.
444-
if v, ok := authority.(AlgorithmSigner); ok && v.PublicKey().Type() == KeyAlgoRSA {
453+
if v, ok := authority.(MultiAlgorithmSigner); ok {
454+
if len(v.Algorithms()) == 0 {
455+
return errors.New("the provided authority has no signature algorithm")
456+
}
457+
// Use the first algorithm in the list.
458+
sig, err := v.SignWithAlgorithm(rand, c.bytesForSigning(), v.Algorithms()[0])
459+
if err != nil {
460+
return err
461+
}
462+
c.Signature = sig
463+
return nil
464+
} else if v, ok := authority.(AlgorithmSigner); ok && v.PublicKey().Type() == KeyAlgoRSA {
465+
// Default to KeyAlgoRSASHA512 for ssh-rsa signers.
466+
// TODO: consider using KeyAlgoRSASHA256 as default.
445467
sig, err := v.SignWithAlgorithm(rand, c.bytesForSigning(), KeyAlgoRSASHA512)
446468
if err != nil {
447469
return err

‎ssh/certs_test.go‎

Lines changed: 94 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -187,10 +187,30 @@ func TestHostKeyCert(t *testing.T) {
187187
}
188188

189189
for _, test := range []struct {
190-
addr string
191-
succeed bool
190+
addr string
191+
succeed bool
192+
certSignerAlgorithms []string // Empty means no algorithm restrictions.
193+
clientHostKeyAlgorithms []string
192194
}{
193195
{addr: "hostname:22", succeed: true},
196+
{
197+
addr: "hostname:22",
198+
succeed: true,
199+
certSignerAlgorithms: []string{KeyAlgoRSASHA256, KeyAlgoRSASHA512},
200+
clientHostKeyAlgorithms: []string{CertAlgoRSASHA512v01},
201+
},
202+
{
203+
addr: "hostname:22",
204+
succeed: false,
205+
certSignerAlgorithms: []string{KeyAlgoRSASHA256, KeyAlgoRSASHA512},
206+
clientHostKeyAlgorithms: []string{CertAlgoRSAv01},
207+
},
208+
{
209+
addr: "hostname:22",
210+
succeed: false,
211+
certSignerAlgorithms: []string{KeyAlgoRSASHA256, KeyAlgoRSASHA512},
212+
clientHostKeyAlgorithms: []string{KeyAlgoRSASHA512}, // Not a certificate algorithm.
213+
},
194214
{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'
195215
{addr: "lasthost:22", succeed: false},
196216
} {
@@ -207,14 +227,24 @@ func TestHostKeyCert(t *testing.T) {
207227
conf := ServerConfig{
208228
NoClientAuth: true,
209229
}
210-
conf.AddHostKey(certSigner)
230+
if len(test.certSignerAlgorithms) > 0 {
231+
mas, err := NewSignerWithAlgorithms(certSigner.(AlgorithmSigner), test.certSignerAlgorithms)
232+
if err != nil {
233+
errc <- err
234+
return
235+
}
236+
conf.AddHostKey(mas)
237+
} else {
238+
conf.AddHostKey(certSigner)
239+
}
211240
_, _, _, err := NewServerConn(c1, &conf)
212241
errc <- err
213242
}()
214243

215244
config := &ClientConfig{
216-
User: "user",
217-
HostKeyCallback: checker.CheckHostKey,
245+
User: "user",
246+
HostKeyCallback: checker.CheckHostKey,
247+
HostKeyAlgorithms: test.clientHostKeyAlgorithms,
218248
}
219249
_, _, _, err = NewClientConn(c2, test.addr, config)
220250

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

244274
func TestCertTypes(t *testing.T) {
275+
algorithmSigner, ok := testSigners["rsa"].(AlgorithmSigner)
276+
if !ok {
277+
t.Fatal("rsa test signer does not implement the AlgorithmSigner interface")
278+
}
279+
multiAlgoSignerSHA256, err := NewSignerWithAlgorithms(algorithmSigner, []string{KeyAlgoRSASHA256})
280+
if err != nil {
281+
t.Fatalf("unable to create multi algorithm signer SHA256: %v", err)
282+
}
283+
// Algorithms are in order of preference, we expect rsa-sha2-512 to be used.
284+
multiAlgoSignerSHA512, err := NewSignerWithAlgorithms(algorithmSigner, []string{KeyAlgoRSASHA512, KeyAlgoRSASHA256})
285+
if err != nil {
286+
t.Fatalf("unable to create multi algorithm signer SHA512: %v", err)
287+
}
288+
245289
var testVars = []struct {
246290
name string
247291
signer Signer
@@ -251,8 +295,10 @@ func TestCertTypes(t *testing.T) {
251295
{CertAlgoECDSA384v01, testSigners["ecdsap384"], ""},
252296
{CertAlgoECDSA521v01, testSigners["ecdsap521"], ""},
253297
{CertAlgoED25519v01, testSigners["ed25519"], ""},
254-
{CertAlgoRSAv01, testSigners["rsa"], KeyAlgoRSASHA512},
298+
{CertAlgoRSAv01, testSigners["rsa"], KeyAlgoRSASHA256},
255299
{"legacyRSASigner", &legacyRSASigner{testSigners["rsa"]}, KeyAlgoRSA},
300+
{"multiAlgoRSASignerSHA256", multiAlgoSignerSHA256, KeyAlgoRSASHA256},
301+
{"multiAlgoRSASignerSHA512", multiAlgoSignerSHA512, KeyAlgoRSASHA512},
256302
{CertAlgoDSAv01, testSigners["dsa"], ""},
257303
}
258304

@@ -318,3 +364,45 @@ func TestCertTypes(t *testing.T) {
318364
})
319365
}
320366
}
367+
368+
func TestCertSignWithMultiAlgorithmSigner(t *testing.T) {
369+
type testcase struct {
370+
sigAlgo string
371+
algoritms []string
372+
}
373+
cases := []testcase{
374+
{
375+
sigAlgo: KeyAlgoRSA,
376+
algoritms: []string{KeyAlgoRSA, KeyAlgoRSASHA512},
377+
},
378+
{
379+
sigAlgo: KeyAlgoRSASHA256,
380+
algoritms: []string{KeyAlgoRSASHA256, KeyAlgoRSA, KeyAlgoRSASHA512},
381+
},
382+
{
383+
sigAlgo: KeyAlgoRSASHA512,
384+
algoritms: []string{KeyAlgoRSASHA512, KeyAlgoRSASHA256},
385+
},
386+
}
387+
388+
cert := &Certificate{
389+
Key: testPublicKeys["rsa"],
390+
ValidBefore: CertTimeInfinity,
391+
CertType: UserCert,
392+
}
393+
394+
for _, c := range cases {
395+
t.Run(c.sigAlgo, func(t *testing.T) {
396+
signer, err := NewSignerWithAlgorithms(testSigners["rsa"].(AlgorithmSigner), c.algoritms)
397+
if err != nil {
398+
t.Fatalf("NewSignerWithAlgorithms error: %v", err)
399+
}
400+
if err := cert.SignCert(rand.Reader, signer); err != nil {
401+
t.Fatalf("SignCert error: %v", err)
402+
}
403+
if cert.Signature.Format != c.sigAlgo {
404+
t.Fatalf("got signature format %q, want %q", cert.Signature.Format, c.sigAlgo)
405+
}
406+
})
407+
}
408+
}

‎ssh/client_auth.go‎

Lines changed: 66 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,9 @@ func (c *connection) clientAuthenticate(config *ClientConfig) error {
7171
for auth := AuthMethod(new(noneAuth)); auth != nil; {
7272
ok, methods, err := auth.auth(sessionID, config.User, c.transport, config.Rand, extensions)
7373
if err != nil {
74-
return err
74+
// We return the error later if there is no other method left to
75+
// try.
76+
ok = authFailure
7577
}
7678
if ok == authSuccess {
7779
// success
@@ -101,6 +103,12 @@ func (c *connection) clientAuthenticate(config *ClientConfig) error {
101103
}
102104
}
103105
}
106+
107+
if auth == nil && err != nil {
108+
// We have an error and there are no other authentication methods to
109+
// try, so we return it.
110+
return err
111+
}
104112
}
105113
return fmt.Errorf("ssh: unable to authenticate, attempted methods %v, no supported methods remain", tried)
106114
}
@@ -217,21 +225,45 @@ func (cb publicKeyCallback) method() string {
217225
return "publickey"
218226
}
219227

220-
func pickSignatureAlgorithm(signer Signer, extensions map[string][]byte) (as AlgorithmSigner, algo string) {
228+
func pickSignatureAlgorithm(signer Signer, extensions map[string][]byte) (MultiAlgorithmSigner, string, error) {
229+
var as MultiAlgorithmSigner
221230
keyFormat := signer.PublicKey().Type()
222231

223-
// Like in sendKexInit, if the public key implements AlgorithmSigner we
224-
// assume it supports all algorithms, otherwise only the key format one.
225-
as, ok := signer.(AlgorithmSigner)
226-
if !ok {
227-
return algorithmSignerWrapper{signer}, keyFormat
232+
// If the signer implements MultiAlgorithmSigner we use the algorithms it
233+
// support, if it implements AlgorithmSigner we assume it supports all
234+
// algorithms, otherwise only the key format one.
235+
switch s := signer.(type) {
236+
case MultiAlgorithmSigner:
237+
as = s
238+
case AlgorithmSigner:
239+
as = &multiAlgorithmSigner{
240+
AlgorithmSigner: s,
241+
supportedAlgorithms: algorithmsForKeyFormat(underlyingAlgo(keyFormat)),
242+
}
243+
default:
244+
as = &multiAlgorithmSigner{
245+
AlgorithmSigner: algorithmSignerWrapper{signer},
246+
supportedAlgorithms: []string{underlyingAlgo(keyFormat)},
247+
}
248+
}
249+
250+
getFallbackAlgo := func() (string, error) {
251+
// Fallback to use if there is no "server-sig-algs" extension or a
252+
// common algorithm cannot be found. We use the public key format if the
253+
// MultiAlgorithmSigner supports it, otherwise we return an error.
254+
if !contains(as.Algorithms(), underlyingAlgo(keyFormat)) {
255+
return "", fmt.Errorf("ssh: no common public key signature algorithm, server only supports %q for key type %q, signer only supports %v",
256+
underlyingAlgo(keyFormat), keyFormat, as.Algorithms())
257+
}
258+
return keyFormat, nil
228259
}
229260

230261
extPayload, ok := extensions["server-sig-algs"]
231262
if !ok {
232-
// If there is no "server-sig-algs" extension, fall back to the key
233-
// format algorithm.
234-
return as, keyFormat
263+
// If there is no "server-sig-algs" extension use the fallback
264+
// algorithm.
265+
algo, err := getFallbackAlgo()
266+
return as, algo, err
235267
}
236268

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

248-
keyAlgos := algorithmsForKeyFormat(keyFormat)
280+
// Filter algorithms based on those supported by MultiAlgorithmSigner.
281+
var keyAlgos []string
282+
for _, algo := range algorithmsForKeyFormat(keyFormat) {
283+
if contains(as.Algorithms(), underlyingAlgo(algo)) {
284+
keyAlgos = append(keyAlgos, algo)
285+
}
286+
}
287+
249288
algo, err := findCommon("public key signature algorithm", keyAlgos, serverAlgos)
250289
if err != nil {
251-
// If there is no overlap, try the key anyway with the key format
252-
// algorithm, to support servers that fail to list all supported
253-
// algorithms.
254-
return as, keyFormat
290+
// If there is no overlap, return the fallback algorithm to support
291+
// servers that fail to list all supported algorithms.
292+
algo, err := getFallbackAlgo()
293+
return as, algo, err
255294
}
256-
return as, algo
295+
return as, algo, nil
257296
}
258297

259298
func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand io.Reader, extensions map[string][]byte) (authResult, []string, error) {
@@ -267,10 +306,17 @@ func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand
267306
return authFailure, nil, err
268307
}
269308
var methods []string
309+
var errSigAlgo error
270310
for _, signer := range signers {
271311
pub := signer.PublicKey()
272-
as, algo := pickSignatureAlgorithm(signer, extensions)
273-
312+
as, algo, err := pickSignatureAlgorithm(signer, extensions)
313+
if err != nil && errSigAlgo == nil {
314+
// If we cannot negotiate a signature algorithm store the first
315+
// error so we can return it to provide a more meaningful message if
316+
// no other signers work.
317+
errSigAlgo = err
318+
continue
319+
}
274320
ok, err := validateKey(pub, algo, user, c)
275321
if err != nil {
276322
return authFailure, nil, err
@@ -317,22 +363,12 @@ func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand
317363
// contain the "publickey" method, do not attempt to authenticate with any
318364
// other keys. According to RFC 4252 Section 7, the latter can occur when
319365
// additional authentication methods are required.
320-
if success == authSuccess || !containsMethod(methods, cb.method()) {
366+
if success == authSuccess || !contains(methods, cb.method()) {
321367
return success, methods, err
322368
}
323369
}
324370

325-
return authFailure, methods, nil
326-
}
327-
328-
func containsMethod(methods []string, method string) bool {
329-
for _, m := range methods {
330-
if m == method {
331-
return true
332-
}
333-
}
334-
335-
return false
371+
return authFailure, methods, errSigAlgo
336372
}
337373

338374
// validateKey validates the key provided is acceptable to the server.

0 commit comments

Comments
 (0)