Conversation
hocman2
force-pushed
the
feat/webauthn
branch
from
8a2a503 to
dbf79f4
Compare
api/src/application/http/trident/handlers/webauthn_public_key_authenticate.rs
Show resolved
Hide resolved
| use super::api_success::ApiSuccess; | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum Response<T: Serialize + PartialEq> { |
There was a problem hiding this comment.
Why remove PartialEq derive ?
There was a problem hiding this comment.
So I initially PRed deriving PartialEq on some webauthn_rs types to prevent this from happening but after a long debate with the owner this ended up being refused.
My only two solutions were to either implement manually PartialEq on the response types, which would have been a boilerplate non-sense; or to remove the PartialEq requirement on Response which does not appear to be needed, but I understand why it could exist (and I forgot to remove the derive traits on Response itself)
My thought process was that in the future we could be returning responses of deeply nested types that don't necessarily implement PartialEq and this eases up the process a bit.
We can reverse this if you want but that would require to reimplement a lot (almost all actually) of WebAuthn spec types on our end and just do a 1:1 conversion between ours and the webauthn_rs crate's.
This does have the advantage of letting us control which trait we derive or not but it could also backfire into a maintenance nightmare on crate update.
There was a problem hiding this comment.
And I do agree that PartialEq is such a primitive operation that it should be derived whenever possible
There was a problem hiding this comment.
Okay no problem, if this operation don't make problem, then...
There was a problem hiding this comment.
It seems harmless, even when running tests
api/src/application/http/trident/handlers/webauthn_public_key_create_options.rs
Show resolved
Hide resolved
|
Really great work Maybe after fix pre-commit , we can merge it |
|
Sorry I have the bad habit of pushing immediately after commit Edit: pre-commit failed anyway 😢 |
|
That was messy but it should be good |
|
It's good for me , but for merged that it's necessary to verify your commit with a verified signatures |
|
Should I sign every commit in the branch ? |
|
Hello , sorry for the time, I'll be at the Alasca Summit for the next few days. Maybe you can try for 1 commit, else you can rebase all commits and signed it. |
|
Ok i've setup signing as well as complying with the latest credentialtype changes. Tell me if it's enough or not |
|
Normally, it's good ! |
|
To merge, all commits must be signed. You can do a rebase to achieve this. |
|
Alright I'll do that either this week end or next week |
It was required to conditionally derive ToSchema for the various WebAuthn types
|
Hi @hocman2 you can rebase your branch ? |
Yes, had unexpected events this week end so I could allocate less time than I thought to the task. |
Okay no problem ! |
hocman2
force-pushed
the
feat/webauthn
branch
from
6e14f5f to
94f1e67
Compare
|
Ok should be good now, the bug actually highlighted syntax errors in the migrations I had written so it's win win |
Okay, so it's good for merge ? |
yes |
|
merged ! 🚀 |
3 participants
Draft note:
This is a proposal for issue #314
The current implementation allows for creation and authentication of webauthn credentials through four new routes.
There are a lot of file changes but only a handful of files are interesting:
Codebase change
This feature required to add columns to the auth_sessions and credentials tables.
More importantly, I had to switch CredentialData from a struct to an enum because webauthn credentials required a completely different data model.
I updated all places where credential_data was used, which required some function signature changes.
Namely, the HasherRepository interface now requires two explicit parameters for hash_iterations and algorithm, this isn't all so bad as the hasher's interface doesn't need to be aware of anything going on with Credentials anymore.
This approach isn't ideal though. For one it makes using credential_data more tedious than it used to be as you need to explicitly reject any unexpected credential type (generally through an ISE) and two it only patches a broader issue being that Credential should probably be an enum type altogether.
I've also rebased against the latest main
Implementation
The feature uses webauthn-rs under-the-hood for all spec compliance.
Testing
I've run a few successful normal auth flows (registering and authenticating multiple times on the same account)
The server correctly consumes any issued challenge with both successful and failed attempts, preventing replay attacks.