feat: add passkey authenticator to sui #18126
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
joyqvq
requested review from
mystenmark
and removed request for
patrickkuo and
mlegner
| })?; | ||
| if author | ||
| != SuiAddress::from( | ||
| &PublicKey::try_from_bytes( |
There was a problem hiding this comment.
do we want it to be derived exactly like any r1 account? what are the pros and cons of not having it as a different account?
|
addressed comments. todo: add protocol config for devnet only(when review is stable to avoid merge conflicts) open q for meeting:
|
crates/sui-types/src/transaction.rs
Outdated
| @@ -2465,7 +2465,8 @@ impl VersionedProtocolMessage for SenderSignedData { | |||
| } | |||
| GenericSignature::Signature(_) | |||
| | GenericSignature::MultiSigLegacy(_) | |||
| | GenericSignature::ZkLoginAuthenticator(_) => (), | |||
| | GenericSignature::ZkLoginAuthenticator(_) | |||
| | GenericSignature::PasskeyAuthenticator(_) => (), | |||
There was a problem hiding this comment.
we need to check for a protocol flag here, as the comments say (btw can you remove the comment about the code doing nothing, since checks have been added since that comment was written)
There was a problem hiding this comment.
fn check_user_signature_protocol_compatibility(&self, config: &ProtocolConfig) -> SuiResult {
if !config.zklogin_auth() && self.has_zklogin_sig() {
return Err(SuiError::UnsupportedFeatureError {
error: "zklogin is not enabled on this network".to_string(),
});
}
looks like we check this at a different place too, maybe we should keep them all at the same place?
There was a problem hiding this comment.
Seems like we should be calling check_user_signature_protocol_compatibility inside here, and then call this function from validity_check()?
I have meant to continue working on the validity check refactoring but got distracted, will continue looking as we ll
There was a problem hiding this comment.
btw I am refactoring this part in #18342
|
|
||
| /// Normalized r1 signature returned by passkey. | ||
| /// Initialized from `user_signature` in `RawPasskeyAuthenticator`. | ||
| #[serde(skip)] |
There was a problem hiding this comment.
Do we still need #[serde(skip)]? Does JsonSchema use it?
There was a problem hiding this comment.
yeah JsonSchema recognizes serde attrs, but that's a good question on if we do need to include it or not
There was a problem hiding this comment.
Secp256r1Signature, Secp256r1PublicKey doesn't implement jsonschema - i can try adding them to fastcrypto, but is it necessary?
There was a problem hiding this comment.
this is a dumb question, how is this open json rpc generated spec being used?
There was a problem hiding this comment.
honestly, for crypto stuff i think it can be ignored for now since afaik we just serialize crypto sigs to a base64 encoding in jsonrpc. So this is all ignored I believe.
There was a problem hiding this comment.
i see, ill leave it as is then.
| "description": "`clientDataJSON` contains a JSON-compatible UTF-8 encoded string of the client data which is passed to the authenticator by the client during the authentication request (see [CollectedClientData](https://www.w3.org/TR/webauthn-2/#dictdef-collectedclientdata))", | ||
| "type": "string" | ||
| }, | ||
| "digest": { |
There was a problem hiding this comment.
is this needed?
what about the signature?
There was a problem hiding this comment.
Secp256r1Signature, Secp256r1PublicKey does not currently derive JsonSchema, i will have to write a custom one. but since the generated format is not needed, we can ignore these fields.
crates/sui-types/src/transaction.rs
Outdated
| @@ -2378,6 +2380,11 @@ impl SenderSignedData { | |||
| error: "zklogin is not enabled on this network".to_string(), | |||
| }); | |||
| } | |||
| if !config.passkey_auth() && self.has_passkey_sig() { | |||
There was a problem hiding this comment.
btw I have refactored this function, you may have to rebase. The checks are now done with the matches
|
just went through a few more rebase - no major changes other than sequencing behind a bunch of other feature flags in protocol config. PTAL @benr-ml @bmwill @mystenmark @lxfind |
Description
implementation for sui-foundation/sips#9
Test plan
added unit tests and e2e tests. also tested with real passkey result from frontend
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.