SEP-10 Multisig Support #107

JakeUrban · 2020-01-24T18:20:12Z

Refactors the SEP-10 authentication code and adds support for multi-signature accounts according to the changes in stellar-protocol, utilizing the stellar_sdk changes released in 2.1.1-beta1.

2.1.1 has been released today, so I'll update this PR to use that.

codecov-io · 2020-01-24T18:28:37Z

Codecov Report

Merging #107 into master will decrease coverage by 0.89%.
The diff coverage is 77.41%.

@@            Coverage Diff            @@
##           master     #107     +/-   ##
=========================================
- Coverage   87.44%   86.55%   -0.9%     
=========================================
  Files          25       25             
  Lines        1171     1190     +19     
=========================================
+ Hits         1024     1030      +6     
- Misses        147      160     +13

Impacted Files	Coverage Δ
polaris/polaris/helpers.py	`86.54% <100%> (+0.07%)`	⬆️
...is/polaris/management/commands/check_trustlines.py	`68.29% <100%> (ø)`	⬆️
polaris/polaris/sep10auth/views.py	`79.61% <76.13%> (-5.1%)`	⬇️
...aris/management/commands/create_stellar_deposit.py	`55.55% <0%> (-27.78%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecb99f1...c7a8647. Read the comment docs.

JakeUrban · 2020-01-24T18:51:44Z

@overcat I'd like your thoughts on how this should be implemented in the Python SDK. I left code comments describing my ideas.

leighmcculloch

@JakeUrban I took a look at the PR. I'm not fluent in Python, so please apologies if I've misinterpreted the code, please correct me if I've done so.

I noted inline some concerns ❗️ and ideas 💡.

I really like 👏 how you're proposing to change the Python SDK, I think that will have the least impact on existing users and tells a clear story for how to use the function with the changes.

polaris/polaris/sep10auth/views.py

overcat · 2020-01-25T02:56:37Z

Thanks for your suggestions, I will release a pre-release version in these two days.

overcat · 2020-01-27T04:42:23Z

Hi @JakeUrban, I just released a pre-release version, please let me know if you have any questions.

leighmcculloch

I took a look and this looks good bar a couple comments that are inline below.

I haven't finished looking through the Python SDK challenge transaction implementation that is in 2.1.1, I'll post here when I've done that too.

leighmcculloch · 2020-01-31T17:30:45Z

polaris/polaris/sep10auth/views.py

    """
-    Validate the provided TransactionEnvelope XDR (base64 string). Return the
-    appropriate error if it fails, else the empty string.
+    `GET /auth` can be used to get an invalid challenge Stellar transaction.


invalid challenge Stellar transaction

When you say that the challenge transaction is invalid, do you mean to say that it hasn't been signed by the client? If that's the case I'd avoid the concept of invalid here because from the clients perspective who will be reading the challenge transaction before they sign it it is valid, it is just not yet signed by the client.

leighmcculloch · 2020-01-31T18:16:20Z

polaris/polaris/sep10auth/views.py

+            server_secret=settings.SIGNING_SEED,
+            client_account_id=client_account,
+            anchor_name=ANCHOR_NAME,
+            network_passphrase=settings.STELLAR_NETWORK_PASSPHRASE,


The Python SDK still has a default timeout of 300ms rather than 900ms as in v1.3.0. I think we should set it explicitly here though so we're making a conscious decision that's what we want the timeout to be. Could we add that?

polaris/polaris/sep10auth/views.py

leighmcculloch · 2020-01-31T18:29:11Z

polaris/polaris/sep10auth/views.py

+        )
+        hash_hex = binascii.hexlify(transaction_envelope.hash()).decode()
+        jwt_dict = {
+            "iss": request.build_absolute_uri("/auth"),


I think this line allows injection into the JWT. If an anchor hosts their server in anyway that allows arbitrary hostnames in the requests, e.g. if they host it on a static IP without hostname filtering which is possible, the iss field can be made to contain any hostname a caller wants.

I'd lean towards this being the server signing key address, e.g. https://github.com/stellar/go/blob/4268b290621430b89c3ed687e0d83df3800c227a/exp/services/webauth/internal/serve/token.go#L87, if we don't have a good way to get some other definitive exact URI for the issuer.

I'm not convinced that using the server signing key is any better than the current value for iss; both can be faked. The only real protection against someone attempting to side-step SEP-10 by using a fake JWT (a JWT not created by the anchor server) is the server's secret used to encode the data. The SEP also states that iss should be a URI. What do you think?

Ah you're right, the SEP does say to use a URI. I was thinking about the RFC7519 Section 4.1.1 where technically it doesn't have to be a URI, but I forgot we specify that in the SEP. I'll change the webauth implementation to make it a URI too.

Can we get the hostname from the settings or somewhere else instead of the incoming request? I still think we should avoid allowing any data from the request making its way into the JWT.

FYI I updated the Go implementation here to align with ☝️, stellar/go#2214.

Yea, I can add an AUTH_SERVER setting since the SEP-10 authentication could be hosted on a different URI than the anchor server.

Actually, if they're using Polaris there isn't a reason you'd want to have a separate server handling authentication. I'll use a HOST_URL instead.

leighmcculloch · 2020-02-01T01:50:40Z

I haven't finished looking through the Python SDK challenge transaction implementation that is in 2.1.1, I'll post here when I've done that too.

FYI I had a look through the Python SDK challenge transaction implementation that is in 2.1.1 and I posted some recommendations on that repositories issue about. It would be ideal if the changes to how signatures and signers are matched could be made since we're using this in Polaris.
StellarCN/py-stellar-base#252 (comment)

JakeUrban requested review from leighmcculloch and msfeldstein January 24, 2020 18:50

JakeUrban mentioned this pull request Jan 24, 2020

Support SEP10 Multisig #60

Closed

leighmcculloch reviewed Jan 24, 2020

View reviewed changes

JakeUrban force-pushed the sep10-multisig branch from 3562c90 to 982bf32 Compare January 29, 2020 21:55

leighmcculloch reviewed Jan 31, 2020

View reviewed changes

JakeUrban added 11 commits January 31, 2020 12:59

So far

74d22c6

small changes

3c48873

Added catch for stellar_sdk exception

0f5a7df

Rename function

a1272aa

Added thoughts on implementation in stellar_sdk

74460a0

Changes in response to PR comments

aba4651

Use stellar-sdk 2.1.1-beta1 for SEP-10 multisig support

2bc9c4e

Refactored SEP10Auth into APIView, fixed urldecoding

893d05a

Small adjustments

ce9bb6d

Use stellar_sdk 2.1.1, not beta release

3f9ebe6

Added HOST_URL setting

c7a8647

JakeUrban force-pushed the sep10-multisig branch from 001f31c to c7a8647 Compare January 31, 2020 21:03

JakeUrban added 2 commits January 31, 2020 13:05

Add 900ms timeout

dcf740e

Remove 'invalid' language to describe unsigned challenge transaction

17408a5

JakeUrban merged commit f03a100 into stellar:master Jan 31, 2020

JakeUrban deleted the sep10-multisig branch January 31, 2020 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SEP-10 Multisig Support #107

SEP-10 Multisig Support #107

JakeUrban commented Jan 24, 2020 •

edited

Loading

codecov-io commented Jan 24, 2020 •

edited

Loading

JakeUrban commented Jan 24, 2020 •

edited

Loading

leighmcculloch left a comment

overcat commented Jan 25, 2020

overcat commented Jan 27, 2020 •

edited

Loading

leighmcculloch left a comment

leighmcculloch Jan 31, 2020 •

edited

Loading

leighmcculloch Jan 31, 2020

leighmcculloch Jan 31, 2020

JakeUrban Jan 31, 2020

leighmcculloch Jan 31, 2020

leighmcculloch Jan 31, 2020 •

edited

Loading

JakeUrban Jan 31, 2020

JakeUrban Jan 31, 2020

leighmcculloch commented Feb 1, 2020 •

edited

Loading

SEP-10 Multisig Support #107

SEP-10 Multisig Support #107

Conversation

JakeUrban commented Jan 24, 2020 • edited Loading

codecov-io commented Jan 24, 2020 • edited Loading

Codecov Report

JakeUrban commented Jan 24, 2020 • edited Loading

leighmcculloch left a comment

Choose a reason for hiding this comment

overcat commented Jan 25, 2020

overcat commented Jan 27, 2020 • edited Loading

leighmcculloch left a comment

Choose a reason for hiding this comment

leighmcculloch Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

leighmcculloch Jan 31, 2020

Choose a reason for hiding this comment

leighmcculloch Jan 31, 2020

Choose a reason for hiding this comment

JakeUrban Jan 31, 2020

Choose a reason for hiding this comment

leighmcculloch Jan 31, 2020

Choose a reason for hiding this comment

leighmcculloch Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

JakeUrban Jan 31, 2020

Choose a reason for hiding this comment

JakeUrban Jan 31, 2020

Choose a reason for hiding this comment

leighmcculloch commented Feb 1, 2020 • edited Loading

JakeUrban commented Jan 24, 2020 •

edited

Loading

codecov-io commented Jan 24, 2020 •

edited

Loading

JakeUrban commented Jan 24, 2020 •

edited

Loading

overcat commented Jan 27, 2020 •

edited

Loading

leighmcculloch Jan 31, 2020 •

edited

Loading

leighmcculloch Jan 31, 2020 •

edited

Loading

leighmcculloch commented Feb 1, 2020 •

edited

Loading