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

Fix ed25519 generated SSH key marshalling #14101

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

cipherboy
Copy link
Contributor

This updates the generation test to ensure that, for each generated CA key type, we can sign certificates with this key pair. This ensures that they're stored in a format understood by golang.org/x/crypto/ssh.

Notably, this caught a bug in the Ed25519 key handling: OpenSSH (and golang.org/x/crypto/ssh) do not understand the PKCS8 PrivateKey structure we were using. As mentioned in an internal RFC (TF 461), and with investigation from a terraform-provider-tls PR author, it appears that using edkey is the best approach.

As this feature hasn't been released, we shouldn't need a changelog entry for it.

This adds a test to ensure that we can issue leaf SSH certificates using
the newly generated SSH CA keys. Presently this fails because the
ed25519 key private is stored using PKIX's PKCS8 PrivateKey object
format rather than using OpenSSH's desired private key format:

> path_config_ca_test.go:211: bad case 12: err: failed to parse stored CA private key: ssh: invalid openssh private key format, resp: <nil>

Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy added bug Used to indicate a potential bug pr/no-changelog labels Feb 16, 2022
@cipherboy cipherboy requested a review from a team February 16, 2022 16:33
As mentioned in various terraform-provider-tls discussions, OpenSSH
doesn't understand the standard OpenSSL/PKIX ed25519 key structure (as
generated by PKCS8 marshalling). Instead, we need to place it into the
OpenSSH RFC 8709 format. As mentioned in this dependency's README,
support in golang.org/x/crypto/ssh is presently lacking for this.
When the associated CL is merged, we should be able to remove this dep
and rely on the (extended) standard library, however, no review progress
appears to have been made since the CL was opened by the author.

See also: https://go-review.googlesource.com/c/crypto/+/218620/

Signed-off-by: Alexander Scheel <[email protected]>
Copy link
Contributor

@schultz-is schultz-is left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cipherboy
Copy link
Contributor Author

Thanks @schultz-is!

@cipherboy cipherboy merged commit 42b90ff into main Feb 16, 2022
@@ -306,6 +306,7 @@ require (
github.com/mattn/go-isatty v0.0.14 // indirect
github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
github.com/miekg/dns v1.1.41 // indirect
github.com/mikesmitty/edkey v0.0.0-20170222072505-3356ea4e686a // indirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is a direct dependency so running go mod tidy will force a change in the future?

@cipherboy cipherboy deleted the cipherboy-issue-from-generated branch February 16, 2022 21:05
@cipherboy cipherboy added this to the 1.10 milestone Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug cryptosec pr/no-changelog secret/ssh
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants