Skip to content
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

Adding Eq and PartialEq so I can use biscuit claims for the json #179

Closed
wants to merge 1 commit into from

Conversation

chotchki
Copy link

@chotchki chotchki commented Aug 19, 2022

Added Eq and PartialEq to types to allow use of PasskeyAuthentication and PasskeyRegistration in biscuit JWE claims.

For reference: https://docs.rs/biscuit/latest/biscuit/struct.ClaimsSet.html

  • cargo fmt has been run
  • cargo test has been run and passes
  • [N/A] documentation has been updated with relevant examples (if relevant)

@benwis
Copy link
Collaborator

benwis commented Aug 19, 2022

Out of curiosity, what does Eq and Partial Eq give you with regard to biscuit for these structs?

@Firstyear
Copy link
Member

Looks like they want to stuff the reg and auth state into biscuit tokens rather than storing it server side.

@Firstyear
Copy link
Member

I'm not really sure how I feel about this PR. Yes, it solves your immediate problem, but I think it seems more like an issue in biscuit, because these aren't really items that actually need or should have to support equality. Can you send some sample code so that I can look at?

@Firstyear
Copy link
Member

@chotchki Hey there, do you have some sample code I can look at?

@chotchki
Copy link
Author

@Firstyear The codebase is still in flight / I haven't open sourced it yet so I've pulled out the two relevant parts. So the normal... totally not tested, probably has a bunch of stupid mistakes but I'm hoping it shows my thought process.

Gist is here: https://gist.github.com/chotchki/b70c39214acaea80da7b55c9ca4c17a4

The registration code using webauthn-rs is in authentication.rs and the service that produces the JWT/JWE is in jwe_service.rs.

@Firstyear
Copy link
Member

@chotchki I think having a look at this, despite your 5 minute expiration window you still may be open to short lived replay attacks since the client controls the complete state of the authentication flow in this. So it's "not a great idea" to proceed with this design. These state elements MUST be stored server side.

Additionally, you don't actually need to derive PartialEq here to this library. If you implement PartialEq manually rather than deriving it on your various Claims structures, that satisfies the trait bounds for ClaimSet.

I will document this in the library, so that other people don't fall into the same trap, but sadly it's something we can't easily "prevent" occurring. It may even be worth removing serialise/deserialise on the state types to remove the "illusion" they can be persisted in a token like this.

So I'm sorry, but I'm going to close this, and I have to advise that you redesign your integration to store the authentication and registration state to be server side stored OR that you manually implement partial eq on your ClaimSet types rather than deriving it.

@Firstyear Firstyear closed this Aug 24, 2022
@chotchki
Copy link
Author

I appreciate you taking a look! I tend to avoid sessions since they end up using cookies and you then have to implement additional cross site scripting protections but you're motivating me to better understand the attack surface for webauthn.

@Firstyear
Copy link
Member

No problem at all! Webauthn is certainly "not perfect" and there are ways to still hold it wrong, and the goal of this library is to minimise those. So thanks for reporting the issue :)

@Firstyear
Copy link
Member

PS: Don't feel bad either, it looks like the project was being naughty in some of it's demo code, so now I have to fix that! Opps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants