SEP-0007 network selection

[orbitlens]

While trying to implement SEP-0007 in stellar-expert-id I faced a few problems related to network selection.

If the source account in the xdr is all zeros then the URI handler should replace the source account and sequence number with the user's source account and sequence number before it signs it.

The network_passphrase parameter is not sufficient for custom networks because a signer has to fetch the source account data from Horizon first when replacing the source account sequence. For public and testnet networks the signer may use default Horizon servers (https://horizon.stellar.org and https://horizon-testnet.stellar.org respectively).

"All zeros" in description is ambiguous. I used buffer of zero integers in my implementation (equivalent to GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF public key). However the phrase may be also interpreted by developers as buffer of '0' (int 48) characters which gives us completely different public key.

The msg parameter may be used by malicious actors to mislead a user by providing messages like "Approved by SDF" alongside with unsafe transactions.

I have the following proposal:

1. Add optional parameter horizon. Motivation: allow custom Horizon instances for public and private networks.
2. Use parameter network instead of network_passphrase. It can be 'public', 'testnet', or arbitrary private network passphrase. Motivation: unify network selection while providing horizon defaults for 'public' and 'testnet' networks.
3. Remove msg parameter from the spec. Motivation: protect users from misleading comments.

[MisterTicot]

Hi, I did it this way in cosmic links (upcoming release) :

• network parameter, that can be public, test or passphrase
• server parameter, that define the horizon instance - it's different from callback. Maybe using horizon is more explicit.

I did the same comment about msg and I'm glad at least someone else understand the issue.

[MisterTicot]

@orbitlens

I'm actually pondering whether I should allow for defining arbitrary horizon url into cosmic links. While it makes sense in the context of using custom networks, it's kind of weird to offer that option over known networks.

I may rather use something like &network=network:horizon where network can be either 'public', 'test' or a passphrase, and horizon is an optional fallback horizon instance to use for that network in case cosmic-lib/wallet doesn't provide one.

The rational here is that under normal conditions the wallet should define which horizon node to use, not the transaction request; and this rule should be broken only when the wallet have no clue where to reach an horizon node for a specific network.

Can you tell me what you think about that please?

[MisterTicot]

Or maybe keep the '&horizon=' syntax but only use its value when wallet doesn't define any horizon node for a given network.

[orbitlens]

I think that it would be better to give the priority in Horizon server selection to the link creator.

So far almost all transactions are submitted through SDF-maintained Horizon instances (https://horizon.stellar.org and https://horizon-testnet.stellar.org). However, SDF stated multiple times that they won't provide guaranteed SLA in the future and encouraged anchors to use their own Horizon instances. Again, custom Horizon instance can be used, say, as a multi-sig coordinator, or trigger some custom logic.

network=network:horizon scheme results in a possible collision, because a : character may be a part of the network seed. Therefore a colon character can't be used as a delimiter, moreover, encodeURIComponent(':') encodes it as %3A. Besides that, a delimiter makes the scheme more complex. IETF standards discourage mixing custom delimiters with "key=value" pairs in query string.

That's why a separate optional horizon parameter that overrides default Horizon URL looks like a more robust and future-proof option to me.

@nikhilsaraf What do you think?

[MisterTicot]

I never said that SDF nodes would be used. I'm saying it would be up to the wallet to decide which horizon node to use, and ultimately to the user assuming the relevant UI is provided. For example, if you look at myEtherWallet, you'll see that a scroll box allow user to do that kind of choice. So the decentralization issue is irrelevant here.

When you're talking about "custom horizon", it feels to me that you're mentioning functionalities that should be handled by the callback option. The transaction recipient doesn't have to be a horizon server at all, by the way.

Now the difference between horizon and callback options is that horizon is also where the account data are fetched from. I see very little interested allowing transaction emitters to control that part, except in cases where no network horizon node is known.

[MisterTicot]

My concern is actually that allowing transaction emitter to override horizon url could open the door to some kind of hairy attack; I didn't figured out the details yet, but I would rather not allow it if I don't see a good reason to do so.

[orbitlens]

My concern is actually that allowing transaction emitter to override horizon url could open the door to some kind of hairy attack

I thought about it, but can't imagine how this could be exploited. There are no important data that can be forged when fetched from the malicious server. An account sequence is the only important param here. In the worst possible case the tx will fail.

When the transaction is submitted to the malicious Horizon, it can accept it or not. The tx modification is impossible (well, not impossible, rather pointless), so it can't be a vector for MITM attack. Again, in the worst case scenario the transaction won't be submitted to the Network.

[MisterTicot]

Found it. The pay operation is supposed to give user the choice of which asset to use for the payment. Account balance is fetched from horizon as well as order books data from which the path payments can be computed. I guess that leaves the doors open to something pretty bad.

Once again beside the question of possible attacks there's the question of whether this would have any use anyway. If you find any use case for overriding horizon instance please let me know, as I didn't yet.

The denial of validation attack is also possible with callback service. I think I will simply make it clear for users when transaction are not sent directly to the blockchain that they are giving confidence to a tier service.

[MonsieurNicolas]

I agree that providing a horizon url like this is strange: the network passphrase is supposed to be a globally unique identifier for a network; then clients just need a mapping UUID -> set of horizon servers that they trust.

If clients want to perform more checks based on ledger information (like verify assets in the transaction), they will need to query and trust horizon for it.

[MisterTicot]

@MonsieurNicolas The point is to support any custom (potentially private) network in SEP-0007/CosmicLinks. Then, you'll be able to use you Stellar Authenticator / Stellar Expert ID / Sep-0007 wallet with your home-made blockchain.

One could argue the horizon node setup should be made directly in the wallet. I kind of feel it could be convenient to have the option to share a default starting point in the transaction link.

[orbitlens]

Ok. I think we all can agree that Horizon selection is up to the signer(wallet) app, as @MisterTicot mentioned.

Therefore, I propose to make query parameter horizon required only if a custom network specified (when parameter network contains a network passphrase). It's up to the signer whether to ignore the horizon parameter for well-known networks (public and testnet) or show the Horizon selection dialog. This behavior may be changed in the future.

[MisterTicot]

I would put it like this:

• horizon parameter (optional, only with network_passphrase) Provides a fallback horizon URL in case the wallet doesn't knows any for network_passphrase. By default, wallet must use its own horizon node setup for security reasons. However, it might allow the user to use the provided horizon punctually. If horizon is not provided and no horizon URL for network_passphrase is known, the wallet should offer to enter one.

I would leave this parameter as optional in any cases, as in the future a side-network might become known to the point it doesn't needs to burden links with an horizon URL anymore. @orbitlens would you agree with this formulation?

[orbitlens]

Yes, agree. It makes sense. The only thing that confuses me is a network_passphrase parameter. I don't think that it would be wise to support both network and network_passphrase. Let's use single network parameter as I proposed originally.

• horizon parameter (optional, only when network contains a network passphrase) Provides a fallback Horizon URL in case the wallet doesn't knows any for network. By default, a signer must use its own Horizon node setup for security reasons. However, it might allow the user to choose the provided horizon punctually. If horizon is not provided and no horizon URL for network is known, the signer should offer to enter one.

[MisterTicot]

Ok, so we end up with the following proposals:

• network parameter (optional) - Only need to set if this transaction is for a network other than the public network (URL-encoded network passphrase, or 'test').

Rationale: This change strip out a few characters ('_passphrase', testnet passphrase), which will make QR code easier to scan.

• horizon parameter (optional, only with network) Provides a fallback horizon URL in case the wallet doesn't knows any for network. By default, wallet must use its own horizon node setup for security reasons. However, it might allow the user to use the provided horizon URL punctually. If horizon is not provided and no horizon URL for network is known, the wallet should offer to enter one.

Rationale: This improve compatibility of this SEP with custom/private network while retaining a satisfying level of security.

Remove msg parameter

Rationale: This parameter allow untrusted source to display a message in a trusted environment, which isn't a good practice. Moreover, lengthening the URL by up to 305 characters will makes the QRcodes harder to scan especially on low-end hardware. The cost/benefit of this feature is therefore negative. msg should be included in transaction emitter document instead.

[nikhilsaraf]

@orbitlens and @MisterTicot I've responded to your points towards the end of this comment, but there is some new information I wanted to share first which will provide more context on my responses below.

Proposed Updates to SEP-0007

The current version of the SEP-0007 spec is ambiguous about how it represents a source account needing replacement (specifically, the “all zeros” statement). When we initially wrote the SEP-0007 spec, we did not have a standardized way to represent specific fields in an XDR. However, there is a new SEP proposal (#159) that, as a subpart of the proposal, eliminates this ambiguity by introducing a canonical way to reference fields in a transaction. It does this by depending on the XDR definition of a TransactionEnvelope.

New Substitution Logic

Using the field names as described in the new SEP proposal, we can simplify the URI scheme by adding a replace parameter with the fields that need to be replaced. If you want to replace multiple fields you can repeat the replace key to include additional values. Here is an example of a SEP-0007 request that needs the source account replaced by the wallet before signing: web+stellar:tx?xdr=AAAAAP%2Byw%2BZEuNg533pUmwlYxfrq6%2FBoMJqiJ8vuQhf6rHWmAAAAZAB8NHAAAAABAAAAAAAAAAAAAAABAAAAAAAAAAEAAAAA%2F7LD5kS42DnfelSbCVjF%2Burr8GgwmqIny%2B5CF%2FqsdaYAAAAAAAAAAACYloAAAAAAAAAAAA&replace=tx.sourceAccount&replace=tx.seqNum

The values tx.sourceAccount and tx.seqNum are taken from the XDR definition of a Transaction Envelope and XDR definition of a Transaction as is defined in the new SEP proposal along with the combination rules defined there.

Special Handling for tx.seqNum

Since the tx.seqNum needs to always be replaced if we replace the tx.sourceAccount, wallets should add a special condition whereby if the tx.sourceAccount needs replacement then the tx.seqNum should also always be replaced. This means the above URI request can alternatively be represented as the one below while being functionally equivalent: web+stellar:tx?xdr=AAAAAP%2Byw%2BZEuNg533pUmwlYxfrq6%2FBoMJqiJ8vuQhf6rHWmAAAAZAB8NHAAAAABAAAAAAAAAAAAAAABAAAAAAAAAAEAAAAA%2F7LD5kS42DnfelSbCVjF%2Burr8GgwmqIny%2B5CF%2FqsdaYAAAAAAAAAAACYloAAAAAAAAAAAA&replace=tx.sourceAccount

Conclusion

This approach affords us very powerful substitution logic whereby we can replace very specific parts of an XDR, for example, specifying the field tx.operations[0].body.paymentOp.amount as a value to the replace key would replace the amount of the payment operation in a transaction which can be used for donations to a specific account. In the case of any replacement it is up to the user that signs the transaction to determine what values to fill in. This ensures that the user is in full control of the transaction being created and signed.

This allows us to be consistent with all the Motivations outlined in the original SEP-0007 specification, specifically Future-Proof and Standardized.

Responses to Points

1. SEP-0007 is independent of horizon — you can submit it to any horizon instance that you want. In order to fetch the sequence number for an account correctly you should rely on the network_passphrase to indicate the related network for the request — the owner of an account should already have a mapping of network_passphrase to horizon instance mapping given that their wallet would need this mapping to begin with in order to submit regular transactions. I hope this rationale alleviates your concerns over the horizon parameter, however, I may have misunderstood your use case for including this information in the request -- if so, would you mind elaborating on the use case?

2. network_passphrase is intended to bind the signed XDR to a specific network. If you are using the public or test networks you should use the respective network_passphrase and you can submit to any horizon instance connected to the network associated with the network_passphrase. If you are using a private network then use this field to specify the network_passphrase for your private network. Is the concern over using network instead of network_passphrase only the length?

3. Agreed. There is definitely a lot of value in removing the msg field for purposes of security, I’ll include that in the update to the spec.

I hope I’ve understood your concerns and use cases and that the responses above address them. I'm happy to discuss alternatives or poke holes in anything mentioned above.

[orbitlens]

the owner of an account should already have a mapping of network_passphrase to horizon instance mapping

It's impossible.

Let's imagine that the signer app maintains the network-horizon mapping. I create my private network for testing purpose. None of the signer apps will allow me to use account sequence substitution or direct account submission (without callback) out-of-the-box, without code modification. There's simply no Horizon mapping for my freshly-created network and I need to jump through hoops instead. Or I just add a horizon parameter to the link and everything works.

Is the concern over using network instead of network_passphrase only the length?

No. The original intent was to simplify well-known networks selection.

For now we have two well-known networks, predefined in SDKs: public and testnet. In the future there will be more of them. Maybe, kin or facebook. For developers it will be easier to use network=testnet instead of network_passphrase=Test SDF Network ; September 2015. And again, a signer app can show fancy "Network: Testnet" message in the UI instead of "Network: Test SDF Network ; September 2015".

For all private networks the logic remains the same as it was with network_passphrase: &network=My_Custom_Network_Passphrase.

[MisterTicot]

Response to points

1. As we specified it, the horizon parameters is a fallback URL in case wallet doesn't knows any. We agreed that in normal case wallet should handle the mapping, however there are cases where this will be nice to have, like for testing purpose, private networks and application based on custom network. Its scope is well-defined and it costs nearly nothing to implement. Example use case: if I make an application running on a private network (let's say I make a blogging platform and don't want to burden main net), I can get immediate compatibility with all SEP wallets without requiring any configuration on user side. This matches the goal of the SEP as linking services to wallets.

2. Yes it's about length. The more there's data, the more QRcodes get difficult to scan. Therefore I advice to use network and to allow aliases like network=test, leaving the door open for the addition of new aliases in the future. This is costless implementation-wise and bring benefits

Proposed updates

I'd like to discuss that a bit from implementation perspective. I both like and hate the current pay operation: on one side embedding donation in a simple link is absolutely great and this all makes sense, on the other side it's a pain to implement. My Javascript library support both node and browser environment. Handling any Stellar operation is straightforward, but implementing client inputs over both environments is a real challenge. I have to accept inputs from web pages, from console and possibly from anything else. So, while I'm willing to support it, this little feature may easily end up doubling the work around implementing this SEP.

Now I wouldn't even dare going on implementing something like that for every possible transaction field. I really think this kind of stuff should be restricted to situations where this is really meaningful, rather than fully abstracted. Sticking to UNIX philosophy, I would rather leave this kind of transaction edition to services external to the wallet.

For source/seq/signatures I did:

&stripSources, &stripSequence, &stripSignatures

Each one implying the later, keeps things short & easy.

[orbitlens]

I like the concept of the replace parameter, but things like tx.operations[0].body.paymentOp.amount will be hard to implement. As a signer app developer, I need to write a parser that will resolve a specified path and determine a friendly name (messages like "Amount in operation 4" won't look convincing to end-users), data type (string, number, complex type, say, assets), security restrictions (some operations properties can't be modified, like type). Then create appropriate dynamic fields for user to fill, validate them. It's too complex.

It's better to leave such things to the app that generates a link.

[MisterTicot]

@nikhilsaraf @orbitlens

I'm implementing the use of a "neutral account" with sequence 0 in cosmic-lib to generate XDR when no source is specified. I'm using GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF as it's what I get on Stellar labs for a all zeros account entry.

[theaeolianmachine]

@nikhilsaraf is there an existing issues, or would you like me to create another issue that has the content of https://github.com/stellar/stellar-protocol/issues/157#issuecomment-419309150?

Otherwise, @orbitlens, could you summarize what needs to be done to SEP-0007 here? If you feel it can be expressed more explicitly through a PR updating SEP-0007, I can push that further along as well.

[github-actions[bot]]

This issue is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed, or a comment is posted.