Skip to content

Add Support for User Verification (UV) and BioEnrollment #718

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bradjc-veridian
Copy link

This PR includes our changes to OpenSK to support UV and bioenrollment. Because there is no fingerprint sensor on the nRF52840dk or in Tock, the environment implementation is just a stub.

We are happy to test this PR against our internal hardware platform as updates are made to this PR after feedback.

  • Local tests pass (running run_desktop_tests.sh)
  • Tested against boards
    • Nordic nRF52840 DK
    • Nordic nRF52840 Dongle (JTAG programmed)
    • Nordic nRF52840 Dongle (DFU programmed)
    • Makerdiary nRF52840 MDK USB Dongle
  • Appropriate changes to README are included in PR

Copy link

google-cla bot commented Feb 28, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've just reviewed src/api/persist{,/keys}.rs.

Comment on lines +145 to +150

/// Stored counter UV retries.
UV_RETRIES = 2048;

/// Stored friendly names for enrolled fingerprints.
FRIENDLY_NAMES = 2100..2105;
Copy link
Member

Choose a reason for hiding this comment

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

Those should be between 2005 and 2037, or between 21 and 999, as per the comment line 60 reserving keys 2048 and above for migration purposes.

Note that cargo test --features=std keys_are_disjoint fails otherwise.

@@ -386,6 +408,23 @@ pub trait Persist {
}
}

/// Store a Bio Enrollment friendly name for a given template_id.
fn store_friendly_name(&mut self, template_id: u8, friendly_name: &str) -> CtapResult<()> {
let key = keys::FRIENDLY_NAMES.start + template_id as usize;
Copy link
Member

Choose a reason for hiding this comment

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

We should check key < keys::FRIENDLY_NAMES.end.


/// Retrieve the Bio Enrollment friendly name for a given template_id.
fn get_friendly_name(&mut self, template_id: u8) -> CtapResult<String> {
let key = keys::FRIENDLY_NAMES.start + template_id as usize;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.


match self.find(key)? {
None => Err(Ctap2StatusCode::CTAP1_ERR_OTHER),
Some(value) => Ok(String::from_utf8(value).unwrap_or(String::from(""))),
Copy link
Member

Choose a reason for hiding this comment

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

We should return an internal error if the value is not UTF-8.

@ia0 ia0 requested a review from kaczmarczyck March 1, 2025 08:00
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