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-0010: Revert home domain verification and relax manage data operation constraints (v2.1.0) #740

Merged
merged 5 commits into from
Oct 14, 2020

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Oct 7, 2020

What

Revert home domain verification and relax manage data operation constraints allowing additional manage data operations as long as their source account is the server account.

Why

There's been some ambiguity about the intended contents of home domain and in the interest of encouraging compatibility between servers and clients I think we need to consider reverting verification the SEP and SDKs are requiring on that field.

The relaxing of the manage data operations so that we can add a new manage data operation in that adds a new hostname for verification. Allowing additional manage data operations with the server account as the source account will allow future releases to add additional fields of data without breaking older clients. Requiring the source account to be the server account ensures that if there is a need in the future to add new fields that the client should confirm and sign for, that older clients won't blindly sign those challenge transactions.

This change is backwards compatible with v2.0.0.

Example Implementations

stellar/go#3108
stellar/js-stellar-sdk#580

@leighmcculloch leighmcculloch self-assigned this Oct 7, 2020
@leighmcculloch leighmcculloch changed the title SEP-0010: Revert home domain verification and relax manage data operation constraints SEP-0010: Revert home domain verification and relax manage data operation constraints (v2.1.0) Oct 7, 2020
@stellar stellar deleted a comment from JakeUrban Oct 8, 2020
@JakeUrban
Copy link
Contributor

@nebolsin Can we get your thoughts here? Thanks for being patient with us as we develop a plan for these changes. The Ruby SDK already has this functionality correct? It was good intuition on your part.

Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

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

A couple suggestions but looks good

ecosystem/sep-0010.md Outdated Show resolved Hide resolved
ecosystem/sep-0010.md Outdated Show resolved Hide resolved
ecosystem/sep-0010.md Outdated Show resolved Hide resolved
@leighmcculloch
Copy link
Member Author

@JakeUrban Thanks. I made those changes to the protocol.

@JakeUrban
Copy link
Contributor

I'm slightly curious/concerned that allowing N Manage Data Ops will give room for anchors-client pairs to start using that functionality to pass non-standardized key-value pairs. I don't know how to feel about that, what do you think? @leighmcculloch

@leighmcculloch
Copy link
Member Author

@JakeUrban That's possible. It's not clear to me that it'd be a problem if that happened. As long as a server doesn't include operations that are intended for the client to sign/authorize for, which is prevented by the requirement that the source accounts of any additional ops be the server account.

An anchor-client pair could do this today without this change too. And in both cases they need to modify the SDKs since the challenge transaction build functions don't allow adding arbitrary manage data ops.

An alternative would be to add the server_hostname manage data op that we're adding in SEP-10 v3.0.0 as a reserved for future use manage data op in this change. I think that is unnecessarily specific and binds the releases together.

@JakeUrban
Copy link
Contributor

And in both cases they need to modify the SDKs

But that won't be the case with this change. A specific client-server pair could have 2 additional manage data ops to store account data, say. They expect the home domain op first as specified by the standard, then the 2 custom ops. With SEP-10 3.0, the order changes. The second Manage Data op will be something different than it used to be, which would likely require them to adjust the expectations in their code.

@leighmcculloch
Copy link
Member Author

In v3.0.0 the order of additional ops after the first op isn't intended to matter. Let's discuss on #741 how we can clarify that in v3.0.0.

Copy link
Member

@accordeiro accordeiro left a comment

Choose a reason for hiding this comment

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

The changes look good – I left a few suggestions to reduce ambiguity 👇

ecosystem/sep-0010.md Outdated Show resolved Hide resolved
ecosystem/sep-0010.md Show resolved Hide resolved
ecosystem/sep-0010.md Show resolved Hide resolved
@gekpp
Copy link

gekpp commented Oct 10, 2020

Hi everybody.
On behalf of Tempo Money transfer, EURT asset issuer I'd like to vote for this PR.

Let me describe why.
Recently we faced the limitation of having single manage data operation in the challenge transaction.
We wanted to add more key-value pairs for being included in the resulting JWT. More specifically, we wanted to add auth token expiration time and ACL. The idea was the wallet backend knows a way to learn user's permissions, the wallet backend interacts with payment system backend via SEP-XX protocols. We want to implement the flow of issuing SEP-10 token as follows:

  1. Wallet backend learns user's access rights from passport service.
  2. Wallet backend learns the expiration time for particular use-case. It might depend on front-end application using the wallet backend.
  3. Wallet backend requests a challenge via GET /challenge sending extra query parameters (required expiration time and ACL) along with account.
  4. Payment System Backend verifies extra parameters and adds them as manage data operations to transaction, signs and replies to the requestor.
  5. Wallet backend validates that manage data equal to what was requested, signs a transaction and POST it for issuing the JWT.
  6. Payment System Backend iterates over manage data operations and adds them as claims into JWT.

This approach will give the possibility to extend resulting JWT with any data the parties agreed on.
There are different approaches of implementing adding extra params to JWT but all of them require introducing more connection between backend components and we would like to avoid it if possible.

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