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-10 should define a network passphrase used when signing challenge transaction #339

Closed
bartekn opened this issue Jul 4, 2019 · 8 comments · Fixed by #345
Closed

SEP-10 should define a network passphrase used when signing challenge transaction #339

bartekn opened this issue Jul 4, 2019 · 8 comments · Fixed by #345

Comments

@bartekn
Copy link
Contributor

bartekn commented Jul 4, 2019

Transaction signatures depend on the network passphrase. Challenge endpoint (and SEP-10 itself) do not define what network should be used when signing so server and client can use different networks. SEP-10 should define which network should be used for signing (probably pubnet).

@tomquisel
Copy link
Contributor

Thanks @bartekn, this is an interesting point! I'm not sure it's necessary to add to SEP-10, though. I would imagine that organizations have different domains for testnet and pubnet, and the onus is on the client using the SDK to know which domain needs to be hit depending on if they want to run on testnet or pubnet.

I think this is similar to the way Horizon is usually used. The client decides if they want to operate on testnet or pubnet, and based on that they pick the Horizon URL as well as the passphrase they'll configure in the SDK.

@bartekn
Copy link
Contributor Author

bartekn commented Jul 5, 2019

It really depends what SEP-10 was meant for. From description:

This SEP defines the standard way for clients such as wallets or exchanges to create authenticated web sessions on behalf of a user who holds a Stellar account. A wallet may want to authenticate with any web service which requires a Stellar account ownership verification (...).

So it's de facto the authentication standard implemented by various Stellar wallets (like Lobstr, Centaurus etc). Imagine there are 2 wallets, one is using pubnet passphrase and the other one is using testnet passphrase. They communicate with some SEP-10 server. If SEP-10 server is using pubnet, the wallet using testnet passphrase won't work.

One option to solve this is to pass the passphrase the client is using with the first challenge request but it will be much easier (and less error prone) if we decide on a single network passphrase (can even be something like SEP-10). Servers always send random bytes so sessions created in test environments won't work in production.

@tomquisel
Copy link
Contributor

tomquisel commented Jul 8, 2019

Gotcha, I see your point.

Let's say we decide that SEP-10 uses a fixed passphrase, like the pubnet one.

Here's the scenario I think it's good to avoid:

When I'm running my wallet in testnet mode, and if I'm using the JS SDK, I'll call useTestNetwork() to set it up. From there, here are the steps the wallet takes to perform a testnet withdrawal of USD:

  • request assets from testnet Horizon
  • look up stellar.toml file for USD issuer (hosted at testnet.usd.com)
  • grab WEB_AUTH_ENDPOINT from the stellar.toml (say the result is testnet.usd.com/auth)
  • do GET testnet.usd.com/auth/challenge
  • switch password with usePublicNetwork()
  • verify the signature on the challenge from the server (using pubnet pass)
  • sign the challenge, send it back to testnet.usd.com/auth/token (using pubnet pass)
  • make a request to testnet.usd.com/withdraw to find withdrawal address
  • switch password with useTestNetwork()
  • create and sign (with testnet pass) the withdrawal payment transaction

And of course the testnet.usd.com server needs to do the same thing. Mostly it uses the test network password, but for SEP-10 signing and verification, it needs to switch to pubnet.

The goal is to avoid the client and server having to switch back and forth between testnet and pubnet passwords in a single session.

I think the best solution is to define a convention for whether the testnet or pubnet password is used. I'd suggest: for production systems, or systems that interact with the Stellar pubnet, use the pubnet password in SEP-10. For testing systems or systems that interact with the Stellar testnet, use the testnet password in SEP-10.

What do you think?

@bartekn
Copy link
Contributor Author

bartekn commented Jul 8, 2019

Sounds good but then we need to update the challenge request to accept the network passphrase.

@tomquisel
Copy link
Contributor

I don't think so. The idea is it's a convention and doesn't need to be negotiated in the protocol. The caller knows if they're hitting a test server or a prod server, and knows to sign with the appropriate pass phrase. Similarly the server knows if it's a test server or prod server, and knows to always use the appropriate pass phrase.

@bartekn
Copy link
Contributor Author

bartekn commented Jul 9, 2019

I remember a bug that was really hard to track because Horizon server (with testnet passphrase) was connected to Stellar-Core (with pubnet passphrase). A fix (stellar/go#815) was to check if the passphrases match. So would be great to expose what passphrase the SEP-10 server is using but then you always need to make additional request before challenge request to ensure the passphrase is correct.

@tomquisel
Copy link
Contributor

That's fair, it does seem valuable for a client to be able to check if its passphrase matches the server's. I'd love if we could add that in a backwards compatible way that adds minimal extra complexity.

How about the server's response to /challenge contains both a transactions and a network_passphrase field? The client will then have the information available if it wants to check, but no mandatory complexity is added to the flow.

@bartekn
Copy link
Contributor Author

bartekn commented Jul 9, 2019

Sounds good to me!

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 a pull request may close this issue.

2 participants