Accept crypto.Signer that contains a ed25519.PublicKey in ed25519

[MagicalTux]

in order to allow usage of other ed25519 providers than crypto/ed25519

For example in my case I use a HSM (YubiHSM2), which means the private key is not visible to the go code (can't check its len, etc), however it is indeed a ed25519 key and the object provides the Sign() method

The HSM lib: https://github.com/KarpelesLab/hsm

[oxisto]

Thank you for you contribution! I am a bit sceptical about this though. I understand your specific use case, however accepting a generic crypto.Signer could lead to situations where people insert the wrong kind of key into this function and we will never be able to catch it.

I fear that the "safer" way for you would probably be to register a custom signing method that does this specifically for the key struct/interface that you have or we could think about having a "generic" signing method that does accept any crypto.Signer interface.

[MagicalTux]

Golang does not provide a "good way" to check if a given signer is a RSA, ECDSA or Ed25519 key.

One option would be to use ed25519Key.Public() to get the public key and check its type and optionally size (returning the right type of public key like ed25519.PublicKey is easy). The only thing is that adds some overhead compared to a normal type check.

Another option would be to add a "getTypeByObject" function that checks if the key is one of known types, and only accept either ed25519 or unknown keys (for advanced users), but reject common errors of for example someone putting a RSA key into a ed25519 signature.

[lggomez]

I think the issue, to start, is that the method name was not matching its exact semantics, because in fact it should be named SignEd25519. This change a retrocompatible altough as @oxisto says in its current form removes safety from any consumer's errors. Maybe we can extend the API with a Sign_[signer]_ family of methods to provide a type-safe way of working with the stdlib signers and leave this method for working with custom use cases like the HSM signer

[MagicalTux]

I've looked at Go's own libraries since these work just fine and it turns out typically Go will detect the algorithm based on the public key, so that's probably an acceptable way of handling things.

Ideally the key type shouldn't be passed and indeed be automatically detected based on the key itself as other go apis do, but in the meantime this is an update that makes jwt more generic without modifying existing APIs.

• Now checks if public key is indeed of type ed25519.PublicKey
• Will not panic if the key length is long because we use the .Sign() Signer object method instead of ed25519.Sign() which has no way of returning errors

The only difference is that passing an invalid ed25519 key will now return a different kind of error, but that is probably a good thing as it helps differentiate between invalid ed25519 key and wrong key algorithm.

I do think this should be modified in the future to remove the need of specifying the key algorithm.

[oxisto]

I've looked at Go's own libraries since these work just fine and it turns out typically Go will detect the algorithm based on the public key, so that's probably an acceptable way of handling things.

Ideally the key type shouldn't be passed and indeed be automatically detected based on the key itself as other go apis do, but in the meantime this is an update that makes jwt more generic without modifying existing APIs.

• Now checks if public key is indeed of type ed25519.PublicKey
• Will not panic if the key length is long because we use the .Sign() Signer object method instead of ed25519.Sign() which has no way of returning errors

The only difference is that passing an invalid ed25519 key will now return a different kind of error, but that is probably a good thing as it helps differentiate between invalid ed25519 key and wrong key algorithm.

I do think this should be modified in the future to remove the need of specifying the key algorithm.

I think I could live with that solution.

Could you maybe add a comment to the crypto.Hash(0) part, why it is necessary or what it does (or basically not does). It looks weird at first glance, but looking at the documentation of Signer.Sign, It looks to be ok.

[MagicalTux]

Added a comment about crypto.Hash(0). This actually comes from crypto/ed25519, see https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/crypto/ed25519/ed25519.go;l=80

[oxisto]

LGTM from my side.