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

proposal: x/crypto/ssh: add SSHSIG support #68197

Open
caarlos0 opened this issue Jun 26, 2024 · 5 comments · May be fixed by golang/crypto#316
Open

proposal: x/crypto/ssh: add SSHSIG support #68197

caarlos0 opened this issue Jun 26, 2024 · 5 comments · May be fixed by golang/crypto#316
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@caarlos0
Copy link
Contributor

caarlos0 commented Jun 26, 2024

Proposal Details

I'd like to propose we support encoding and decoding SSHSIG signature format.

I already have a working implementation (armoring a *ssh.Signature and then parsing it back into the signed data), but I'm not sure what the api should look like.

We have a couple of steps to create a signature:

  • create a blob
  • sign the blob (this signing step is already implemented here)
  • create the signed data
  • encode it into a PEM format

To verify a signature, we need to:

  • create a blob
  • decode the previously created PEM formatted signature
  • call publickey.Verify(blob, decodedBlod)

Given all this, I'd suggest the following functions:

func CreateBlob(r io.Reader) ([]byte, error) // or (io.Reader, error)
func Encode(pk ssh.PublicKey, sig *ssh.Signature) ([]byte, error) // or (io.Reader, error)
func Decode(r io.Reader) (*ssh.Signature, ssh.PublicKey, error)

We would also need these two structs:

// Blob according to the SSHSIG protocol.
type Blob struct {
	Namespace     string
	Reserved      string
	HashAlgorithm string
	Hash          string
}

// SignedData according to the SSHSIG protocol.
type SignedData struct {
	MagicPreamble [6]byte
	Version       uint32
	PublicKey     string
	Namespace     string
	Reserved      string
	HashAlgorithm string
	Signature     string
}

and some constants:

const (
	magicPreamble = "SSHSIG"
	version       = 1
	namespace     = "file"
	hashAlgorithm = "sha512"
	armorType     = "SSH SIGNATURE"
)

There's also the discussion of which hash algorithms to support... only rsa-sha2-512 or rsa-sha2-256, which I think it's easy enough to support both.

Finally, the namespace, not sure if we allow to customize that or not.


Anyway, I would love to work on this, just need some direction on how the API should look like.

@gopherbot gopherbot added this to the Proposal milestone Jun 26, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jun 26, 2024
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jun 26, 2024
@ianlancetaylor
Copy link
Member

CC @golang/security @drakkan

@FiloSottile
Copy link
Contributor

I suggest we implement a higher-level API, without exposing the formatting of the signed blob to the application.

// Sign returns a detached SSH Signature for the provided message.
//
// The namespace is a domain-specific identifier for the context in which the
// signature will be used. It must match between the Sign and [Verify] calls. A
// fully-qualified suffix is recommended, e.g. "[email protected]".
//
// These signatures are compatible with those generated by "ssh-keygen -Y sign",
// and can be verified with [Verify] or "ssh-keygen -Y verify". The returned
// bytes are usually PEM encoded with [encoding/pem] and type "SSH SIGNATURE".
//
// If the Signer has an RSA PublicKey, it must also implement [AlgorithmSigner].
// If it also implements [MultiAlgorithmSigner], the first algorithm returned by
// Algorithms will be used, otherwise "rsa-sha2-512" is used.
func Sign(s Signer, rand io.Reader, message []byte, namespace string) ([]byte, error)

// Verify verifies a detached SSH Signature for the provided message.
//
// The namespace is a domain-specific identifier for the context in which the
// signature will be used. It must match between the [Sign] and Verify calls. A
// fully-qualified suffix is recommended, e.g. "[email protected]".
//
// The provided signature is usually decoded from a PEM block of type "SSH
// SIGNATURE" using [encoding/pem].
func Verify(pub PublicKey, message, signature []byte, namespace string) error

We can accept both sha256 and sha512 in Verify, and always use sha512 in Sign, like ssh-keygen.

As required by the specification, we will reject ssh-rsa signatures in Verify.

caarlos0 added a commit to caarlos0/crypto that referenced this issue Mar 20, 2025
Initial implementation of proposal
golang/go#68197.

Want to make sure the API is all right before adding more tests.
Also seeking feedback on how to best test this - is it OK to sign and
verify in the same test, or do you have other ideas? (maybe a fixed rand
reader?).
@caarlos0 caarlos0 linked a pull request Mar 20, 2025 that will close this issue
@caarlos0
Copy link
Contributor Author

Thanks @FiloSottile!

Implemented it here: golang/crypto#316

Still unsure about how to best unit test this, but otherwise the API feels good I think.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/659715 mentions this issue: ssh: sign and verify

@FiloSottile
Copy link
Contributor

Thank you for the PR! I see that you implemented it to take/return PEM. In the docs I meant "is usually decoded / encoded" as a suggestion for the user to do it themselves, if they need it armored. We should make the docs clearer if we return raw bytes, or maybe we should return just PEM?

As for unit testing, round-trip tests and a test against a ssh-keygen produced signature (for at least each signature algorithm) would be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

Successfully merging a pull request may close this issue.

4 participants