Review public API about webauthn-rs HOT 11 CLOSED

That does seem much simpler! 😅 I'll put together a PR for this soon then, and I'll tag you to review if that's okay. Thanks for your input.

from webauthn-rs.

cc @adrienperonnet since you have been looking at this library recently.

from webauthn-rs.

@Firstyear Thanks for creating this project! Great work and overall it was very easy to use.

Re: ergonomics the one feedback I have is that it might be a bit too opinionated with the WebAuthnConfig trait. I had to fight the abstraction a bit by creating a struct that wraps a mutable reference of the main struct that implements the trait. For example, I needed to pull out some data after registration/auth was performed (like which credential it was that succeeded). In some cases you might store challenges in a session cookie or in other cases a database.

What I imagine could be a less opinionated API is something like ~5 functions:
-generate_challenge
-register_request
-complete_register
-authenticate_request
-complete_authenticate

(Naming could be more symmetric to the actual webauthn spec, but you get the idea.)

from webauthn-rs.

I can see where you are coming from. You want the api to simply do the cryptographic operations given some json, you get "an output".

I think the issue with this is that on each operation then, you would need origin, relying party name, party id, valid algos on each request.

So you would probably end up with something where you have to give a "webauthnconfig" on each function call instead, where as the current model tries to have the "policy" defined once and then reusable in a thread safe/wrapped manner.

Don't think I'm saying no to the suggestion - but I'd like to get the idea a bit better defined before I commit to it :)

Some example cases is that on generate_challenge, we need to know:

• the credentials associated to a username
• if verified tokens are requested or not
• the timeout window
• the current relying party id
• a way to store the challenge

So I'd imagine for your example the function might change to say;

fn generate_challenge_authenticate(&config: WebauthnConfig, username: UserId, credentials: &[Credential]) -> WebauthnChallenge

This would make it such that:

• The config between calls to the function could change (good or bad, depends on your view). We still have a config too that has to do quite a bit of heavy lifting ...
• you have to externally provide the usercredentials
• you have to persist the challenge associated to a session/transaction.

Part of the idea behind the webauthnconfig trait today is it "forces" you to implement something that can correctly handle the workflow of webauthn, but the issue with the "bag of bits" you suggest is it requires people to "read the spec" and implement the external code "just right" to ensure certain states are upheld.

A possible middle ground is to retain the current webauthnconfig design, but remove the "persistance" functions and have those returned by the calls. These would be "persist_credential, retrieve_credential, credential_update_counter, credential_report_invalid_counter". All other parts of the webauthnconfig would stay the same, but we rely more on the user for some of the persistance parts.

Does that seem like it could be an improvement?

from webauthn-rs.

fn generate_challenge_authenticate(&config: WebauthnConfig, username: UserId, credentials: &[Credential]) -> WebauthnChallenge

Yep exactly what I was thinking!

Part of the idea behind the webauthnconfig trait today is it "forces" you to implement something that can correctly handle the workflow of webauthn, but the issue with the "bag of bits" you suggest is it requires people to "read the spec" and implement the external code "just right" to ensure certain states are upheld.

I think we can enforce the correct workflow just using the type system. It just would leave out where you should store things like challenges and credentials.

A possible middle ground is to retain the current webauthnconfig design, but remove the "persistance" functions and have those returned by the calls. These would be "persist_credential, retrieve_credential, credential_update_counter, credential_report_invalid_counter". All other parts of the webauthnconfig would stay the same, but we rely more on the user for some of the persistance parts.

Does that seem like it could be an improvement?

What do you mean by "returned by the calls"? Would the persistence functions take a closure as a param?

from webauthn-rs.

What were you thinking for enforcing the workflow? Maybe something like:

struct WebauthnChallenge {
    config: &WebauthnConfig,
    challenge: Vec<u8>,
    username: userId,
    credentials: Vec<Credential>,
}

impl WebauthnChallenge {
    into_json(&self) -> ..... {
    }
}

fn generate_challenge_authenticate(&config: WebauthnConfig, username: UserId, credentials: &[Credential]) -> WebauthnChallenge {
}

fn authenticate_credential(challenge: WebauthnChallenge, clientResponseJson: ...) -> Result<.....> {
    // check the clientResponseJson is associated to the challenge issued?
}

Maybe something like this would work. You guarantee that you need to have issued a challenge before you can do the authenticate. Another option is:

impl WebauthnChallenge {
    into_json(&self) -> ..... {
    }

    fn authenticate(clientResponseJson: ....) -> .... {
    } 
}

Something like that?

What do you think? did you have another idea in mind about how the type system could be used to guarantee the correct flows?

from webauthn-rs.

You could have a WebAuthn struct that takes a config struct on init.

Then you could do something like:

impl WebAuthn {
  pub fn generate_challenge(&self) -> Challenge { .. }
  pub fn register_request(&self, challenge: Challenge, user_id: UserId) -> RegisterRequest {..}
  pub fn register(&self, challenge: Challenge, response: RegisterResponse) -> Result<Credential, Error> { .. }
}

where Challenge, RegisterRequest can be serialized to JSON and RegisterResponse can be deserialized from JSON.

from webauthn-rs.

Hey there, I've just merged a bunch of changes to master to improve this. Would be great if you could review and let me know what you think of the changes @agrinman . :)

from webauthn-rs.

@Firstyear awesome, looks good to me. Just added a couple of comments here: https://github.com/Firstyear/webauthn-rs/pull/21#pullrequestreview-355642202

from webauthn-rs.

Thank you! I'll certainly address those comments. I want to try it out a bit more before I release it as a new version. If you could give it a go in a server or a project, and let me know if you hit any challenges on the way, that would help too :)

from webauthn-rs.

Going to close this as I think we are getting a lot of improvements generally from the community and users.

from webauthn-rs.