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

Use appropriate digest algorithm during signature creation #97

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oncilla
Copy link

@oncilla oncilla commented Oct 30, 2021

Pass the public key instead of the marshalled public key to
digestAlgorithmForPublicKey in SignedData.AddSignerInfo.

Previously, the marshalled public key was passed instead of the actual
public key. The result is that always SHA256 was being selected, even
for ECDSA where the hash algorithm should be selected based on the curve.

Pass the public key instead of the marshalled public key to
`digestAlgorithmForPublicKey` in `SignedData.AddSignerInfo`.

Previously, the marshalled public key was passed instead of the actual
public key. The result is that always SHA256 was being selected, even
for ECDSA where the hash algorithm should be selected based on the curve.
@oncilla
Copy link
Author

oncilla commented Nov 13, 2021

Hi @lgarron

What can I do to move this forward?

@lgarron
Copy link
Contributor

lgarron commented Nov 14, 2021

I'm not qualified to review this, but @vcsjones may be!

@vcsjones vcsjones self-requested a review November 16, 2021 18:53
@vcsjones
Copy link
Member

I see:

func digestAlgorithmForPublicKey(pub crypto.PublicKey) pkix.AlgorithmIdentifier {
if ecPub, ok := pub.(*ecdsa.PublicKey); ok {
switch ecPub.Curve {
case elliptic.P384():
return pkix.AlgorithmIdentifier{Algorithm: oid.DigestAlgorithmSHA384}
case elliptic.P521():
return pkix.AlgorithmIdentifier{Algorithm: oid.DigestAlgorithmSHA512}
}
}
return pkix.AlgorithmIdentifier{Algorithm: oid.DigestAlgorithmSHA256}
}

Since we're passing in something that is not an ecdsa.PublicKey, none of the if checks match and it defaults to SHA256.

I think this change looks good, but it would be great to get some test coverage that indeed the right digest algorithm is used for the curve. Would you be able to add some test coverage for this, @oncilla?

Copy link
Member

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

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

Changes look reasonable as I previously mentioned, but hoping we can get some test coverage for this.

@oncilla
Copy link
Author

oncilla commented Nov 16, 2021

@vcsjones sure. I will have look tomorrow

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