[crypto] ssh: support rsa-sha2-256/512 for client authentication (client-side)
272 views
Skip to first unread message
Filippo Valsorda (Gerrit)
Mar 14, 2022, 10:54:10 AM3/14/22
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Filippo Valsorda would like Roland Shoemaker to review this change.
ssh: support rsa-sha2-256/512 for client authentication (client-side)
Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.
Changed the type in TestClientAuthErrorList just so we wouldn't get 3
errors for one key.
Fixes golang/go#39885
For golang/go#49952
Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8
---
M ssh/client_auth.go
M ssh/client_auth_test.go
M ssh/common.go
M ssh/server.go
4 files changed, 115 insertions(+), 55 deletions(-)
diff --git a/ssh/client_auth.go b/ssh/client_auth.go
index c611aeb..f5a7970 100644
--- a/ssh/client_auth.go
+++ b/ssh/client_auth.go
@@ -201,54 +201,82 @@
}
var methods []string
for _, signer := range signers {
- ok, err := validateKey(signer.PublicKey(), user, c)
- if err != nil {
- return authFailure, nil, err
- }
- if !ok {
- continue
- }
-
pub := signer.PublicKey()
- pubKey := pub.Marshal()
- sign, err := signer.Sign(rand, buildDataSignedForAuth(session, userAuthRequestMsg{
- User: user,
- Service: serviceSSH,
- Method: cb.method(),
- }, []byte(pub.Type()), pubKey))
- if err != nil {
- return authFailure, nil, err
+
+ // Like in sendKexInit, if the public key implements AlgorithmSigner we
+ // assume it supports all algorithms, otherwise only the key format one.
+ var algorithms []string
+ as, ok := signer.(AlgorithmSigner)
+ if ok {
+ algorithms = algorithmsForKeyFormat(pub.Type())
+ } else {
+ as = algorithmSignerWrapper{signer}
+ algorithms = []string{pub.Type()}
}
- // manually wrap the serialized signature in a string
- s := Marshal(sign)
- sig := make([]byte, stringLength(len(s)))
- marshalString(sig, s)
- msg := publickeyAuthMsg{
- User: user,
- Service: serviceSSH,
- Method: cb.method(),
- HasSig: true,
- Algoname: pub.Type(),
- PubKey: pubKey,
- Sig: sig,
- }
- p := Marshal(&msg)
- if err := c.writePacket(p); err != nil {
- return authFailure, nil, err
- }
- var success authResult
- success, methods, err = handleAuthResponse(c)
- if err != nil {
- return authFailure, nil, err
- }
+ // We don't implement RFC 8308 extensions, somewhat intentionally
+ // (because they involve a speculative read), which means we can't use
+ // the "server-sig-algs" extension to learn about the server's supported
+ // algorithms. The concern with trial and error, besides the
+ // round-trips, is that the server might react poorly upon seeing an
+ // unsupported algorithm. OpenSSH has supported the only "additional"
+ // algorithms we support (the rsa-sha2 ones) since 2016. Feels like it's
+ // fine to default to them. If an application doesn't want that, they
+ // can wrap the HostKey in a Signer without SignWithAlgorithm.
+ for _, algo := range algorithms {
+ ok, err := validateKey(pub, algo, user, c)
+ if err != nil {
+ return authFailure, nil, err
+ }
+ if !ok {
+ continue
+ }
- // If authentication succeeds or the list of available methods does not
- // 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()) {
- return success, methods, err
+ pubKey := pub.Marshal()
+ data := buildDataSignedForAuth(session, userAuthRequestMsg{
+ User: user,
+ Service: serviceSSH,
+ Method: cb.method(),
+ }, algo, pubKey)
+ underlyingAlgo := algo
+ if keyAlgo, ok := certKeyAlgoNames[underlyingAlgo]; ok {
+ underlyingAlgo = keyAlgo
+ }
+ sign, err := as.SignWithAlgorithm(rand, data, underlyingAlgo)
+ if err != nil {
+ return authFailure, nil, err
+ }
+
+ // manually wrap the serialized signature in a string
+ s := Marshal(sign)
+ sig := make([]byte, stringLength(len(s)))
+ marshalString(sig, s)
+ msg := publickeyAuthMsg{
+ User: user,
+ Service: serviceSSH,
+ Method: cb.method(),
+ HasSig: true,
+ Algoname: algo,
+ PubKey: pubKey,
+ Sig: sig,
+ }
+ p := Marshal(&msg)
+ if err := c.writePacket(p); err != nil {
+ return authFailure, nil, err
+ }
+ var success authResult
+ success, methods, err = handleAuthResponse(c)
+ if err != nil {
+ return authFailure, nil, err
+ }
+
+ // If authentication succeeds or the list of available methods does not
+ // 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()) {
+ return success, methods, err
+ }
}
}
@@ -266,26 +294,25 @@
}
// validateKey validates the key provided is acceptable to the server.
-func validateKey(key PublicKey, user string, c packetConn) (bool, error) {
+func validateKey(key PublicKey, algo string, user string, c packetConn) (bool, error) {
pubKey := key.Marshal()
msg := publickeyAuthMsg{
User: user,
Service: serviceSSH,
Method: "publickey",
HasSig: false,
- Algoname: key.Type(),
+ Algoname: algo,
PubKey: pubKey,
}
if err := c.writePacket(Marshal(&msg)); err != nil {
return false, err
}
- return confirmKeyAck(key, c)
+ return confirmKeyAck(key, algo, c)
}
-func confirmKeyAck(key PublicKey, c packetConn) (bool, error) {
+func confirmKeyAck(key PublicKey, algo string, c packetConn) (bool, error) {
pubKey := key.Marshal()
- algoname := key.Type()
for {
packet, err := c.readPacket()
@@ -302,7 +329,7 @@
if err := Unmarshal(packet, &msg); err != nil {
return false, err
}
- if msg.Algo != algoname || !bytes.Equal(msg.PubKey, pubKey) {
+ if msg.Algo != algo || !bytes.Equal(msg.PubKey, pubKey) {
return false, nil
}
return true, nil
diff --git a/ssh/client_auth_test.go b/ssh/client_auth_test.go
index f73079b..c921e4b 100644
--- a/ssh/client_auth_test.go
+++ b/ssh/client_auth_test.go
@@ -118,6 +118,20 @@
}
}
+// TestClientAuthNoSHA2 tests a ssh-rsa Signer that doesn't implement AlgorithmSigner.
+func TestClientAuthNoSHA2(t *testing.T) {
+ config := &ClientConfig{
+ User: "testuser",
+ Auth: []AuthMethod{
+ PublicKeys(&legacyRSASigner{testSigners["rsa"]}),
+ },
+ HostKeyCallback: InsecureIgnoreHostKey(),
+ }
+ if err := tryAuth(t, config); err != nil {
+ t.Fatalf("unable to dial remote side: %s", err)
+ }
+}
+
func TestAuthMethodPassword(t *testing.T) {
config := &ClientConfig{
User: "testuser",
@@ -659,7 +673,7 @@
clientConfig := &ClientConfig{
Auth: []AuthMethod{
- PublicKeys(testSigners["rsa"]),
+ PublicKeys(testSigners["ecdsa"]),
},
HostKeyCallback: InsecureIgnoreHostKey(),
}
diff --git a/ssh/common.go b/ssh/common.go
index d6d9bf9..2a47a61 100644
--- a/ssh/common.go
+++ b/ssh/common.go
@@ -297,8 +297,9 @@
}
// buildDataSignedForAuth returns the data that is signed in order to prove
-// possession of a private key. See RFC 4252, section 7.
-func buildDataSignedForAuth(sessionID []byte, req userAuthRequestMsg, algo, pubKey []byte) []byte {
+// possession of a private key. See RFC 4252, section 7. algo is the advertised
+// algorithm, and may be a certificate type.
+func buildDataSignedForAuth(sessionID []byte, req userAuthRequestMsg, algo string, pubKey []byte) []byte {
data := struct {
Session []byte
Type byte
@@ -306,7 +307,7 @@
Service string
Method string
Sign bool
- Algo []byte
+ Algo string
PubKey []byte
}{
sessionID,
diff --git a/ssh/server.go b/ssh/server.go
index c743e9e..39764ae 100644
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -568,7 +568,7 @@
authErr = fmt.Errorf("ssh: algorithm %q not accepted", sig.Format)
break
}
- signedData := buildDataSignedForAuth(sessionID, userAuthReq, algoBytes, pubKeyData)
+ signedData := buildDataSignedForAuth(sessionID, userAuthReq, algo, pubKeyData)
if err := pubKey.Verify(signedData, sig); err != nil {
return nil, err
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 11:22:38 AM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Filippo Valsorda.
Filippo Valsorda uploaded patch set #2 to this change.
ssh: support rsa-sha2-256/512 for client authentication
Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.
The server side already worked, as long as the client selected one of
the SHA-2 algorithms, because the signature flowed freely to Verify.
There was however nothing verifying that the signature algorithm matched
the advertised one. The comment suggested the check was being performed,
but it got lost back in CL 86190043. Not a security issue because the
signature had to pass the callback's Verify method regardless, and both
values were checked to be acceptable.
Changed the type in TestClientAuthErrorList just so we wouldn't get 3
errors for one key.
Fixes golang/go#39885
For golang/go#49952
Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8
---
M ssh/client_auth.go
M ssh/client_auth_test.go
M ssh/common.go
M ssh/server.go
4 files changed, 145 insertions(+), 56 deletions(-)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Roman Mohr (Gerrit)
Mar 14, 2022, 12:06:37 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Gopher Robot, Roland Shoemaker, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Filippo Valsorda.
1 comment:
File ssh/client_auth.go:
Patch Set #1, Line 213:
as = algorithmSignerWrapper{signer}Does this cover ssh agent provided keys?
I added something like this to the agent signer:
```
func (s *agentKeyringSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*ssh.Signature, error) {
// The agent has its own entropy source, so the rand argument is ignored.
if s.PublicKey().Type() != ssh.SigAlgoRSA {
return nil, fmt.Errorf("public key must be of type ssh-rsa, but got %v", s.PublicKey().Type())
}switch algorithm {
case ssh.SigAlgoRSASHA2256:
return s.agent.SignWithFlags(s.PublicKey(), data, SignatureFlagRsaSha256)
case ssh.SigAlgoRSASHA2512:
return s.agent.SignWithFlags(s.PublicKey(), data, SignatureFlagRsaSha512)
}
return nil, fmt.Errorf("algorithm not supported")
}
```maybe it is helpful.
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Roman Mohr (Gerrit)
Mar 14, 2022, 12:06:37 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Gopher Robot, Roland Shoemaker, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Filippo Valsorda.
2 comments:
Patchset:
I just wanted to ensure that ssh agent signers are not overlooked, you probably catch them there :)
File ssh/client_auth.go:
Patch Set #1, Line 213:
as = algorithmSignerWrapper{signer}Does this cover ssh agent provided keys?
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 1:04:41 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Gopher Robot, Roman Mohr, Roland Shoemaker, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Roman Mohr.
1 comment:
File ssh/client_auth.go:
Patch Set #1, Line 213:
as = algorithmSignerWrapper{signer}
I added something like this to the agent signer: […]
Thanks for pointing it out. Turns out that those signers implement the wrong interface, which was changed during the review of CL 123955.
Fixed. (Note that SignWithAlgorithm must support also non-RSA algorithms.)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 1:04:41 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Roman Mohr.
Filippo Valsorda uploaded patch set #3 to this change.
ssh: support rsa-sha2-256/512 for client authentication
Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.
The server side already worked, as long as the client selected one of
the SHA-2 algorithms, because the signature flowed freely to Verify.
There was however nothing verifying that the signature algorithm matched
the advertised one. The comment suggested the check was being performed,
but it got lost back in CL 86190043. Not a security issue because the
signature had to pass the callback's Verify method regardless, and both
values were checked to be acceptable.
The Signers returned by ssh/agent (when backed by an agent client)
didn't actually implement AlgorithmSigner but ParameterizedSigner, an
interface defined in an earlier version of CL 123955.
Changed the type in TestClientAuthErrorList just so we wouldn't get 3
errors for one key.
Fixes golang/go#39885
For golang/go#49952
Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8
---
M ssh/agent/client.go
M ssh/client_auth.go
M ssh/client_auth_test.go
M ssh/common.go
M ssh/server.go
5 files changed, 165 insertions(+), 66 deletions(-)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 1:11:23 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Roman Mohr.
Filippo Valsorda uploaded patch set #4 to this change.
ssh: support rsa-sha2-256/512 for client authentication
CL 220037 had implemented support for host authentication using
rsa-sha2-256/512, but not client public key authentication. OpenSSH
disabled the SHA-1 based ssh-rsa by default in version 8.8 (after
pre-announcing it in versions 8.2, 8.3, 8.4, 8.5, 8.6, and 8.7) although
some distributions re-enable it. GitHub will start rejecting ssh-rsa for
keys uploaded before November 2, 2021 on March 15, 2022.
https://github.blog/2021-09-01-improving-git-protocol-security-github/
The server side already worked, as long as the client selected one of
the SHA-2 algorithms, because the signature flowed freely to Verify.
There was however nothing verifying that the signature algorithm matched
the advertised one. The comment suggested the check was being performed,
but it got lost back in CL 86190043. Not a security issue because the
signature had to pass the callback's Verify method regardless, and both
values were checked to be acceptable.
Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.
The Signers returned by ssh/agent (when backed by an agent client)
didn't actually implement AlgorithmSigner but ParameterizedSigner, an
interface defined in an earlier version of CL 123955.
Changed the type in TestClientAuthErrorList just so we wouldn't get 3
errors for one key.
Fixes golang/go#39885
For golang/go#49952
Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8
---
M ssh/agent/client.go
M ssh/client_auth.go
M ssh/client_auth_test.go
M ssh/common.go
M ssh/server.go
5 files changed, 174 insertions(+), 66 deletions(-)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 6:21:37 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Roman Mohr.
Filippo Valsorda uploaded patch set #5 to this change.
ssh: support rsa-sha2-256/512 for client authentication
CL 220037 had implemented support for host authentication using
rsa-sha2-256/512, but not client public key authentication. OpenSSH
disabled the SHA-1 based ssh-rsa by default in version 8.8 (after
pre-announcing it in versions 8.2, 8.3, 8.4, 8.5, 8.6, and 8.7) although
some distributions re-enable it. GitHub will start rejecting ssh-rsa for
keys uploaded before November 2, 2021 on March 15, 2022.
https://github.blog/2021-09-01-improving-git-protocol-security-github/
The server side already worked, as long as the client selected one of
the SHA-2 algorithms, because the signature flowed freely to Verify.
There was however nothing verifying that the signature algorithm matched
the advertised one. The comment suggested the check was being performed,
but it got lost back in CL 86190043. Not a security issue because the
signature had to pass the callback's Verify method regardless, and both
values were checked to be acceptable.
Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.
The Signers returned by ssh/agent (when backed by an agent client)
didn't actually implement AlgorithmSigner but ParameterizedSigner, an
interface defined in an earlier version of CL 123955.
Fixes golang/go#39885
For golang/go#49952
Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8
---
M ssh/agent/client.go
M ssh/client_auth.go
M ssh/client_auth_test.go
M ssh/common.go
M ssh/handshake.go
M ssh/messages.go
M ssh/server.go
7 files changed, 219 insertions(+), 36 deletions(-)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 6:22:23 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Roman Mohr.
Filippo Valsorda uploaded patch set #6 to this change.
ssh: support rsa-sha2-256/512 for client authentication
CL 220037 had implemented support for host authentication using
rsa-sha2-256/512, but not client public key authentication. OpenSSH
disabled the SHA-1 based ssh-rsa by default in version 8.8 (after
pre-announcing it in versions 8.2, 8.3, 8.4, 8.5, 8.6, and 8.7) although
some distributions re-enable it. GitHub will start rejecting ssh-rsa for
keys uploaded before November 2, 2021 on March 15, 2022.
https://github.blog/2021-09-01-improving-git-protocol-security-github/
The server side already worked, as long as the client selected one of
the SHA-2 algorithms, because the signature flowed freely to Verify.
There was however nothing verifying that the signature algorithm matched
the advertised one. The comment suggested the check was being performed,
but it got lost back in CL 86190043. Not a security issue because the
signature had to pass the callback's Verify method regardless, and both
values were checked to be acceptable.
Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.
The Signers returned by ssh/agent (when backed by an agent client)
didn't actually implement AlgorithmSigner but ParameterizedSigner, an
interface defined in an earlier version of CL 123955.
Updates golang/go#49269
Fixes golang/go#39885
For golang/go#49952
Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8
---
M ssh/agent/client.go
M ssh/client_auth.go
M ssh/client_auth_test.go
M ssh/common.go
M ssh/handshake.go
M ssh/messages.go
M ssh/server.go
7 files changed, 220 insertions(+), 36 deletions(-)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Mar 14, 2022, 7:07:16 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Gopher Robot, Roman Mohr, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda, Roman Mohr.
Patch set 6:Code-Review +2
7 comments:
Patchset:
Couple minor things, but otherwise looks good!
File ssh/agent/client.go:
Patch Set #6, Line 791:
var _ ssh.AlgorithmSigner = &agentKeyringSigner{}Is this necessary to register the hashes? or something else weird?
File ssh/client_auth.go:
Patch Set #6, Line 239:
algo, err := findCommon("public key signature algorithm", keyAlgos, serverAlgos)One possible issue here is that we'll always pick the first common algorithm. If for _whatever reason_ the first algorithm fails, but one of the other common algorithms would work, we will never try it. That might be fine though?
Patch Set #6, Line 292:
Algoname: algo,I don't think it does, but just to check, this doesn't need to use the `underlyingAlgo`?
Patch Set #6, Line 403:
case msgExtInfo:minor nit: might want to check that we only accept one of these, otherwise the server could just keep sending them forever (extremely unlikely but ¯\_(ツ)_/¯).
Patch Set #6, Line 489:
case msgExtInfo:ditto as above.
File ssh/handshake.go:
msg.KexAlgos = make([]string, 0, len(t.config.KeyExchanges)+1)
msg.KexAlgos = append(msg.KexAlgos, t.config.KeyExchanges...)I think this is already instantiated above around line 447, should just need to append the extension value.
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 7:28:24 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Roman Mohr.
Filippo Valsorda uploaded patch set #7 to this change.
7 files changed, 230 insertions(+), 36 deletions(-)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 7:28:25 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Roland Shoemaker, Gopher Robot, Roman Mohr, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker, Roman Mohr.
6 comments:
File ssh/agent/client.go:
Patch Set #6, Line 791:
var _ ssh.AlgorithmSigner = &agentKeyringSigner{}Is this necessary to register the hashes? or something else weird?
This is a compile-time assertion that *agentKeyringSigner implements ssh.AlgorithmSigner. If the methods were wrong or something, the assignment would fail.
File ssh/client_auth.go:
Patch Set #6, Line 239:
algo, err := findCommon("public key signature algorithm", keyAlgos, serverAlgos)
One possible issue here is that we'll always pick the first common algorithm. […]
I think that's the intended behavior. The alternatives is to try a failing key multiple times, which will burn through our precious few authentication attempts. We could leave the rest of the overlapping algorithms last, but that feels like too much complexity for little gain.
In practice, since the preference order is that of algorithmsForKeyFormat, this will pick rsa-sha2-256/512 if available, and ssh-rsa otherwise, which feels like the correct outcome. I expect no server will advertise support for rsa-sha2-256 and then reject it by policy (while I saw evidence of that being true sometimes for ssh-rsa).
Patch Set #6, Line 292:
Algoname: algo,I don't think it does, but just to check, this doesn't need to use the `underlyingAlgo`?
PROTOCOL.certkeys is scant on details, but it does say
These RSA/SHA-2 types should not appear in keys at rest or transmitted
on the wire, but do appear in a SSH_MSG_KEXINIT's host-key algorithms
field or in the "public key algorithm name" field of a "publickey"
SSH_USERAUTH_REQUEST to indicate that the signature will use the
specified algorithm.
referring to rsa-sha2-2...@openssh.com which is not an underlying algorithm.In general, the underlying algorithms are only used in sig.Format (and therefor in SignWithAlgorithm and in Verify).
Patch Set #6, Line 403:
case msgExtInfo:
minor nit: might want to check that we only accept one of these, otherwise the server could just kee […]
Wouldn't that be the same as sending banners forever? I guess the application's bannerCallback could limit those, supposedly.
Done.
Patch Set #6, Line 489:
case msgExtInfo:ditto as above.
Done
File ssh/handshake.go:
msg.KexAlgos = make([]string, 0, len(t.config.KeyExchanges)+1)
msg.KexAlgos = append(msg.KexAlgos, t.config.KeyExchanges...)
I think this is already instantiated above around line 447, should just need to append the extension […]
Yes, but we don't own the slice's backing array, since it's set in the Config by the application, so we can't append to it without making a copy. (Imagine it was set to someImportantLargerSlice[:len(someImportantLargerSlice)-2]. We'd overwrite it.)
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Mar 14, 2022, 7:32:01 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Gopher Robot, Roman Mohr, golang-co...@googlegroups.com
Attention is currently required from: Filippo Valsorda, Roman Mohr.
Patch set 7:Code-Review +2
3 comments:
File ssh/agent/client.go:
Patch Set #6, Line 791:
var _ ssh.AlgorithmSigner = &agentKeyringSigner{}
This is a compile-time assertion that *agentKeyringSigner implements ssh.AlgorithmSigner. […]
Oh I see, makes sense.
File ssh/client_auth.go:
Patch Set #6, Line 239:
algo, err := findCommon("public key signature algorithm", keyAlgos, serverAlgos)
I think that's the intended behavior. […]
Ack, probably the right choice.
File ssh/handshake.go:
msg.KexAlgos = make([]string, 0, len(t.config.KeyExchanges)+1)
msg.KexAlgos = append(msg.KexAlgos, t.config.KeyExchanges...)
Yes, but we don't own the slice's backing array, since it's set in the Config by the application, so […]
Bleh, right, lovely.
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Mar 14, 2022, 7:47:29 PM3/14/22
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Gopher Robot, Roland Shoemaker, Roman Mohr, golang-co...@googlegroups.com
Filippo Valsorda submitted this change.
Approvals:
Roland Shoemaker: Looks good to me, approved
Filippo Valsorda: Trusted; Run TryBots
Gopher Robot: TryBots succeeded
ssh: support rsa-sha2-256/512 for client authentication
CL 220037 had implemented support for host authentication using
rsa-sha2-256/512, but not client public key authentication. OpenSSH
disabled the SHA-1 based ssh-rsa by default in version 8.8 (after
pre-announcing it in versions 8.2, 8.3, 8.4, 8.5, 8.6, and 8.7) although
some distributions re-enable it. GitHub will start rejecting ssh-rsa for
keys uploaded before November 2, 2021 on March 15, 2022.
https://github.blog/2021-09-01-improving-git-protocol-security-github/
The server side already worked, as long as the client selected one of
the SHA-2 algorithms, because the signature flowed freely to Verify.
There was however nothing verifying that the signature algorithm matched
the advertised one. The comment suggested the check was being performed,
but it got lost back in CL 86190043. Not a security issue because the
signature had to pass the callback's Verify method regardless, and both
values were checked to be acceptable.
Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.
The Signers returned by ssh/agent (when backed by an agent client)
didn't actually implement AlgorithmSigner but ParameterizedSigner, an
interface defined in an earlier version of CL 123955.
Updates golang/go#49269
Fixes golang/go#39885
For golang/go#49952
Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/392394
Trust: Filippo Valsorda <fil...@golang.org>
Run-TryBot: Filippo Valsorda <fil...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Roland Shoemaker <rol...@golang.org>
---
M ssh/agent/client.go
M ssh/client_auth.go
M ssh/client_auth_test.go
M ssh/common.go
M ssh/handshake.go
M ssh/messages.go
M ssh/server.go
7 files changed, 235 insertions(+), 36 deletions(-)
diff --git a/ssh/agent/client.go b/ssh/agent/client.go
index b909471..3cfe723 100644
--- a/ssh/agent/client.go
+++ b/ssh/agent/client.go
@@ -25,7 +25,6 @@
"math/big"
"sync"
- "crypto"
"golang.org/x/crypto/ed25519"
"golang.org/x/crypto/ssh"
)
@@ -771,19 +770,26 @@
return s.agent.Sign(s.pub, data)
}
-func (s *agentKeyringSigner) SignWithOpts(rand io.Reader, data []byte, opts crypto.SignerOpts) (*ssh.Signature, error) {
- var flags SignatureFlags
- if opts != nil {
- switch opts.HashFunc() {
- case crypto.SHA256:
- flags = SignatureFlagRsaSha256
- case crypto.SHA512:
- flags = SignatureFlagRsaSha512
- }
+func (s *agentKeyringSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*ssh.Signature, error) {
+ if algorithm == "" || algorithm == s.pub.Type() {
+ return s.Sign(rand, data)
}
+
+ var flags SignatureFlags
+ switch algorithm {
+ case ssh.KeyAlgoRSASHA256:
+ flags = SignatureFlagRsaSha256
+ case ssh.KeyAlgoRSASHA512:
+ flags = SignatureFlagRsaSha512
+ default:
+ return nil, fmt.Errorf("agent: unsupported algorithm %q", algorithm)
+ }
+
return s.agent.SignWithFlags(s.pub, data, flags)
}
+var _ ssh.AlgorithmSigner = &agentKeyringSigner{}
+
// Calls an extension method. It is up to the agent implementation as to whether or not
// any particular extension is supported and may always return an error. Because the
// type of the response is up to the implementation, this returns the bytes of the
diff --git a/ssh/client_auth.go b/ssh/client_auth.go
index affc01d..a962a67 100644
--- a/ssh/client_auth.go
+++ b/ssh/client_auth.go
@@ -9,6 +9,7 @@
"errors"
"fmt"
"io"
+ "strings"
)
type authResult int
@@ -29,6 +30,33 @@
if err != nil {
return err
}
+ // The server may choose to send a SSH_MSG_EXT_INFO at this point (if we
+ // advertised willingness to receive one, which we always do) or not. See
+ // RFC 8308, Section 2.4.
+ extensions := make(map[string][]byte)
+ if len(packet) > 0 && packet[0] == msgExtInfo {
+ var extInfo extInfoMsg
+ if err := Unmarshal(packet, &extInfo); err != nil {
+ return err
+ }
+ payload := extInfo.Payload
+ for i := uint32(0); i < extInfo.NumExtensions; i++ {
+ name, rest, ok := parseString(payload)
+ if !ok {
+ return parseError(msgExtInfo)
+ }
+ value, rest, ok := parseString(rest)
+ if !ok {
+ return parseError(msgExtInfo)
+ }
+ extensions[string(name)] = value
+ payload = rest
+ }
+ packet, err = c.transport.readPacket()
+ if err != nil {
+ return err
+ }
+ }
var serviceAccept serviceAcceptMsg
if err := Unmarshal(packet, &serviceAccept); err != nil {
return err
@@ -41,7 +69,7 @@
sessionID := c.transport.getSessionID()
for auth := AuthMethod(new(noneAuth)); auth != nil; {
- ok, methods, err := auth.auth(sessionID, config.User, c.transport, config.Rand)
+ ok, methods, err := auth.auth(sessionID, config.User, c.transport, config.Rand, extensions)
if err != nil {
return err
}
@@ -93,7 +121,7 @@
// If authentication is not successful, a []string of alternative
// method names is returned. If the slice is nil, it will be ignored
// and the previous set of possible methods will be reused.
- auth(session []byte, user string, p packetConn, rand io.Reader) (authResult, []string, error)
+ auth(session []byte, user string, p packetConn, rand io.Reader, extensions map[string][]byte) (authResult, []string, error)
// method returns the RFC 4252 method name.
method() string
@@ -102,7 +130,7 @@
// "none" authentication, RFC 4252 section 5.2.
type noneAuth int
-func (n *noneAuth) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
+func (n *noneAuth) auth(session []byte, user string, c packetConn, rand io.Reader, _ map[string][]byte) (authResult, []string, error) {
if err := c.writePacket(Marshal(&userAuthRequestMsg{
User: user,
Service: serviceSSH,
@@ -122,7 +150,7 @@
// a function call, e.g. by prompting the user.
type passwordCallback func() (password string, err error)
-func (cb passwordCallback) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
+func (cb passwordCallback) auth(session []byte, user string, c packetConn, rand io.Reader, _ map[string][]byte) (authResult, []string, error) {
type passwordAuthMsg struct {
User string `sshtype:"50"`
Service string
@@ -189,7 +217,36 @@
return "publickey"
}
-func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
+func pickSignatureAlgorithm(signer Signer, extensions map[string][]byte) (as AlgorithmSigner, algo string) {
+ 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
+ }
+
+ 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
+ }
+
+ serverAlgos := strings.Split(string(extPayload), ",")
+ keyAlgos := algorithmsForKeyFormat(keyFormat)
+ 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
+ }
+ return as, algo
+}
+
+func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand io.Reader, extensions map[string][]byte) (authResult, []string, error) {
// Authentication is performed by sending an enquiry to test if a key is
// acceptable to the remote. If the key is acceptable, the client will
// attempt to authenticate with the valid key. If not the client will repeat
@@ -201,7 +258,10 @@
}
var methods []string
for _, signer := range signers {
- ok, err := validateKey(signer.PublicKey(), user, c)
+ pub := signer.PublicKey()
+ as, algo := pickSignatureAlgorithm(signer, extensions)
+
+ ok, err := validateKey(pub, algo, user, c)
if err != nil {
return authFailure, nil, err
}
@@ -209,13 +269,13 @@
continue
}
- pub := signer.PublicKey()
pubKey := pub.Marshal()
- sign, err := signer.Sign(rand, buildDataSignedForAuth(session, userAuthRequestMsg{
+ data := buildDataSignedForAuth(session, userAuthRequestMsg{
User: user,
Service: serviceSSH,
Method: cb.method(),
- }, []byte(pub.Type()), pubKey))
+ }, algo, pubKey)
+ sign, err := as.SignWithAlgorithm(rand, data, underlyingAlgo(algo))
if err != nil {
return authFailure, nil, err
}
@@ -229,7 +289,7 @@
Service: serviceSSH,
Method: cb.method(),
HasSig: true,
- Algoname: pub.Type(),
+ Algoname: algo,
PubKey: pubKey,
Sig: sig,
}
@@ -266,26 +326,25 @@
}
// validateKey validates the key provided is acceptable to the server.
-func validateKey(key PublicKey, user string, c packetConn) (bool, error) {
+func validateKey(key PublicKey, algo string, user string, c packetConn) (bool, error) {
pubKey := key.Marshal()
msg := publickeyAuthMsg{
User: user,
Service: serviceSSH,
Method: "publickey",
HasSig: false,
- Algoname: key.Type(),
+ Algoname: algo,
PubKey: pubKey,
}
if err := c.writePacket(Marshal(&msg)); err != nil {
return false, err
}
- return confirmKeyAck(key, c)
+ return confirmKeyAck(key, algo, c)
}
-func confirmKeyAck(key PublicKey, c packetConn) (bool, error) {
+func confirmKeyAck(key PublicKey, algo string, c packetConn) (bool, error) {
pubKey := key.Marshal()
- algoname := key.Type()
for {
packet, err := c.readPacket()
@@ -302,14 +361,14 @@
if err := Unmarshal(packet, &msg); err != nil {
return false, err
}
- if msg.Algo != algoname || !bytes.Equal(msg.PubKey, pubKey) {
+ if msg.Algo != algo || !bytes.Equal(msg.PubKey, pubKey) {
return false, nil
}
return true, nil
case msgUserAuthFailure:
return false, nil
default:
- return false, unexpectedMessageError(msgUserAuthSuccess, packet[0])
+ return false, unexpectedMessageError(msgUserAuthPubKeyOk, packet[0])
}
}
}
@@ -330,6 +389,7 @@
// along with a list of remaining authentication methods to try next and
// an error if an unexpected response was received.
func handleAuthResponse(c packetConn) (authResult, []string, error) {
+ gotMsgExtInfo := false
for {
packet, err := c.readPacket()
if err != nil {
@@ -341,6 +401,12 @@
if err := handleBannerResponse(c, packet); err != nil {
return authFailure, nil, err
}
+ case msgExtInfo:
+ // Ignore post-authentication RFC 8308 extensions, once.
+ if gotMsgExtInfo {
+ return authFailure, nil, unexpectedMessageError(msgUserAuthSuccess, packet[0])
+ }
+ gotMsgExtInfo = true
case msgUserAuthFailure:
var msg userAuthFailureMsg
if err := Unmarshal(packet, &msg); err != nil {
@@ -395,7 +461,7 @@
return "keyboard-interactive"
}
-func (cb KeyboardInteractiveChallenge) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
+func (cb KeyboardInteractiveChallenge) auth(session []byte, user string, c packetConn, rand io.Reader, _ map[string][]byte) (authResult, []string, error) {
type initiateMsg struct {
User string `sshtype:"50"`
Service string
@@ -412,6 +478,7 @@
return authFailure, nil, err
}
+ gotMsgExtInfo := false
for {
packet, err := c.readPacket()
if err != nil {
@@ -425,6 +492,13 @@
return authFailure, nil, err
}
continue
+ case msgExtInfo:
+ // Ignore post-authentication RFC 8308 extensions, once.
+ if gotMsgExtInfo {
+ return authFailure, nil, unexpectedMessageError(msgUserAuthInfoRequest, packet[0])
+ }
+ gotMsgExtInfo = true
+ continue
case msgUserAuthInfoRequest:
// OK
case msgUserAuthFailure:
@@ -497,9 +571,9 @@
maxTries int
}
-func (r *retryableAuthMethod) auth(session []byte, user string, c packetConn, rand io.Reader) (ok authResult, methods []string, err error) {
+func (r *retryableAuthMethod) auth(session []byte, user string, c packetConn, rand io.Reader, extensions map[string][]byte) (ok authResult, methods []string, err error) {
for i := 0; r.maxTries <= 0 || i < r.maxTries; i++ {
- ok, methods, err = r.authMethod.auth(session, user, c, rand)
+ ok, methods, err = r.authMethod.auth(session, user, c, rand, extensions)
if ok != authFailure || err != nil { // either success, partial success or error terminate
return ok, methods, err
}
@@ -542,7 +616,7 @@
target string
}
-func (g *gssAPIWithMICCallback) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
+func (g *gssAPIWithMICCallback) auth(session []byte, user string, c packetConn, rand io.Reader, _ map[string][]byte) (authResult, []string, error) {
m := &userAuthRequestMsg{
User: user,
Service: serviceSSH,
diff --git a/ssh/client_auth_test.go b/ssh/client_auth_test.go
index f73079b..a6bedbf 100644
--- a/ssh/client_auth_test.go
+++ b/ssh/client_auth_test.go
@@ -105,11 +105,63 @@
return err, serverAuthErrors
}
+type loggingAlgorithmSigner struct {
+ used []string
+ AlgorithmSigner
+}
+
+func (l *loggingAlgorithmSigner) Sign(rand io.Reader, data []byte) (*Signature, error) {
+ l.used = append(l.used, "[Sign]")
+ return l.AlgorithmSigner.Sign(rand, data)
+}
+
+func (l *loggingAlgorithmSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error) {
+ l.used = append(l.used, algorithm)
+ return l.AlgorithmSigner.SignWithAlgorithm(rand, data, algorithm)
+}
+
func TestClientAuthPublicKey(t *testing.T) {
+ signer := &loggingAlgorithmSigner{AlgorithmSigner: testSigners["rsa"].(AlgorithmSigner)}
config := &ClientConfig{
User: "testuser",
Auth: []AuthMethod{
- PublicKeys(testSigners["rsa"]),
+ PublicKeys(signer),
+ },
+ HostKeyCallback: InsecureIgnoreHostKey(),
+ }
+ if err := tryAuth(t, config); err != nil {
+ t.Fatalf("unable to dial remote side: %s", err)
+ }
+ // Once the server implements the server-sig-algs extension, this will turn
+ // into KeyAlgoRSASHA256.
+ if len(signer.used) != 1 || signer.used[0] != KeyAlgoRSA {
+ t.Errorf("unexpected Sign/SignWithAlgorithm calls: %q", signer.used)
+ }
+}
+
+// TestClientAuthNoSHA2 tests a ssh-rsa Signer that doesn't implement AlgorithmSigner.
+func TestClientAuthNoSHA2(t *testing.T) {
+ config := &ClientConfig{
+ User: "testuser",
+ Auth: []AuthMethod{
+ PublicKeys(&legacyRSASigner{testSigners["rsa"]}),
+ },
+ HostKeyCallback: InsecureIgnoreHostKey(),
+ }
+ if err := tryAuth(t, config); err != nil {
+ t.Fatalf("unable to dial remote side: %s", err)
+ }
+}
+
+// TestClientAuthThirdKey checks that the third configured can succeed. If we
+// were to do three attempts for each key (rsa-sha2-256, rsa-sha2-512, ssh-rsa),
+// we'd hit the six maximum attempts before reaching it.
+func TestClientAuthThirdKey(t *testing.T) {
+ config := &ClientConfig{
+ User: "testuser",
+ Auth: []AuthMethod{
+ PublicKeys(testSigners["rsa-openssh-format"],
+ testSigners["rsa-openssh-format"], testSigners["rsa"]),
},
HostKeyCallback: InsecureIgnoreHostKey(),
}
diff --git a/ssh/common.go b/ssh/common.go
index d6d9bf9..2a47a61 100644
--- a/ssh/common.go
+++ b/ssh/common.go
@@ -297,8 +297,9 @@
}
// buildDataSignedForAuth returns the data that is signed in order to prove
-// possession of a private key. See RFC 4252, section 7.
-func buildDataSignedForAuth(sessionID []byte, req userAuthRequestMsg, algo, pubKey []byte) []byte {
+// possession of a private key. See RFC 4252, section 7. algo is the advertised
+// algorithm, and may be a certificate type.
+func buildDataSignedForAuth(sessionID []byte, req userAuthRequestMsg, algo string, pubKey []byte) []byte {
data := struct {
Session []byte
Type byte
@@ -306,7 +307,7 @@
Service string
Method string
Sign bool
- Algo []byte
+ Algo string
PubKey []byte
}{
sessionID,
diff --git a/ssh/handshake.go b/ssh/handshake.go
index 4bceb33..f815cdb 100644
--- a/ssh/handshake.go
+++ b/ssh/handshake.go
@@ -476,6 +476,13 @@
}
} else {
msg.ServerHostKeyAlgos = t.hostKeyAlgorithms
+
+ // As a client we opt in to receiving SSH_MSG_EXT_INFO so we know what
+ // algorithms the server supports for public key authentication. See RFC
+ // 8303, Section 2.1.
+ msg.KexAlgos = make([]string, 0, len(t.config.KeyExchanges)+1)
+ msg.KexAlgos = append(msg.KexAlgos, t.config.KeyExchanges...)
+ msg.KexAlgos = append(msg.KexAlgos, "ext-info-c")
}
packet := Marshal(msg)
diff --git a/ssh/messages.go b/ssh/messages.go
index 62f9330..19bc67c 100644
--- a/ssh/messages.go
+++ b/ssh/messages.go
@@ -141,6 +141,14 @@
Service string `sshtype:"6"`
}
+// See RFC 8308, section 2.3
+const msgExtInfo = 7
+
+type extInfoMsg struct {
+ NumExtensions uint32 `sshtype:"7"`
+ Payload []byte `ssh:"rest"`
+}
+
// See RFC 4252, section 5.
const msgUserAuthRequest = 50
@@ -782,6 +790,8 @@
msg = new(serviceRequestMsg)
case msgServiceAccept:
msg = new(serviceAcceptMsg)
+ case msgExtInfo:
+ msg = new(extInfoMsg)
case msgKexInit:
msg = new(kexInitMsg)
case msgKexDHInit:
@@ -843,6 +853,7 @@
msgDisconnect: "disconnectMsg",
msgServiceRequest: "serviceRequestMsg",
msgServiceAccept: "serviceAcceptMsg",
+ msgExtInfo: "extInfoMsg",
msgKexInit: "kexInitMsg",
msgKexDHInit: "kexDHInitMsg",
msgKexDHReply: "kexDHReplyMsg",
diff --git a/ssh/server.go b/ssh/server.go
index 9bb714c..70045bd 100644
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -554,6 +554,7 @@
if !ok || len(payload) > 0 {
return nil, parseError(msgUserAuthRequest)
}
+
// Ensure the public key algo and signature algo
// are supported. Compare the private key
// algorithm name that corresponds to algo with
@@ -563,7 +564,12 @@
authErr = fmt.Errorf("ssh: algorithm %q not accepted", sig.Format)
break
}
- signedData := buildDataSignedForAuth(sessionID, userAuthReq, algoBytes, pubKeyData)
+ if underlyingAlgo(algo) != sig.Format {
+ authErr = fmt.Errorf("ssh: signature %q not compatible with selected algorithm %q", sig.Format, algo)
+ break
+ }
+
+ signedData := buildDataSignedForAuth(sessionID, userAuthReq, algo, pubKeyData)
if err := pubKey.Verify(signedData, sig); err != nil {
return nil, err
To view, visit change 392394. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages