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

largeBlobKey extension #274

Merged
merged 1 commit into from
Jan 31, 2021

Conversation

kaczmarczyck
Copy link
Collaborator

Implements the last extension missing for CTAP 2.1. Also cleans up the GetInfo output for all options.

One design choice made here is how we want to store the per-credential largeBlobKey. The specification mandates:

  • only discoverable (aka resident) credentials need a largeBlobKey
  • only return a largeBlobKey when created during MakeCredential

Therefore, we have to store per-credential information. I decided to go with the 32 byte array itself, as the cryptographically most straight-forward implementation.
We could sometimes save some memory by only storing a bool indicating existence, and later derive the largeBlobKey using a master secret similar to credRandom. This approach only pays off with the second created credentials that uses largeBlobKey, because of the cost for master secrets. For users (including those never using largeBlobKey at all), I expect this to be more expensive on average.

  • Tests pass

@kaczmarczyck kaczmarczyck requested review from ia0 and jmichelp January 27, 2021 16:06
@kaczmarczyck kaczmarczyck self-assigned this Jan 27, 2021
@google-cla google-cla bot added the cla: yes label Jan 27, 2021
@kaczmarczyck kaczmarczyck changed the title new extension entry for largeBlobKey largeBlobKey extension Jan 27, 2021
@kaczmarczyck kaczmarczyck mentioned this pull request Jan 27, 2021
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.

How come the default cred protect became force pin change?

Non-logical changes LGTM, but I'll let @jmichelp approve if the logic needs to be reviewed too.

@kaczmarczyck
Copy link
Collaborator Author

It changed somewhere between review draft 1 and 2. I can go search for the specific PR and dig out the reasoning. But I'd assume the default cred protect isn't really necessary since you can get individual levels per credentials from credential management.

@ia0
Copy link
Member

ia0 commented Jan 27, 2021

It changed somewhere between review draft 1 and 2. I can go search for the specific PR and dig out the reasoning. But I'd assume the default cred protect isn't really necessary since you can get individual levels per credentials from credential management.

Sounds good, no need to search for references. I was just wondering why and the answer is the draft changed.

Copy link
Collaborator

@jmichelp jmichelp left a comment

Choose a reason for hiding this comment

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

Sorry, this took me a while because I had to review it side by side with the specification due to non-obvious changes :)

@kaczmarczyck kaczmarczyck merged commit 5683b45 into google:develop Jan 31, 2021
@kaczmarczyck kaczmarczyck deleted the extension-large-blobs branch January 31, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants