-
Notifications
You must be signed in to change notification settings - Fork 419
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
Implement EIP-4361 support with SIWS message handling and verification #1918
base: master
Are you sure you want to change the base?
Conversation
internal/utilities/siws/helpers.go
Outdated
// GenerateNonce creates a random 16-byte nonce, returning a hex-encoded string. | ||
func GenerateNonce() (string, error) { | ||
b := make([]byte, 16) | ||
_, err := rand.Read(b) | ||
if err != nil { | ||
return "", err | ||
} | ||
return hex.EncodeToString(b), nil | ||
} |
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.
Move this to crypto
package / re-use something in it?
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.
the siws folder was supposed to be a separate package that could be deployed separately, I just switched it to use nonce := crypto.SecureToken()
would that work?
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.
This is really good! Great direction!
A few general notes:
- We need to think a bit about how the config is encoded. I get that
EIP<numbers>
makes a lot of sense for people deep in this matter, but it's not very descriptive for regular programmers that are self-hosting Supabase Auth. I'd say let's just doGOTRUE_EXTERNAL_WEB3_<chain>
or something. If there's a new EIP later on, we'll think about its config options then. - Similarly
POST /token?grant_type=<this should be readable by regular programmers>
.siws
,web3
,siwe
, things of this sort are better thaneip<numbers>
. This value will also be mapped into auth-js as well. POST /token?grant_type=<web3 type>
should only take in JSON, not URL forms. All APIs currently only use JSON, so this is a weird exception. Is there a particular reason we can't use JSON here?utilities/siws
is a weird name for a package that doeseip<numbers>
. Maybe just move this under/internal/web3
and it will house this and any future additions.
What is the best way to test this? Any simple test app you can host somewhere?
This makes sense, originally i thought of this heirarchy: Web3 > EIP4361 > Sign in with solana /sign in with ethereum, as there's maybe other web3 modes of auth that aren't eip4361 compliant, eth/sol go under eip4361, but what you said makes sense, I think web3 is friendly here.
I'm using JSON, is there something i missed? func (a *API) EIP4361Grant(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
db := a.db.WithContext(ctx)
params := &Web3GrantParams{}
if err := retrieveRequestParams(r, params); err != nil {
return err
}
...
I'll look into it and get back to you. |
I saw this which caught my eye. https://github.com/supabase/auth/pull/1918/files#diff-db4fba83e08f520d74d66c9283e5dcd938e1746de1c4e7c481cce8aff142b5a1R71-R73
No need to go into |
…kage and remove legacy code
…legacy web3 references
- harden siws verification. - remove redundant checks. - add /nonce? endpoint. - create new nonce table (nonce usage without db is incompliant with siws).
Hey guys, I pushed an update a few days ago with some things I missed:
|
- mounted the /nonce endpoint to the router. - switched nonce to random OTP (slashes not allowed in wallet adapters). - adjusted migration file to add the nonce tables.
…rity feat(api): add address field to nonce generation for targeted nonce use refactor(api): replace SignedMessage with Web3GrantParams for clarity feat(api): implement nonce verification in Web3Grant flow for security refactor(api): remove unused logging and clean up code for clarity fix(api): correct nonce storage to include address for targeted validation chore(api): remove Address field from Web3GrantParams as it's parsed from message feat(crypto.go): add base32 encoding and secure alphanumeric generation refactor(crypto.go): replace ethereum and btc libraries with uuid and internal storage for better modularity feat(crypto.go): implement nonce verification and consumption logic for enhanced security feat(migrations): enforce non-null constraint on address in nonces table for data integrity
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.
hey @Bewinxed 👋🏼 nice work! I gave this a first pass and left some comments. The replay detection looks correct, but it would be good to have a test that ensures it stays correct.
…, removed redundant token functions in crypto.go
Thanks for your reviews, I’ve replied to the comments and adjusted the code accordingly. Thanks! |
Hello! Just resolved all issues, please review one more time. as per our discussion I’ve removed serverside nonce processing, in favor of a nonce that applies to all oidc providers, which will be implemented later. |
Before merge remember to change title into conventional commits format. |
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.
OK we're getting very close!
Would you mind updating the openapi.yaml
file with the changes to the API as well!
Also what happened with the nonce verification per the agreement to check it via the identities
table? I don't think I'm able to see it in the code.
case BlockchainSolana: | ||
err = p.verifySolanaSignature(params.Signature, params.Message, parsedMessage) | ||
default: |
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.
Let's remove the other chains in the parse supported chains method above, if those are not being supported.
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.
Do you want me to remove the default condition? I've already removed eth stuff.
case *storage.CommitWithError: | ||
return err | ||
case *HTTPError: | ||
return err | ||
default: | ||
return oauthError("server_error", "Internal Server Error").WithInternalError(err) |
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.
To my knowledge all errors are non-pointer types. Did you go through these errors at some point and made sure this code works?
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 haven't, but HTTPError and CommitWithError have structs, so both of them are pointers, I believe those were defined prior.
The errors seem to work but I didn't add explicit tests.
Co-authored-by: Stojan Dimitrovski <[email protected]>
Co-authored-by: Stojan Dimitrovski <[email protected]>
Resolved all issues, wrt nonce verification, the team has mentioned that they're going to implement nonce verification for oidc based providers and web3 later on, so simple verification is in place for now, the team is going to implement this. |
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.
Seems good enough to go in from my POV. Still need to test E2E though and potential adjustments might come in future PRs.
Would you mind making sure the comments about validation and OpenAPI are addressed before merging?
w := httptest.NewRecorder() | ||
ts.API.handler.ServeHTTP(w, req) | ||
ts.assertErrorResponse(w, http.StatusBadRequest, errorInvalidGrant) | ||
} |
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.
} | |
} | |
switch chain { | ||
case BlockchainSolana: | ||
err = p.verifySolanaSignature(params.Signature, params.Message, parsedMessage) | ||
default: | ||
return nil, httpError(http.StatusNotImplemented, "signature verification not implemented for %s", network) | ||
} |
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.
To me it feels like this validation needs to be done before entering this function. Such as validating on the API handler, not on the verifier.
From that point of view, the default case should panic
instead as a programmer error.
// Decode base64 signature into bytes | ||
sigBytes, err := base64.StdEncoding.DecodeString(string(signature)) | ||
if err != nil { | ||
return fmt.Errorf("invalid signature encoding: %w", err) | ||
} |
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.
Likewise let's push validation of Base58 in the API handler, and panic on the check.
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.
Could you make sure to clean up the diff... I think your editor doesn't like "
and switches to '
but it's making it hard to see what has actually changed.
What kind of change does this PR introduce?
EIP4361 Auth on the backend via SIWS That can easily be extended further for any other EIP4361 compliant sign-in method.
What is the current behavior?
No onchain auth available.
What is the new behavior?
I've added a new grant type ?grant_type=eip4361, which takes the siws message and uses that to create/authenticate users using the existing methods, I've avoided modifying existing structures as much as possible unless necessary.
In the root folder, there is a external_eip4361_siws_example.go
uncomment the last 3 lines and run it using go run and it will provide an example SIWS message you can test.
built it, spun up the server:
Response:
Additional context
To support this, and for it to be easily extendable, I've added this to the .env.example
since siws/siwe use eip4361, i added the grant type eip4361, which can extend eth/sol and any compatible network, which can be specified in the .env
then based on the chosen network e.g solana:mainnet the appropriate validation will be used.
I've implemented a siws package inside internal/utilities/siws that implement many of the necessary siws functions (need to review them to double check the validations).
for solana, I tried a no-dependency validation however i added the btc base58 package
as for ethereum, I'm, using the ethereum-go package, which is widely supported, but perhaps we can omit and try to implement a native validation without the dep.
I haven't worked much with GO specifically, but I'm the author of https://deauth.vercel.app/, which has won 4th place in the Infra track of the Solana Hyperdrive Hackathon before.
Further considerations
Willing to hear your opinions, and revise the conventions. 🙏