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

Adds a high level persistent storage API #679

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

kaczmarczyck
Copy link
Collaborator

@kaczmarczyck kaczmarczyck commented Apr 2, 2024

Changes the store API to a wider API for data in a persistant way. Joins the attestation store into the regular store, and allows more control over how to special case some of the storage.

The KeyStore had some persistent elements that were moved out, and the crypto-related parts were kept separately.

In some cases, we can consider unifying even more, like the way the PIN hash is stored. To keep the PR smaller, it only implements the general API changes, no updates to specific parts of the API when not necessary.

One planned follow-up is LargeBlob (see the TODO comment).

@kaczmarczyck kaczmarczyck requested a review from ia0 April 2, 2024 17:02
@kaczmarczyck kaczmarczyck self-assigned this Apr 2, 2024
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.

I didn't check in details, but for the change to be backward compatible, we need to use the same keys and encode the values the same way. From what I've seen so far it seems like it, but just making sure it's conscious.

libraries/opensk/src/api/persist.rs Show resolved Hide resolved
libraries/opensk/src/api/persist.rs Show resolved Hide resolved
libraries/opensk/src/api/persist.rs Outdated Show resolved Hide resolved
libraries/opensk/src/api/persist.rs Show resolved Hide resolved
@kaczmarczyck kaczmarczyck requested a review from ia0 April 3, 2024 13:52
ia0
ia0 previously approved these changes Apr 3, 2024
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.

LGTM
What's the testing strategy?

libraries/opensk/src/api/persist.rs Outdated Show resolved Hide resolved
@kaczmarczyck
Copy link
Collaborator Author

I flashed the PR on hardware, then went through the following tests, mostly storage related:

  • webauthn.io: register, then login.
  • Go to Chrome settings, delete some credentials, change the PIN, reset the security key.
  • A script that writes and then reads a LargeBlob.
  • A script that toggles alwaysUv.
  • Our configure script to write batch attestation key material.
  • webauthn.io again, but using attestation this time.

Then deploy erase_storage, deploy OpenSK again, and another round of the above. Before, it upgraded from the storage as it was right before this PR.

@kaczmarczyck kaczmarczyck requested a review from ia0 April 3, 2024 15:31
@kaczmarczyck kaczmarczyck merged commit 3faea59 into google:develop Apr 4, 2024
9 checks passed
@kaczmarczyck kaczmarczyck deleted the persistent-trait branch April 4, 2024 07:50
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