feat: add passkey authenticator to sui #18126
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped 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
| use fastcrypto::traits::VerifyingKey; | ||
| use fastcrypto::{error::FastCryptoError, traits::ToFromBytes}; | ||
| use once_cell::sync::OnceCell; | ||
| use passkey::types::webauthn::{ClientDataType, CollectedClientData}; |
There was a problem hiding this comment.
Question: Given all we're using the passkey library for is these data types could we either:
- only depend on the passkey_types crate and not the whole passkey top level crate
- or can we just reimplement these types here so we don't need to depend on them at all?
I think 1 is probably the safest option and we could start there.
There was a problem hiding this comment.
revised, only passkey-types is included in deps. passkey-authenticator and passkey-client are included for dev-deps for e2e tests and unit tests
| /// Compute the hash(tx_data) from the intent msg. | ||
| pub fn to_tx_digest<T: Serialize>(intent_msg: &IntentMessage<T>) -> [u8; DefaultHash::OUTPUT_SIZE] { | ||
| let mut hasher = DefaultHash::default(); | ||
| bcs::serialize_into(&mut hasher, &intent_msg.value) | ||
| .expect("Message serialization should not fail"); | ||
| hasher.finalize().digest | ||
| } |
There was a problem hiding this comment.
So this is actually incorrect as its not properly taking into account the salt "TransactionData::" that is required for actually computing the transaction digest. this means the output of this function and Transaction::digest will differ.
There was a problem hiding this comment.
good point, revised to signing digest
| /// Compute the hash(tx_data) from the intent msg. | ||
| pub fn to_signing_digest<T: Serialize>( | ||
| intent_msg: &IntentMessage<T>, | ||
| ) -> [u8; DefaultHash::OUTPUT_SIZE] { | ||
| let mut hasher = DefaultHash::default(); | ||
| bcs::serialize_into(&mut hasher, &intent_msg.value) | ||
| .expect("Message serialization should not fail"); | ||
| hasher.finalize().digest | ||
| } |
There was a problem hiding this comment.
I thought we agreed on this being the Transaction Digest? I don't see the value in encoding the challenge with (Intent | non tx digest) over hash(Intent | non tx digest) in this case. The value that was being provided with this sending (Intent | digest) was that we could match the digest to the transaction digest that is well known.
There was a problem hiding this comment.
i misunderstood before - since it will end up diverging with the original signing digest i thought we were ok with signing_digest not tx_digest.
while i can change to tx_digest, this introduces a non trivial change to the code that involves an additional trait everywhere that we wanted to avoid before
T: Serialize + Signable<DefaultHash>
instead of
T: Serialize
for all authenticator trait implementers. this makes a lot of places hard to implement such as PersonalMessage since this can trait can only be implemented as a private trait.
is this desirable? is there an alternative way to avoid it?
There was a problem hiding this comment.
PersonalMessage also uses this trait but its not implemented inside the private trait module, it only implements Serialize
There was a problem hiding this comment.
After discussing it seems like we should leave this as-is given deviating from this would be thrashing against the status quo (which we've identified is not great but not something we can really worry about addressing right now)
## Description implementation for sui-foundation/sips#9 ## Test plan added unit tests and e2e tests. also tested with real passkey result from frontend ``` credential id (base64): 9Aj01fIn/T555beIoJ7swA87mLc= pubkey (hex): 03e61ebc6b1796021e33fd3937298f2c460e77e5bb7fbeb3c42f7e0f11f67792cb sui address: 0xb88d3e91880e6befc13881a4a7d5b4d2dfa402ae2b149cd9b36d6e084ba25925 tx bytes: AAAAALiNPpGIDmvvwTiBpKfVtNLfpAKuKxSc2bNtbghLolklAfuV/HGZhQdRfe9WXfY8A7b76qucni1/FFaa7LlYHgcGAgAAAAAAAAAgUuv6miRJNCj1OhyRzP5sZyrIU1DSRTLrXfvbmp9cuuu4jT6RiA5r78E4gaSn1bTS36QCrisUnNmzbW4IS6JZJegDAAAAAAAAgIQeAAAAAAAA tx digest (hex): 000000f1b6d366d79ae9d2d98da7909b8fa4d856f64c4b9663a1d875d7823f5e8585e4 authenticatorData (hex): 49960de5880e8c687434170f6476605b8fe4aeb9a28632c7995cf3ba831d97631d00000000 clientDataJSON: `{"type":"webauthn.get","challenge":"AAAA8bbTZtea6dLZjaeQm4-k2Fb2TEuWY6HYddeCP16FheQ","origin":"http://localhost:5173","crossOrigin":false}` r1 signature (hex): 02ecbbf52b29ec5d306501d00f175bd084d909a4f9a92318cf3df1fa3a86028cbe2dcc606a99f69817991bab559b744039c21b417c988056969b8fec78d17bad7c03e61ebc6b1796021e33fd3937298f2c460e77e5bb7fbeb3c42f7e0f11f67792cb encoded sui signature (base64): BiVJlg3liA6MaHQ0Fw9kdmBbj+SuuaKGMseZXPO6gx2XYx0AAAAAigF7InR5cGUiOiJ3ZWJhdXRobi5nZXQiLCJjaGFsbGVuZ2UiOiJBQUFBOGJiVFp0ZWE2ZExaamFlUW00LWsyRmIyVEV1V1k2SFlkZGVDUDE2RmhlUSIsIm9yaWdpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6NTE3MyIsImNyb3NzT3JpZ2luIjpmYWxzZX1iAuy79Ssp7F0wZQHQDxdb0ITZCaT5qSMYzz3x+jqGAoy+Lcxgapn2mBeZG6tVm3RAOcIbQXyYgFaWm4/seNF7rXwD5h68axeWAh4z/Tk3KY8sRg535bt/vrPEL34PEfZ3kss= encoded webuahthn signature length: 278 signature verified: true localnet onchain verified, digest: DSt3BdgByH5WgKuysdwPMLrCtH2HZhvtzGNBJarBA7VR ``` --- ## 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. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK:
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.