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

Add basic setup for multi-PIN #530

Merged
merged 14 commits into from
Aug 23, 2022
Merged

Conversation

hcyang-google
Copy link
Collaborator

  • Reserve the storage keys for maximum of 8 user slots.
  • Modify the storage functions to take a slot_id parameter.
  • Add the slot_count() customization.
  • Assume slot_id as a parameter when needed except these places:
    • Entrance functions of command processing that directly takes the
      command parameter structure. slot_id is set as 0, and will be
      parsed from the parameters when we enable the feature.
    • MakeCredential/GetAssertion/AuthenticatorConfig will take the
      slot_id from active token state when we enable the feature,
      resulting in an Option<usize>. Below code will act on the option
      value correctly. When the feature isn't enabled, we're always
      referring to the only PIN slot so set slot_id as Some(0).
    • GetInfo returns verdict of whether PIN is supported and enabled, and
      whether PIN needs to be forced changed. There will be new fields to
      represent those values when the feature is enabled, and the old
      fields will not be populated. So when the feature isn't enabled, we
      can treat slot_id as 0.

Not covered in this commit:

  • Unittests for other slots. The existing tests all pass and I plan to
    add unittests for multi-slot case after the codebase allows enabling
    the feature.
  • Persisting and checking the slot_id in credentials. This is planned to
    come in the next commit.

- Reserve the storage keys for maximum of 8 user slots.
- Modify the storage functions to take a slot_id parameter.
- Add the slot_count() customization.
- Assume slot_id as a parameter when needed except these places:
  - Entrance functions of command processing that directly takes the
    command parameter structure. slot_id is set as 0, and will be
    parsed from the parameters when we enable the feature.
  - MakeCredential/GetAssertion/AuthenticatorConfig will take the
    slot_id from active token state when we enable the feature,
    resulting in an `Option<usize>`. Below code will act on the option
    value correctly. When the feature isn't enabled, we're always
    referring to the only PIN slot so set slot_id as Some(0).
  - GetInfo returns verdict of whether PIN is supported and enabled, and
    whether PIN needs to be forced changed. There will be new fields to
    represent those values when the feature is enabled, and the old
    fields will not be populated. So when the feature isn't enabled, we
    can treat slot_id as 0.

Not covered in this commit:
- Unittests for other slots. The existing tests all pass and I plan to
  add unittests for multi-slot case after the codebase allows enabling
  the feature.
- Persisting and checking the slot_id in credentials. This is planned to
  come in the next commit.
src/ctap/client_pin.rs Outdated Show resolved Hide resolved
src/ctap/storage.rs Show resolved Hide resolved
src/ctap/storage/key.rs Outdated Show resolved Hide resolved
src/env/test/customization.rs Show resolved Hide resolved
src/ctap/mod.rs Show resolved Hide resolved
src/ctap/mod.rs Show resolved Hide resolved
src/ctap/storage.rs Outdated Show resolved Hide resolved
src/ctap/storage.rs Outdated Show resolved Hide resolved
kaczmarczyck and others added 7 commits August 22, 2022 15:53
* maximum working bumpalo version

* explicit comment to explain version locking

* removes incorrect comment

* moves serde version lock to dev dependencies

* removes serde dependencies

* reverts serde removal in crypto library
Add support for concatenated values
Pull in the concatenated value storage support.
src/ctap/storage.rs Outdated Show resolved Hide resolved
@kaczmarczyck kaczmarczyck self-requested a review August 23, 2022 09:20
Copy link
Collaborator

@kaczmarczyck kaczmarczyck left a comment

Choose a reason for hiding this comment

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

No strong opinion on my last comment, LGTM.

@hcyang-google hcyang-google merged commit 078e565 into google:multi-pin Aug 23, 2022
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.

None yet

3 participants