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

Implement rsa-sha1/rsa-sha256/ecdsa-sha256 algorithms #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ejholmes
Copy link

@ejholmes ejholmes commented Apr 26, 2018

This supersedes my original PR that only implemented the rsa-* algorithms, and adds support for ecdsa-sha256 as well.

Notes for reviewer

  • I think the ECDSA signature generation is correct, but the HTTP signatures RFC is light on details, and just points to the format that JOSE uses.
  • Probably extra attention needs to be paid to the ECDSA implementation to make sure it adheres to the spec. Unfortunately, the RFC doesn't provide any test fixtures to use for signature generation/verification.

AlgorithmHmacSha1 = &Algorithm{"hmac-sha1", hmacSign(crypto.SHA1), hmacVerify(crypto.SHA1)}
AlgorithmRsaSha256 = &Algorithm{"rsa-sha256", rsaSign(crypto.SHA256), rsaVerify(crypto.SHA256)}
AlgorithmRsaSha1 = &Algorithm{"rsa-sha1", rsaSign(crypto.SHA1), rsaVerify(crypto.SHA1)}
AlgorithmEcdsaSha256 = &Algorithm{"ecdsa-sha256", ecdsaSign(crypto.SHA256), ecdsaVerify(crypto.SHA256)}
Copy link
Author

@ejholmes ejholmes Apr 26, 2018

Choose a reason for hiding this comment

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

This should probably validate that the key is using a P256 curve. If someone generates a P224 curve, the hash would be truncated when siging:

Sign signs a hash (which should be the result of hashing a larger message) using the private key, priv. If the hash is longer than the bit-length of the private key's curve order, the hash will be truncated to that length. It returns the signature as a pair of integers. The security of the private key depends on the entropy of rand.

https://golang.org/pkg/crypto/ecdsa/#PrivateKey.Sign

Choose a reason for hiding this comment

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

Technically "ecdsa-sha256" only prescribes that the digest function uses SHA-256. The EC key can use any curve or number of bits. That's something the consumer must be able to infer from the key id.

@pda
Copy link

pda commented Apr 27, 2018

Thanks for submitting this. I think it's really important. It also needs some careful review which I can't do right now. I hope somebody else can, otherwise I'll get to it eventually.

@ejholmes
Copy link
Author

ejholmes commented May 1, 2018

Thanks for submitting this. I think it's really important. It also needs some careful review which I can't do right now. I hope somebody else can, otherwise I'll get to it eventually.

No worries :). Definitely agree that this should be reviewed carefully, especially the ECDSA implementation, since the RFC is pretty light on details for it.

@liamdennehy
Copy link

Hi @ejholmes, I'm looking at adding EC support to 99designs/http-signatures-php , and I'm also working on improving the RFC itself. Agreed it's not up to scratch, what specifically are you looking to see?

From my end, I don't like that we're pointing to the JWS spec, we should rather aim at an EC-specific spec but I'm not familiar with the literature (or much about EC in general), so some research needed.

@ejholmes
Copy link
Author

ejholmes commented Apr 9, 2020

@liamdennehy I think most of my issues were addressed with the addition of a https://github.com/w3c-ccg/http-signatures-test-suite, but I haven't had a chance to test this PR against it.

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

Successfully merging this pull request may close these issues.

4 participants