-
Notifications
You must be signed in to change notification settings - Fork 313
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: Add back home domain verification with clarity (v3.0.0) #746
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, otherwise LGTM. Well worded.
Co-authored-by: Alex Cordeiro <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the quick iterations offline :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres some ambiguity in the language added around the relationship between the home domain and authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good 👍 Added a few comments and suggestions to eliminate ambiguity and also to allow future protocol extension by adding more operations to the challenge tx without breaking compatibility.
ecosystem/sep-0010.md
Outdated
- A home domain hosting a [SEP-1 stellar.toml](sep-0001.md) containing a `WEB_AUTH_ENDPOINT` value. | ||
- A web auth endpoint providing the GET and POST operations discussed in this document. | ||
- A client who will authenticate with a Stellar account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listing "home domain" and "endpoint" as "parties" sounds a little off to me, maybe we can improve the wording here? Also it might be better to use the word "wallet" instead of "account" because the spec supports authentication with any Stellar keypair, not necessarily created on the network.
- A home domain hosting a [SEP-1 stellar.toml](sep-0001.md) containing a `WEB_AUTH_ENDPOINT` value. | |
- A web auth endpoint providing the GET and POST operations discussed in this document. | |
- A client who will authenticate with a Stellar account. | |
- A web service which requires authentication with a Stellar wallet. The service must host [SEP-1 stellar.toml](sep-0001.md) containing a `WEB_AUTH_ENDPOINT` and `SIGNING_KEY` values. | |
- An authentication service providing the GET and POST endpoints discussed in this document. | |
- A client who wants to authenticate with web service using Stellar wallet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to add the terms web service here is more prescriptive than we need to be. An authentication service or simply an endpoint on some other service is an implementation detail and a decision the SEP doesn't need to make. The intention of this three point list is to call out upfront what the three things are that are involved: You need a domain hosting a stellar.toml, an endpoint to provide the challenge and token, and a client who is going to talk to the first two.
I applied some changes to the sentences to make it clearer and removed the term parties.
Let me know if I'm misunderstanding the distinction you're seeing that needs to be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the intent of the suggestion, just two recommendations:
- swap
wallet
forclient
. Wallets are a subset of SEP-10 clients. - We need a clear distinction between the SEP-10 service and the home domain that has the TOML. When saying
web service which requires authentication
, it is not clear which one we're talking about. I'd change it toweb service hosting a [SEP-1 stellar.toml](sep-0001.md) file containing a WEB_AUTH_ENDPOINT at the standardized /.well-known/stellar.toml URL path.
ecosystem/sep-0010.md
Outdated
|
||
The discovery flow is as follows: | ||
|
||
1. The client retrieves the [SEP-1 stellar.toml](sep-0001.md) from the home domain. The home domain hosts the stellar.toml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we specify how the client gets from the web service URI to a home domain? Starting from "home domain" may be ambiguous, especially in case of non-default ports (for web service "https://host:port/service/", where port is not 443, clients could come up with either host
or host:port
as a home domain).
1. The client retrieves the [SEP-1 stellar.toml](sep-0001.md) from the home domain. The home domain hosts the stellar.toml. | |
1. At the beginning of the flow client has an URI of the web service requesting authentication and the public key of the authenticating user. Client extracts the authority component from the service URI (as defined by [RFC-3986](https://tools.ietf.org/html/rfc3986#page-17)) and notes its value, which will be referred to as "home domain" for the rest of this spec. | |
1. Client retrieves the [SEP-1 stellar.toml](sep-0001.md) metadata file from the home domain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at applying this change here, however I believe this is unnecessary because to do so is to respecify SEP-1 which is linked here. SEP-1 describes how a client should retrieve a stellar.toml from a domain and what portion of the URL is the domain. https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0001.md#specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think SEP-1 defines what we need here, specifically there's no reference to RFC3986 => no resolution for host:port
vs host
ambiguity. SEP-1 starts with Given the domain DOMAIN
, which is one step ahead of where we are in this spec, and there's a good reason for that. SEP-1 originated from a need to link Stellar account to some metadata which is stored and accessed off-chain. It is not explicitly mentioned in the specification section, but account de facto provides the entrypoint and root context for SEP-1 flow. As far as SEP-1 is concerned, DOMAIN is whatever we find in account's home_domain
field.
On the other hand, with SEP-10 we want to adopt Stellar keypairs as an authentication method for traditional web, which means that we need to carefully define how to plug Stellar concepts into the well-established framework of existing standards, in particular HTTP/1.1: Message Syntax and Routing [RFC7230] and HTTP/1.1: Authentication [RFC7235]. The disposition for SEP-10 is user agent (client) wants to obtain authentication credentials in order to request some representation of the protected resource from the origin server. Resources in HTTP are identified by URI, so one thing we know for sure is that at the start of SEP-10 client is aware of the target URI, but there's no notion of "home domain" or "domain". Without specification every client implementing SEP-10 has to figure out how to convert the URI of the protected resource into the DOMAIN
input expected by SEP-1, and that's not a good situation from the compatibility angle.
There's also a security angle, because any loosely specified term like home domain in security boundary definition has a potential for all sorts of vulnerabilities and attacks. For example, RFC3986 goes into great details defining the calculation of URIs equality, which if implemented incorrectly may result in compatibility and/or security issues down the road. Anchoring this SEP to existing well-established internet standards increases the odds that developers will rely on the proper library implementations, instead of going with simple but potentially dangerous approach (like comparing URIs as strings).
Maybe I put too much emphasis on this and this SEP will probably never see the level of adoption where things like that will become important. But if we can eliminate potential ambiguity and make the spec a little more formal, why not take this opportunity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1000 this change, we need to clarify the home domains format so there aren't mismatches of the same service represented by slightly different home domain representations.
A couple things I want to note:
- Add
the
beforeclient
- Are we sure we want to use the Authority format? I've never seen a username on a home domain IRL. I think we should instead specify it as Host + Port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...one thing we know for sure is that at the start of SEP-10 client is aware of the target URI...
...every client implementing SEP-10 has to figure out how to convert the URI of the protected resource into theDOMAIN
input expected by SEP-1...
I'm not sure this is consistent with what I know of some wallets and their implementations. From the limited wallets I've seen if there's a fixed list of anchors within a wallet it's a domain, not a URL. Our stellar.toml clients in some SDKs take a domain as input and build the URL themselves. Examples of this are:
- Go SDK: https://github.com/stellar/go/blob/38aec89/clients/stellartoml/client.go#L14
- JS SDK: https://github.com/stellar/js-stellar-sdk/blob/6a508cb/src/stellar_toml_resolver.ts#L34-L37
I agree there is some room for improvement here, but I don't think it is detrimental to this changes success, and I still think the improvements you're specifying are improvements to SEP-1. SEP-1 is not limited to account-to-toml lookups, it also intended to generally specify how to go from a domain to a stellar.toml. You're correct that the domain is not sufficiently specified in regards to whether it includes a port or not. Practically this is unlikely to matter because production systems consistently use port 443 and HTTPS, or at least not enough for us to address urgently. None of the Stellar TOML clients in SDKs specify ports.
I think we can address this issue independent of SEP-10 v3.0.0.
I have changed the language at this line slightly in e732188 to more explicitly reference SEP-1, rather than casually reference it.
Co-authored-by: Sergey Nebolsin <[email protected]>
@JakeUrban @accordeiro @nebolsin Thank you for diligently reviewing this change. I've made what I think are the final changes to get this version ready for merging. Please let me know if there is anything else, or anything important I have missed, otherwise I believe this will be merged next week. |
Yes, this needs to be merged by Wednesday EOD. I made a few comments on @nebolsin 's original comments. |
I'll revisit those comments on Tuesday afternoon, then let's re-evaluate merging on Wednesday. If we need a couple more days I think it is okay to move forward with SDK changes on Wednesday given that we're really just iterating on wording that has no functional impact on the interactions between a client and a server. |
ecosystem/sep-0010.md
Outdated
1. The client verifies that the transaction is signed by the `SIGNING_KEY` specified by the requested service's [SEP-1 stellar.toml](sep-0001.md). | ||
1. The client verifies that the transaction has a single Manage Data operation with its source account set to the user's account and value set to a nonce value. The client ignores the home domain included. | ||
1. The client verifies that the transaction is signed by the `SIGNING_KEY` obtained through discovery flow. | ||
1. The client verifies that the transaction has a single Manage Data operation with its source account set to the user's account and value set to a nonce value. The client verifies the home domain included is the home domain from the discovery flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this second sentence should be it's own bullet for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 1d066da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakeUrban @nebolsin Could you take a look at my comments and changes since your last 👀 ?
ecosystem/sep-0010.md
Outdated
|
||
The discovery flow is as follows: | ||
|
||
1. The client retrieves the [SEP-1 stellar.toml](sep-0001.md) from the home domain. The home domain hosts the stellar.toml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...one thing we know for sure is that at the start of SEP-10 client is aware of the target URI...
...every client implementing SEP-10 has to figure out how to convert the URI of the protected resource into theDOMAIN
input expected by SEP-1...
I'm not sure this is consistent with what I know of some wallets and their implementations. From the limited wallets I've seen if there's a fixed list of anchors within a wallet it's a domain, not a URL. Our stellar.toml clients in some SDKs take a domain as input and build the URL themselves. Examples of this are:
- Go SDK: https://github.com/stellar/go/blob/38aec89/clients/stellartoml/client.go#L14
- JS SDK: https://github.com/stellar/js-stellar-sdk/blob/6a508cb/src/stellar_toml_resolver.ts#L34-L37
I agree there is some room for improvement here, but I don't think it is detrimental to this changes success, and I still think the improvements you're specifying are improvements to SEP-1. SEP-1 is not limited to account-to-toml lookups, it also intended to generally specify how to go from a domain to a stellar.toml. You're correct that the domain is not sufficiently specified in regards to whether it includes a port or not. Practically this is unlikely to matter because production systems consistently use port 443 and HTTPS, or at least not enough for us to address urgently. None of the Stellar TOML clients in SDKs specify ports.
I think we can address this issue independent of SEP-10 v3.0.0.
I have changed the language at this line slightly in e732188 to more explicitly reference SEP-1, rather than casually reference it.
ecosystem/sep-0010.md
Outdated
1. The client verifies that the transaction is signed by the `SIGNING_KEY` specified by the requested service's [SEP-1 stellar.toml](sep-0001.md). | ||
1. The client verifies that the transaction has a single Manage Data operation with its source account set to the user's account and value set to a nonce value. The client ignores the home domain included. | ||
1. The client verifies that the transaction is signed by the `SIGNING_KEY` obtained through discovery flow. | ||
1. The client verifies that the transaction has a single Manage Data operation with its source account set to the user's account and value set to a nonce value. The client verifies the home domain included is the home domain from the discovery flow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 1d066da
I'm not particularly worried about SDK users figuring out how SEP-1 works and how to use the SDK for it. The logic is much simpler than some of the basic Stellar concepts like building transactions and submitting them to Horizon. |
@leighmcculloch are we good to merge this? |
What
Add back the home domain verification, but with greater clarity about exactly what the home domain is.
Why
In v2.1.0 the home domain verification that was added in v2.0.0 was removed because the exact contents of the home domain field could be interpreted in at least two different ways that led to inconsistent implementations. The home domain is still present in the SEP-10 challenge transaction and this change provides clarity to what it should contain and how clients should verify it. This change represents the intended understanding of the home domain field. See v2.0.0 and v2.1.0 for prior context.
Notes
Servers should use this change to verify that they are placing the correct value for the home domain into the challenge transaction. Once clients implement v3.0.0 they will reject challenge transactions from servers who are providing home domain values that are incorrect.
This is an alternative proposal to #741.