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

Populates the algorithm Signature Parameter based on which Signer is used. #16

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

Conversation

JackSullivan
Copy link

Currently the code uses a hardcoded value for the default signer, which means that attempting to use RsaSha256Sign as a signer produces an invalid authorization header. This PR attempts to fix that by adding a name method to HttpSignatureSign and implementing it with the name literal string passed to the macros. This is the correct value for rsa-sha signing, but may not be correct for hmac. I'm happy to make changes as necessary.

taken from the appropriate signer rather than having it hardcoded as
`hs2019`.
@bradfier bradfier requested a review from jac32 January 18, 2022 12:53
@paulyoung
Copy link

I thought the motivation for introducing hs2019 was to hide the algorithm from the signature.

@thomassa
Copy link
Collaborator

@paulyoung is right: that's what it says in https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-12

That draft has been replaced by a series of others. The latest is from early May 2023 and it no longer mentions algorithm hs2019, but instead uses meaningful algorithm-names, thus no longer trying to conceal the choice of algorithm.

Therefore this pullreq might be useful if/when this repository is updated to implement the latest draft of the standard (or ideally the final version, once the standard is no longer a draft).

@paulyoung
Copy link

@thomassa, in case you’re interested –

I was using @dskyberg's fork and then made some further additions.

@paulyoung
Copy link

@thomassa, in case you’re interested –

I was using @dskyberg's fork and then made some further additions.

I think the most interesting commits are:

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.

3 participants