x/crypto/ssh: support for server-sig-algs extension (RFC8308)
aphistic opened this issue · 17 comments
This relates to:
x/crypto/ssh: publicKeyCallback cannot handshake using ssh-rsa keys signed using the ssh-rsa-sha2-256 algorithm #39885
I decided to create a new issue even though this is mentioned in the comments for that issue because this is more specifically for server-sig-algs and RFC 8308 support.
What version of Go are you using (go version)?
$ go version go version go1.16.3 linux/amd64
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (go env)?
go env Output
$ go env GO111MODULE="" GOARCH="amd64" GOBIN="" GOCACHE="/home/kdavidson/.cache/go-build" GOENV="/home/kdavidson/.config/go/env" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOINSECURE="" GOMODCACHE="/home/kdavidson/go/pkg/mod" GONOPROXY="" GONOSUMDB="" GOOS="linux" GOPATH="/home/kdavidson/go" GOPRIVATE="" GOPROXY="https://proxy.golang.org,direct" GOROOT="/usr/local/go" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64" GOVCS="" GOVERSION="go1.16.3" GCCGO="gccgo" AR="ar" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="/dev/null" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4051208766=/tmp/go-build -gno-record-gcc-switches"
What did you do?
Recently, OpenSSH 8.8 deprecated support for ssh-rsa host keys and ssh-rsa pubkey auth as a default value. If users try to connect to a Go x/crypto/ssh server using OpenSSH 8.8 with ssh-rsa for pubkey auth, the client will fail to find a mutual algorithm and not even attempt to send a client's ssh-rsa pubkey auth, likely to avoid any auth penalties.
What did you expect to see?
I would expect the SSH server to send an SSH_MSG_EXT_INFO containing valid pubkey auth algorithms using the server-sig-algs extension, as defined in RFC 8308.
What did you see instead?
The x/crypto SSH server does not send any pubkey auth algorithms, so a client may end up not sending a potentially valid pubkey auth to avoid penalties.
Change https://golang.org/cl/360195 mentions this issue: ssh: add support for extension negotiation (rfc 8308)
cc @FiloSottile
I'm moving the response to your question over here so it can be tracked in the Gerrit CL for this issue.
Do you also plan to tackle automatically setting the right algorithm for the rsa signers (which are right now hardcoded to ssh-rsa?
I'm not quite sure what you mean here. I do plan to add support for server-sig-algs on the client side to allow that to influence the pubkey auth that's sent to the server, if that's what you mean? That's not in the PR as it exists now because I wanted to solidify the plans for upstreaming the code to make sure it fit.
Sorry that it's taken me so long to get this out here! Holidays and other projects and stuff...
For the public API, I don't really see many changes initially because most of this is implementation and not user-interactive.
Right now the only public API change I'm proposing is to add an additional field to the ServerConfig struct to allow users to tweak the PubKey Auth Algos and remove any they don't need:
type ServerConfig struct {
...
// The allowed public key auth algorithms. If unspecified then
// a default set of algorithms is used.
PublicKeyAuthAlgorithms []string
}Usage would look like:
cfg := &ServerConfig {
PublicKeyAuthAlgorithms: []string{
KeyAlgoRSA,
KeyAlgoDSA,
},
}This would allow users to enable or disable algorithms, or potentially disable this extension completely.
If PublicKeyAuthAlgorithms is nil, a default list of preferredPubKeyAuthAlgos would be used. If it's an empty slice, the server-sig-algs extension would not be sent by the server. For any other list of values, they would be compared against supportedPubKeyAuthAlgos to validate the selected algorithms against the supported ones.
I talked with my team about the potential to allow users to disable the extension negotiation completely, but we ultimately decided that since it's backward compatible it didn't make sense to include it.
The client side wouldn't have any public API changes and would either select from the server-sig-algs or, if those aren't sent, the current pubkey auth flow.
Since this is the first extension being implemented I needed to consider whether something like a map of extensions to settings in the configs would make sense, or whether using more explicit fields and concrete structs would be a better way to configure extensions. I ultimately decided it would make more sense to make them explicit fields and concrete structs to allow more flexibility (and I think better readability for users) for implementing future extensions that aren't already defined and may not fit the current set of potential values.
cc @FiloSottile since we were talking about this in Slack at one point.
I hate to nag on a public bug tracker, but I think a bunch of folks are hoping for this to be done before GitHub's SHA1 cutoff occurs on March 15th (https://github.blog/2021-09-01-improving-git-protocol-security-github/).
We at CircleCI are carrying your patch plus another one we yanked from somewhere else to wire in client support, as a pre-emptive measure.
Is there anything we can do to help move this towards completion?
Hey @pete-woods!
Right now I'm just waiting to hear back on the proposed public API before I rework the PR. There's still more work I need to do for a full client/server implementation of server-sig-algs, but since this my first PR for the package and given the sensitivity of a crypto-related package I want to be sure I'm following a good pattern before I go too far.
Change https://go.dev/cl/392394 mentions this issue: ssh: support rsa-sha2-256/512 for client authentication
I think this is resolved now
Not yet, we accept server-sig-algs on the client but we don't send it on the server yet.
Not yet, we accept server-sig-algs on the client but we don't send it on the server yet.
Do you have any thoughts on the public API changes I proposed? If they sound good to you I could start getting my PR updated to implement them.
@FiloSottile do you know how long it could take to review those public API changes?
The API changes need this issue to be converted to a proposal. I am generally ok with them, except I wouldn't make an empty slice a way to disable the extension. Let's keep things simple and always send it.
In the meantime, I would suggest implementing the extension without an API change, with the default list. It can be as simple as this:
diff --git a/ssh/handshake.go b/ssh/handshake.go
index 653dc4d..b578cfe 100644
--- a/ssh/handshake.go
+++ b/ssh/handshake.go
@@ -615,6 +615,7 @@ func (t *handshakeTransport) enterKeyExchange(otherInitPacket []byte) error {
return err
}
+ firstKeyExchange := t.sessionID == nil
if t.sessionID == nil {
t.sessionID = result.H
}
@@ -626,6 +627,13 @@ func (t *handshakeTransport) enterKeyExchange(otherInitPacket []byte) error {
if err = t.conn.writePacket([]byte{msgNewKeys}); err != nil {
return err
}
+
+ // If the client advertised support for RFC 8032 extensions, send a
+ // server-sig-algs extension.
+ if !isClient && firstKeyExchange && contains(serverInit.KexAlgos, "ext-info-c") {
+ // TODO
+ }
+
if packet, err := t.conn.readPacket(); err != nil {
return err
} else if packet[0] != msgNewKeys {
Sounds good! I'll probably close the initial PR I opened and start a new one based on the latest code since so much has changed and the client-side is implemented now.
I'm following up on the progress of #49952 because it has an impact on one of my projects: #49952 (comment). This issue is the last one to tackle. I'm wondering whether there is anything that I could help to move this forward. I'm happy to propose a PR based on what is discussed here. But I want to check with you first. Cheers!
Change https://go.dev/cl/447757 mentions this issue: ssh: support rsa-sha2-256/512 on the server side
https://go.dev/cl/447757 implements server-side sending of the extension, so that SHA-2-only clients will work. #56561 tracks adding the exposed APIs to configure the supported algorithms, so that SHA-1 can be turned off.