SEP-10: Add multi-sig capabilities #489
Conversation
|
Note: I didn't add the list of signers to the challenge transaction, or to the initial request. The reason for this is because it's unnecessary and it doesn't reduce the number of requests we have to make to external systems to validate the signers are on the account. At best it would give the implementor the option of moving that check to the first request rather than the second. Another reason for why to not put the signers in the initial request would assume the caller knows who they are in the first request. It's feasible a wallet won't know what the signers will be until they present the challenge transaction to the user for signing. |
|
One question is how we handle SEP-10 for users who are currently using services like StellarGuard. StellarGuard submits transactions directly to the network rather than passing signed transactions back to the wallet, so currently there's no way for StellarGuard to add its signature to a SEP-10 transaction. This just happens to be OK in the current SEP-10, because with StellarGuard the user continues to hold the master key, and all that the current SEP-10 requires to authenticate is a signature from the master key. So even though the master key only has half weight, it's enough to authenticate. While this is convenient for users of StellarGuard, I don't think it's a good call going forward. It shouldn't be too difficult to fix StellarGuard to work with the new SEP-10 by modifying it to pass signed transactions back to the client instead of sending them directly to the network. @pselden does this seem ok to you? |
|
It will take me a while to read through the proposal and comments here to see if it will works out, but StellarGuard already supports passing the signed transaction back to the client through a callback parameter. It would be up to the clients to use that and implement the receiver of the callback. However, when I last did a survey (admittedly it’s been about 6 months), over half the users said they used the Stellar Account Viewer, which probably would not support this. As long as that sort of usage is still supported it’s all good. |
The Stellar Account Viewer doesn't provide SEP-10 support so it sounds like many of the users will be unaffected by this change.
@pselden Thanks for pointing us to the callback, I hadn't seen that feature. I gave it a go at test.stellarguard.me and it worked great. These are things I observed in relation to using it for SEP-10 challenge transactions:
Overall this doesn't seem like a blocker. Wallets/clients will need implement the callback if they are not using it today and for a period of time some wallets may become incompatible with an anchor for a subset of users until they add it.
|
|
Another service with a similar API and behavior as StellarGuard is Lobstr Vault (API details here). The use of a callback and/or the idea 💡 above would also be a way forward for it once these changes to SEP-10 are merged and start being picked up by anchors and others. @pitsevich @dmgmyza Does this seem ok to you? |
Add SEP-10 web authentication implementation based on SEP-10 v1.2.0 that requires the master key have a high threshold for authentication to succeed. We need a standalone server implementation of SEP-10 for the mobile-wallet and this provides a server supporting the absolute basics of the existing SEP-10 protocol. The SEP-10 protocol doesn't define what threshold a server should require a signing master key to have on an account, but for the sake of demonstration and our use case it requires the high threshold. It could be configurable but isn't at the moment. This implementation has been written with the proposal in mind that we are making to SEP-10 (stellar/stellar-protocol#489) also, and already sets up the test cases with where we expect multi-sig to go but has those tests set with expectations that are appropriate given the limitations of SEP-10 today. This application is not polished which is why it is being added to the `exp` package and why this is a draft PR.
Yes I can implement that. I'm also planning to add an option where the user can choose whether or not to submit ANY transaction.
I actually have a SEP for this that languished because it paradoxically got too complicated AND didn't address enough use cases: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0019.md
I know that the actual usage of the callback is around 2% of all transactions submitted to StellarGuard -- so not many wallets are using it (or at least the more popular ones are not). |
|
I've completed the work on the Go SDK to add support for these changes in stellar/go#2071, and stellar/go#2072 adds an example of how this would be used in a server that already implements SEP-10 today. It does require a little more work on an implementer, but I think the SDKs will abstract most of it. @tomerweller @msfeldstein If you think this change to the protocol is good and how it is demonstrated in code at those links is good, I'd like to validate that the SEP-10 change is good from the perspective of Polaris and the Integrations team by having the change made to Polaris too. Do you folks have capacity to help with that? cc @accordeiro |
|
I love the idea of building this into Polaris! Seems like a nice way to enable us to start implementing/testing things on the client side (like for Vega) too :) |
|
Thanks for making these changes. Think it's important to have multisig accounts play nicely with SEP-24 servers. I think that it also makes sense to increase the recommended time bounds: For accounts protected with multi-sig setting On a separate note, we've recently seen issues with one of the anchors and a I assume, a server time on the anchor's side was off by a second or so (with server time being ahead of real time), which was resulting in a I'm not really sure if this issue was caused by the specifics of the implementation of this particular anchor and whether it would still apply to other implementations of this SEP, but mentioned this to make sure this SEP approach is not too sensitive to the server time. |
|
@pitsevich Thanks this is really helpful point to have surface. I think increasing the time limit from 5 minutes maybe to 15 minutes would be okay to provide time for multiple-signatures to be attached. Do you think that would be sufficient or do you have a idea for what time limit would be appropriate? The time period is also the period within which a challenge could be reused to get a new JWT so we should keep it as low as practically possible. For the issue of dealing with clock skew between services, could you open an issue on this repository about the issue of clock skew in SEP-10 and we can discuss it separately? Thanks! |
|
@leighmcculloch I was thinking about 30 minutes but 15 minutes should be enough in most cases as well. |
This is quite important for us, since a lot of users are using multi-sig with StellarTerm. We will be rolling out SEP-24 to StellarTerm shortly, so users who would want to make a withdrawal/deposit on StellarTerm and have multisig through Vault or StellarGuard would be having difficulties. The wallets that have a backend could listen for the callback from services like Vault or Stellarguard and collect all signatures before sending to SEP-10 server for JWT token. So I think that the idea 💡 above is the way to go and would work with all sorts of wallet apps. We would be willing to build something like this into Vault and StellarTerm. Earlier we've been talking with @leighmcculloch about whether it would make sense to have the SEP-10 server be able to accept individual signatures for the transaction signing request separately from multiple services, but thinking more about that I think it would the implementation for anchors too difficult. PS: added some edits |
@pselden @pitsevich Would you be up for exploring this idea independently of this change in parallel? Since both of your APIs are the same or very similar today, if you can add this capability in the same way and work with StellarTerm to integrate it, that might be a good point to document this API in a SEP. What do you think? |
|
Yes I'm up for that. We can use https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0019.md as a starting point or scrap it for something simpler. |
|
I'm planning on merging this change and locking it in on Monday January 27th 10am PST, along with the relevant Go SDK changes stellar/go#2071 and the update to our reference implementation stellar/go#2072. @JakeUrban has started work on updating Polaris and it'll come after. |
Add new challenge transactions helpers to `txnbuild` to support verifying a challenge transaction that has multiple signatures. Add three new functions: - `ReadChallengeTx`: Read the details like the transaction and the client account ID out of the transaction. - `VerifyChallengeTxSigners`: Verify that the signers of the transaction are a subset of a list of signers given to the function. - `VerifyChallengeTxThreshold`: Verify that the signers of the transaction are a subset of a list of signers that meet a required threshold. Deprecate `VerifyChallengeTx`. Why: The SEP-10 change that is open in stellar/stellar-protocol#489 clarifies how an implementer should verify that signers of the transaction are signers on the account and that accounts may have multiple signers. An implementer needs to read out of the transaction the client account before verifying the transaction. For this reason we need two functions, one to read out details we need before verification and one to perform verification. The read call also validates the server signature because no challenge transaction would ever be valid to read if it wasn't signed by the server. Known limitations: These helper functions take a decent amount of complexity away from an implementer, but they still leave a decent amount up to the implementer. An implementer would still need to make a call to Horizon. I think it might be out-of-scope of the `txnbuild` package to offer that additional logic and SEP-10 technically leaves the exact details of what happens there up to the implementer so it would also be difficult to implementer in a library function.
Update the SEP-10 implementation to support the changes in stellar/stellar-protocol#489. The SEP-10 reference implementation should be kept up-to-date with SEP-10 as it changes, and making these changes now ahead of the protocol change ensures we are validating what the change will require of other implementors.
|
Please can anyone on here please remove vault signers from lobstr and stellar expert. Tell me how or how to do in laboratory. Step by step please… |
|
@krudder64 I recommend asking either on StackOverflow or the #support Discord. |
What
Add requirements to SEP-10 that a server validate that the client key that has signed the challenge transaction is actually a signer of the account. Add details about how multiple signatures on a challenge transaction are a possibility.
Why
SEP-10 has the goal to provide the mechanics to help a server get proof that a client holds a Stellar account, but if a server was to implement only the functionality described in this document it would only be proving that the client is in possession of the secret key of the account's address, the master key, and not necessarily the Stellar account. The checks required to go beyond that have until now been left up to the implementer to define.
Proving possession of a secret key doesn't prove any threshold of control of an account because an account can alter its signers. The master key for an account may have its weight reduced to zero which removes the master key as a signer, and so possession of a master key is not proof of control of an account.
It's important that implementers of SEP-10 take an extra step that is not currently discussed in the document. That extra step is to verify that the key is a signer on the account and that the weight of that signature is sufficient from the perspective of the service/anchor for whatever interactions taking place.
It's also possible for accounts to have multiple signers where no single signer will have sufficient weight to meet any threshold. In this case it's important for SEP-10 to allow a combination of signatures to prove control of the account.
This change is important for supporting applications that do not use the master key as a signer, that rotate the signing key for a single signature account, and for accounts that have multiple signers.
SDK Changes
This change is backwards compatible in terms of how it works, but it will essentially make the verification functions in the SDKs deprecated since those functions assume the signer of the challenge transaction is always the master key.
CC
@tomquisel @nebolsin – Original authors of SEP-10.
@tomerweller @msfeldstein