signify-ts icon indicating copy to clipboard operation
signify-ts copied to clipboard

signature-params component of signature-base is not serialized

Open da-tychinin opened this issue 1 year ago • 12 comments

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: https://github.com/WebOfTrust/signify-ts/blob/e2b2f6e3d9ecd4d1ae22e93ab82ded4ef366312b/src/keri/core/httping.ts#L103

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.

da-tychinin avatar Mar 07 '24 18:03 da-tychinin

Great catch.

rodolfomiranda avatar Mar 08 '24 12:03 rodolfomiranda

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"

da-tychinin avatar Mar 08 '24 18:03 da-tychinin

the very last quote is misplaced, right?

rodolfomiranda avatar Mar 08 '24 18:03 rodolfomiranda

@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

da-tychinin avatar Mar 08 '24 19:03 da-tychinin

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

psteniusubi avatar Mar 09 '24 12:03 psteniusubi

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

dhh1128 avatar Mar 11 '24 15:03 dhh1128

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

da-tychinin avatar Mar 13 '24 17:03 da-tychinin

@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 :)

2byrds avatar Mar 22 '24 23:03 2byrds

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

pfeairheller avatar Mar 26 '24 14:03 pfeairheller

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.

2byrds avatar Mar 26 '24 14:03 2byrds

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.

pfeairheller avatar Mar 26 '24 16:03 pfeairheller

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.

2byrds avatar Mar 26 '24 16:03 2byrds