proposal: x/crypto: support OpenSSH variant of ChaCha20Poly1305

johnnybubonic commented 1 year ago

What version of Go are you using (`go version`)?

$ go version
go version go1.19.4 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/[REDACTED]/.cache/go-build"
GOENV="/home/[REDACTED]/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/opt/dev/go/pkg/mod"
GONOPROXY="[REDACTED]"
GONOSUMDB="[REDACTED]"
GOOS="linux"
GOPATH="/opt/dev/go"
GOPRIVATE="[REDACTED]"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1148312864=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This isn't so much what I did, but rather something the x/crypto team did. Understandably so, but nonetheless limiting.

With the implementation of #36646, it is now no longer possible to generate and modify chacha20poly1305@openssh.com-encrypted OpenSSH keypairs in a low/direct manner. (AES-encrypted keys are, of course, quite easy to modify and generate as long as the new key structure is understood.)

That is, one cannot create chacha20poly1305-encrypted OpenSSH private keys without vendoring/modifying ChaCha20 and Poly1305. Which then means they are highly susceptible to be not kept in-line with upstream (x/crypto)'s implementation, thus defeating a part of the intention of removing the public components in the first place as outlined in the original discussion.

What did you expect to see?

I expect to see an ability to use a cipher.AEAD with chacha20poly1305@openssh.com (which uses a 16-byte nonce for ChaCha20 instead of a 12 or 24-byte nonce).

What did you see instead?

This functionality is not possible without vendoring or maintaining a separate fork of, at the least:

x/crypto/chacha20
x/crypto/poly1305
x/crypto/internal/poly1305 (unless implemented in fork of the above directly)

Add'l Notes (EDIT)

I received an email notification that someone commented inquiring if (golang.org/x/crypto/chacha20poly1305).New can be used but it appears it has been removed as it seems they understood the issue after commenting. :)

To avoid others making the same mistake, no, chacha20poly1305.New cannot be used because it uses a 12-byte nonce (and uses the RFC-specified padding), both of which are incompatible with the OpenSSH variant. Likewise, chacha20poly1305.NewX cannot be used because it uses a 24-byte nonce (and the RFC-specified padding), which also is incompatible with the OpenSSH variant. OpenSSH implements a 16-byte nonce and a "proprietary" padding mechanism using sequential uint8 integers. This is why their cipher implementation is named chacha20poly1305@openssh.com instead of chacha20poly1305.

For more information/technical details:

cagedmantis commented 1 year ago

/cc @FiloSottile @rolandshoemaker @golang/security

rolandshoemaker commented 1 year ago

It sounds like this is essentially a proposal to add a new API, say NewOpenSSH, which returns a cipher.AEAD that uses a 16 byte nonce.

Given it's no worse than the standard (smaller) nonce size, and it provides support for a deployed construction (OpenSSH choosing the middle of the road size between standard and X was a fun choice), I have no real opposition.

ianlancetaylor commented 1 year ago

Can someone write down the proposed new API with its doc comment? Thanks.

rolandshoemaker commented 1 year ago

Probably this:

// NonceSizeOpenSSH is the size of the nonce used with the chacha20poly1305@openssh.com variant of this
// AEAD, in bytes
NonceSizeOpenSSH = 16

// NewOpenSSH returns a chacha20poly1305@openssh.com AEAD that uses the given 256-bit key.
//
// chacha20poly1305@openssh.com is a ChaCha20-Poly1305 variant that uses a non-standard nonce size,
// suitable for usage with OpenSSH.
func NewOpenSSH(key []byte) (cipher.AEAD, error)

n2vi commented 2 weeks ago

I also need this. My workaround is to copy the source into my personal ssh module, which is a sub-optimal outcome. Indeed poly1305 is a footgun, but as used by ssh it is ok. For the crypto library to hide it entirely, preventing independent implementation of ssh, seems peculiar.

Are we talking about the same nonce? I allocate a 12-byte nonce and for OpenSSH compatibility use a 4-byte packet counter and zero-fill.

rsc commented 1 week ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

n2vi commented 1 week ago

Perhaps we can just move poly1305 to crypto/subtle instead of crypto/internal?

On Wed, Sep 4, 2024, 11:45 Russ Cox @.***> wrote:

This proposal has been added to the active column https://go.dev/s/proposal-status#active of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

— Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/57699#issuecomment-2329746453, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACADPOSOXPD5ODLGZK4TUKDZU5IKZAVCNFSM6AAAAABNGNG762VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRZG42DMNBVGM . You are receiving this because you commented.Message ID: @.***>

rsc commented 5 days ago

It sounds like there are few enough of these variants that we should add them all explicitly instead of exposing primitives like poly1305 and leaving people to connect them correctly. https://github.com/golang/go/issues/57699#issuecomment-1379343978 seems fine except that we need to resolve whether we're talking about two different variants, or maybe three (nonce lengths 12, 16, 24).

rsc commented 5 days ago

/cc @FiloSottile

FiloSottile commented 4 days ago

The existence of an "original" variant of ChaCha20Poly1305 and an IETF variant is unfortunate, but nearly everything and everyone converged on the IETF variant, and I wouldn't want to contribute to rekindling that mess.

Besides, my understanding is that the "original" variant has a 8 bytes (64 bit) nonce, not a 16 bytes nonce. https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.chacha20poly1305 and https://github.com/golang/crypto/blob/9e92970a1eb41e446822e037016aa89d24c0ce7a/ssh/cipher.go#L669 seems to agree with that. Where is the 16 bytes nonce size coming from?

Taking a step back, why do you need to use this variant of the AEAD directly? You mention generating encrypted OpenSSH keypairs, maybe we should just extend the ssh.MarshalPrivateKeyWithPassphrase API, maybe as part of #68723 which is already adding a struct for marshal parameters?

Finally, x/crypto/poly1305 is Deprecated, not removed, so it's not the case that it is no longer possible to use it. If you know what you are doing enough to want to use a non-standard AEAD to do low-level operations on an encrypted key, using a deprecated package doesn't sound wrong.

n2vi commented 4 days ago

Yes, I was also puzzled about the OP comment about 16 byte nonce. Perhaps he can clarify.

I may have misunderstood your proposal with respect to marking deprecated versus moving to internal. As long as crypto/ssh/cipher.go interoperates with OpenSSH and does not import crypto/internal then fine.

FiloSottile commented 4 days ago

As long as crypto/ssh/cipher.go interoperates with OpenSSH and does not import crypto/internal then fine.

x/crypto/ssh may move to the standard library, at which point who knows what it will import. My point is that your ssh module can keep importing x/crypto/poly1305 even if it is deprecated.

golang / go