Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/ssh: support for server-sig-algs extension (RFC8308) #49269

Open
Tracked in #49952
aphistic opened this issue Nov 1, 2021 · 15 comments · May be fixed by golang/crypto#197
Open
Tracked in #49952

x/crypto/ssh: support for server-sig-algs extension (RFC8308) #49269

aphistic opened this issue Nov 1, 2021 · 15 comments · May be fixed by golang/crypto#197
Labels
NeedsInvestigation
Milestone

Comments

@aphistic
Copy link

@aphistic aphistic commented Nov 1, 2021

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.

@gopherbot gopherbot added this to the Unreleased milestone Nov 1, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 1, 2021

Change https://golang.org/cl/360195 mentions this issue: ssh: add support for extension negotiation (rfc 8308)

@seankhliao seankhliao added the NeedsInvestigation label Nov 2, 2021
@seankhliao
Copy link
Member

@seankhliao seankhliao commented Nov 2, 2021

cc @FiloSottile

@aphistic
Copy link
Author

@aphistic aphistic commented Dec 3, 2021

@rmohr

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.

@aphistic
Copy link
Author

@aphistic aphistic commented Jan 11, 2022

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.

DominicLavery pushed a commit to CircleCI-Public/golang-crypto that referenced this issue Jan 18, 2022
This adds support for SSH extension negotiation via SSH_MSG_EXT_INFO as defined in RFC 8308 [1]. It also adds support for the `server-sig-algs` extension on both the client and server sides.

[1] https://datatracker.ietf.org/doc/html/rfc8308

Fixes golang/go#49269

Change-Id: Iab374d1254ec83eabdb5f433b95ff39a1a540cc3
GitHub-Last-Rev: 29bf9ec
GitHub-Pull-Request: golang#197
@pete-woods
Copy link

@pete-woods pete-woods commented Feb 11, 2022

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?

@aphistic
Copy link
Author

@aphistic aphistic commented Feb 11, 2022

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.

pete-woods pushed a commit to CircleCI-Public/golang-crypto that referenced this issue Mar 14, 2022
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
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 14, 2022

Change https://go.dev/cl/392394 mentions this issue: ssh: support rsa-sha2-256/512 for client authentication

gopherbot pushed a commit to golang/crypto that referenced this issue Mar 14, 2022
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 <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@pete-woods
Copy link

@pete-woods pete-woods commented Mar 15, 2022

I think this is resolved now

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Mar 15, 2022

Not yet, we accept server-sig-algs on the client but we don't send it on the server yet.

@aphistic
Copy link
Author

@aphistic aphistic commented Mar 15, 2022

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.

@quackduck
Copy link

@quackduck quackduck commented Apr 21, 2022

👋 This has been stuck for a while.

@FiloSottile do you know how long it could take to review those public API changes?

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented May 6, 2022

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 {

@aphistic
Copy link
Author

@aphistic aphistic commented May 6, 2022

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.

@owenthereal
Copy link

@owenthereal owenthereal commented Jun 3, 2022

👋 @aphistic @FiloSottile:

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!

@drakkan
Copy link

@drakkan drakkan commented Jun 6, 2022

Hi,

take a look here, there are no api changes in this patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants