Skip to content

Conversation

@msirringhaus
Copy link
Contributor

  1. Fix typo
  2. Allow deserializing PublicKeyCredentialDescriptor from JSON and CBOR, instead of only CBOR.
  3. Automatically clear cached PUAT and try getting a new one (e.g.: when doing a few BioEnrollment-operations and then do CredentialManagement-operations).

$cached_puat = false;
$skip_puap = false;
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would that happen? We should be storing permissions with the cached PinUvAuthToken and checking them prior to sending any commands. I suppose we might still need this code to handle PUATs that have expired, so maybe only the example in the comment needs to be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually used it as the comment described. But we should indeed check the permissions before using it.

On a general note: I wanted to have this caching-mechanism as lenient as possible, to never cause non-recoverable errors. If the cached PUAT is bad for whatever reason, auth-rs should just skip it and try again to get a new one.
Hence the change in handle_errors!(). And yes, we also need this for expired ones. But also for "bad" ones, since they are externally provided (all zeros for example, or just a random byte-array).

Copy link
Collaborator

@jschanck jschanck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just make the one change I mention below.

src/ctap2/mod.rs Outdated
| Some(PinUvAuthResult::SuccessGetPinUvAuthTokenUsingUvWithPermissions(t))
| Some(PinUvAuthResult::SuccessGetPinUvAuthTokenUsingPinWithPermissions(t)) => {
| Some(PinUvAuthResult::SuccessGetPinUvAuthTokenUsingPinWithPermissions(t))
if t.permissions == PinUvAuthTokenPermission::BioEnrollment =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to check membership rather than equality. Looks like the version of bitflags that we're using has a contains function.

(aside: I'm a little concerned that more recent versions of bitflags are quite different. Long term we should remove that dependency. It's not really doing much for us.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.
We also need to think about adding a rust-toolchain.toml-file to the repo, to avoid clippy randomly breaking the CI.
From what I can tell, this time it was a clippy-bug and not a real problem.

@jschanck
Copy link
Collaborator

jschanck commented Sep 7, 2023

Thanks!

@jschanck jschanck merged commit d5477d6 into mozilla:ctap2-2021 Sep 7, 2023
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.

2 participants