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

SEP 6,12,24,31: deprecate X-Stellar-Signature in favor of Signature #1333

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

JakeUrban
Copy link
Contributor

resolves #1328

Updates the name of the signature header for callback requests from X-Stellar-Signature to Signature.

While this is technically a breaking change, the addition of the callback signature mechanism is new enough to not be concerned making existing implementations noncompliant.

leighmcculloch
leighmcculloch previously approved these changes Jan 7, 2023
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Would their be value in making this a non-breaking change, by saying that applications should publish both headers until the next breaking change that removes the older X-Stellar-Signature? If no, 👍🏻.

@howardtw
Copy link
Contributor

howardtw commented Jan 9, 2023

by saying that applications should publish both headers until the next breaking change that removes the older X-Stellar-Signature

I think it's safer to do so for a smooth transition. Otherwise, the application (wallet backend) would have to check for both X-Stellar-Signature and Signature headers whenever a partner is making the change.

@C0x41lch0x41
Copy link
Contributor

I made a similar comment about backward compatibility here. Otherwise looks good to me.

C0x41lch0x41
C0x41lch0x41 previously approved these changes Jan 9, 2023
@JakeUrban
Copy link
Contributor Author

JakeUrban commented Jan 9, 2023

Ok, we'll go with the following approach:

  1. Anchors send the signature using both X-Stellar-Signature & Signature
  2. Wallets check for signatures using both X-Stellar-Signature & Signature. Callbacks should be discarded if their values do not match? Or since X-Stellar-Signature is deprecated, maybe Signature is used in this case?
  3. When the RFC is finalized, we'll remove X-Stellar-Signature

@JakeUrban JakeUrban merged commit fb94478 into master Jan 9, 2023
@JakeUrban JakeUrban deleted the callback-signature-name branch January 9, 2023 22:45
@JakeUrban JakeUrban changed the title SEP 6,12,24,31: replace X-Stellar-Signature with Signature SEP 6,12,24,31: deprecate X-Stellar-Signature in favor of Signature Jan 9, 2023
@C0x41lch0x41
Copy link
Contributor

I was thinking of a less restrictive/complicated approach:

  1. Anchors send X-Stellar-Signature (old code) or Signature (new code)
  2. Wallets check for X-Stellar-Signature or Signature. If both exists, values should be the same.
  3. When RFC is finalized, wallets can drop support of X-Stellar-Signature

That allows Anchors to not have to update an existing code for now.

@JakeUrban
Copy link
Contributor Author

I think the approach we took allows outdated wallets & anchors still using X-Stellar-Signature (and not Signature) to remain compatible with wallets & anchors that have implemented the use of both fields.

If we specified that anchors could send Signature without X-Stellar-Signature, all existing wallets would need to update.

@C0x41lch0x41
Copy link
Contributor

remain compatible with wallets & anchors that have implemented the use of both fields.

I think I am missing something here. Today we only use X-Stellar-Signature correct? So wallets and anchors only implemented this field. Using Signature is new, that's why I was suggesting an approach where either X-Stellar-Signature or Signature are valid.

@JakeUrban
Copy link
Contributor Author

This doesn't work for a new anchor (using Signature) & old wallet (uses X-Stellar-Signature). @howardtw maybe this is acceptable given that Vibrant is the first to implement this?

@howardtw
Copy link
Contributor

To work with multiple anchors, before all of them adopt the new Signature header, the wallet needs to validate both X-Stellar-Signature and Signature headers unless they both switch to the new header at the same time, which is unlikely.

@JakeUrban
Copy link
Contributor Author

Thats right, is that acceptable? MoneyGram is unlikely to send both, so Vibrant will likely need to check for both anyway. I agree with @C0x41lch0x41 that allowing anchors to send one header is the simpler approach.

@howardtw
Copy link
Contributor

Yes, I can have Vibrant check for both until all the supported anchors migrate to the new header.

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.

SEPs (6, 12, 24, 31): Update callback header from X-Stellar-Signature to Signature
4 participants