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

signature-params component of signature-base is not serialized #230

Open
da-tychinin opened this issue Mar 7, 2024 · 12 comments
Open

signature-params component of signature-base is not serialized #230

da-tychinin opened this issue Mar 7, 2024 · 12 comments

Comments

@da-tychinin
Copy link

As far as I understand the http messages signature process follows the HTTP Messages Signatures RFC

The algorithm to build a signature-base for signing/verification contains a discrepancy with the HTTP messages signatures RFC. The RFC says that signature-params component must be canonicalized as Inner List Structured Field values with parameters. The current implementation is missing parameters serialization. Here is a link to the exact line of code:

items.push(`"@signature-params: ${params}"`);

The fix is to update the code with signature-params serialization. That will include changes for signify-ts, signifypy and any other implementations that are involved into signature generation/validation.

@rodolfomiranda
Copy link
Collaborator

Great catch.

@da-tychinin
Copy link
Author

da-tychinin commented Mar 8, 2024

Here is the signature-base string example before and after the fix. Please take a look at the @signature-params component and DQUOTES in there:
Before (not following the RFC):

"signify-resource": EWJkQCFvKuyxZi582yJPb0wcwuW3VXmFNuvbQuBpgmIs
"@method": POST
"@path": /signify
"signify-timestamp": 2022-09-24T00:05:48.196795+00:00
"@signature-params: (signify-resource @method @path signify-timestamp);created=1609459200;keyid=DN54yRad_BTqgZYUSi_NthRBQrxSnqQdJXWI5UHcGOQt;alg=ed25519"

After (following the RFC)

"signify-resource": EWJkQCFvKuyxZi582yJPb0wcwuW3VXmFNuvbQuBpgmIs
"@method": POST
"@path": /signify
"signify-timestamp": 2022-09-24T00:05:48.196795+00:00
"@signature-params": ("signify-resource" "@method" "@path" "signify-timestamp");created=1609459200;keyid="DN54yRad_BTqgZYUSi_NthRBQrxSnqQdJXWI5UHcGOQt";alg="ed25519"

@rodolfomiranda
Copy link
Collaborator

rodolfomiranda commented Mar 8, 2024

the very last quote is misplaced, right?

@da-tychinin
Copy link
Author

@rodolfomiranda sorry, I've misplaced the examples in the previous comments. I've edited the comment above and now they are correct
about double quotes - there are a few missing:

  • @signature-params component name should be wrapped into double quote - in the example the last one is missing
  • each element of the inner list - the one that is in the round brackets - should be wrapped into the double quote. in the example there is no wrapping
  • each string signature parameter value - key=value pair after the inner list - should be wrapped into the double quote

@psteniusubi
Copy link
Contributor

There may be some more interop issues with the http message signature implementation of keria/signify. I have reported some findings here WebOfTrust/keria#35

@dhh1128
Copy link

dhh1128 commented Mar 11, 2024

This issue and the one raised by @psteniusubi are timely, since the RFC in question just graduated from draft state to a final standard.

@da-tychinin
Copy link
Author

da-tychinin commented Mar 13, 2024

As a part of discussion - here is a doc where I tried to gather differences between signify-ts and HTTP messages signatures RFC - https://docs.google.com/document/d/10y3ed_CJRv9CUM8nLEM-aV5jUQYCAy55ZKVEeVXlSvA
Feel free to comment if anything. The current issue is included into the doc. The issue mentioned by @psteniusubi is also in there

@2byrds
Copy link
Contributor

2byrds commented Mar 22, 2024

@da-tychinin @psteniusubi @dhh1128 @rodolfomiranda @lenkan has there been enough information/comments for us to solidify the updates to approach the RFC? So we can begin updating the keria/signify related repos? If not, what conversations/work is left to do, before the repo changes? This is needed for the upcoming QVI/EBA related use cases so we are all very interested in wrapping this up i think :)

@pfeairheller
Copy link
Member

This is not needed for EBA use cases. The current implementation is working fine. It is at most a nice to have.

@2byrds
Copy link
Contributor

2byrds commented Mar 26, 2024

This is not needed for EBA use cases. The current implementation is working fine. It is at most a nice to have.

In terms of adoption, its helpful to be able to assure customers like EBA that what we're building towards standards. This RFC has been highlighted to the EBA and they are encouraged that we want to work towards this standard.

@pfeairheller
Copy link
Member

This is not needed for EBA use cases. The current implementation is working fine. It is at most a nice to have.

In terms of adoption, its helpful to be able to assure customers like EBA that what we're building towards standards. This RFC has been highlighted to the EBA and they are encouraged that we want to work towards this standard.

That is a straw-man argument. I said nothing about not working towards standards. Having a current implementation that was compatible with the original RFC when built is working towards standards and should be more than enough to prove to anyone that we are working towards standards.

Doing this now is a complication and potential blocker for what is already a tight schedule. Like I said, at best a nice to have, at worst a source of delay.

@2byrds
Copy link
Contributor

2byrds commented Mar 26, 2024

This is not needed for EBA use cases. The current implementation is working fine. It is at most a nice to have.

In terms of adoption, its helpful to be able to assure customers like EBA that what we're building towards standards. This RFC has been highlighted to the EBA and they are encouraged that we want to work towards this standard.

That is a straw-man argument. I said nothing about not working towards standards. Having a current implementation that was compatible with the original RFC when built is working towards standards and should be more than enough to prove to anyone that we are working towards standards.

Doing this now is a complication and potential blocker for what is already a tight schedule. Like I said, at best a nice to have, at worst a source of delay.

It is simple enough to prioritize it as such. I will not stop working on the EBA pilot and don’t expect the evolution of the headers to block anything (other than the effort to update to it and test). The finalization of the spec to leave draft was a ‘big announcement’ that I had the privilege to announce to the EBA as part of the strategy regarding signed headers. If it takes a while to truly comply with the latest spec, so be it. But I see no reason to let @da-tychinin work to stagnate, we can discuss and systematically move forward. If I believe the spec improvements don't benefit the EBA pilot then I will notify the community so they understand the timeline/priority and will not adopt any changes until its appropriate.

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

No branches or pull requests

6 participants