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 support for the largeBlobKeys extension and the largeBlobs command #41

Merged
merged 7 commits into from
Nov 28, 2023

Conversation

robin-nitrokey
Copy link
Member

@robin-nitrokey robin-nitrokey commented Nov 21, 2023

This patch implements the largeBlobKeys extension and the largeBlobs command. I tried to split it up into atomic commits to make it easier to review.

Limitations:

  • Currently, this implementation uses the core Trussed API, so the size of the entire large-blob array is limited to 1024 bytes as it has to fit into a Trussed message. The next step would be to add a chunked feature that replaces the SimpleStorage in src/ctap2/large_blobs.rs with a ChunkedStorage that uses the streaming extension. The implementation uses the Storage trait to try to ensure that the implementations can be easily switched.
  • The size of the response sent when reading the large-blob array depends on the maximum message size that the authenticator declares in the getInfo command. ctap-types has a REALISTIC_MAX_MESSAGE_SIZE constant (1200) and assumes that this is the maximum message size. But in fact, the max message size can be configured by the runner and is set to usbd_ctaphid::constants::MESSAGE_SIZE (3072). This does not matter at the moment because our large-blob array has at most 1024 bytes, so even the buffer based on the REALISTIC_MAX_MESSAGE_SIZE is large enough. Once we implement chunked storage, we need to add a feature flag to ctap-types to use the message size from usbd-ctaphid instead.

Open issues:

Fixes: #38

This patch updates the ctap-types dependency to pull in support for the
largeBlobKey extension and the largeBlobs command.
This patch adds support for the largeBlobKey extension to the get_info
command.  It also adds a config entry to be able to enable or disable
the extension.
This patch adds support for the largeBlobKey extension to
make_credential.  This means that we have to generate a 32-bit key and
store it together with the credential if requested by the platform.
This patch adds support for the largeBlobKey extension to get_assertion.
This means that we have to return the key stored together with the
credential if it is present and requested by the platform.
robin-nitrokey added a commit to Nitrokey/nitrokey-3-firmware that referenced this pull request Nov 21, 2023
This patch updates fido-authenticator to add support for the
largeBlobKey extension and the largeBlobs command.  See the
fido-authenticator PR for more information:
  Nitrokey/fido-authenticator#41
@robin-nitrokey
Copy link
Member Author

For testing, use these PRs:

You can use the following tools for testing:

  • nitropy fido2 list-large-blobs lists the number of entries in the large blob array, the number of credentials with large blob key, and the entries of the large blob array with information about the corresponding credential (if found).
  • https://webauthn-large-blob.glitch.me can be used to create credentials with a large-blob key and to write entries to the large-blob table (does not work in Firefox).
  • Chrome key management (chrome://settings/securityKeys → Sign-in data) performs garbage collection, i. e. it deletes all entries in the large-blob table without a matching credential.
  • Performing a reset with Chrome or nitropy should also clear the large-blob array.
  • Enabling logging in the firmware should provide some insight into the executed operations and their results.

Note that there is an open issue with Chrome reporting an error for writes even if the write is successful.

Copy link

@sosthene-nitrokey sosthene-nitrokey left a comment

Choose a reason for hiding this comment

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

Not yet tested on hardware.

Comment on lines +101 to +111
trait Storage<C>: Sized {
fn read(client: &mut C, location: Location, offset: usize, length: usize) -> Result<Chunk>;

fn start_write(client: &mut C, offset: usize, expected_length: usize) -> Result<Self>;

fn extend_buffer(&mut self, client: &mut C, data: &[u8]) -> Result<usize>;

fn validate_checksum(&mut self, client: &mut C) -> bool;

fn commit(&mut self, client: &mut C, location: Location) -> Result<()>;
}

Choose a reason for hiding this comment

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

Do we really need a dedicated trait for that?

Can't it be just a group of free-standing functions where the implementation is selected based on the feature-flag?

No need to block merging on that since the storage will be revisited soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was to have two implementations, one using the Trussed core API and one using the streaming extension. The trait should make sure that both are exchangeable.

src/ctap2.rs Show resolved Hide resolved
@robin-nitrokey
Copy link
Member Author

Indeed Chrome no longer shows an error if we send an empty response instead of a response with an empty map when writing the large-blob array: trussed-dev/ctap-types#24

@robin-nitrokey
Copy link
Member Author

I’ve updated Nitrokey/nitrokey-3-firmware#385 with a maximum size of 4096 bytes for the large-blob array. It mostly works fine in the usbip simulation, but sometimes after writing a large file, there seems to be an issue with the filesystem and subsequent larger writes fail.

This patch implements the largeBlobs command for reading and writing the
large-blob array.  Currently, the maximum size of the total array with
metadata is 1024 bytes because it has to fit in a Trussed message.  The
storage location can be configured by the runner.
This patch updates the credential management implementation to include
the largeBlobKey if present.
If a resident credential is passed in the allowlist, we don’t
deserialize the full credential.  This means that we previously did not
have access to the largeBlobKey in that case.  Therefore, this patch
adds the largeBlobKey to the StrippedCredential so that we can always
access it.

The downside is that this inceases the size of the credential ID.  So a
better alternative would be to load the full credential from the
filesystem instead.
@robin-nitrokey robin-nitrokey merged commit a4fff2f into main Nov 28, 2023
@robin-nitrokey robin-nitrokey deleted the large-blobs branch November 28, 2023 09:47
robin-nitrokey added a commit to Nitrokey/nitrokey-3-firmware that referenced this pull request Nov 29, 2023
This patch updates fido-authenticator to add support for the
largeBlobKey extension and the largeBlobs command in the test
configuration over USB.  See the fido-authenticator PR for more
information:
  Nitrokey/fido-authenticator#41
robin-nitrokey added a commit to Nitrokey/nitrokey-3-firmware that referenced this pull request Nov 29, 2023
This patch updates fido-authenticator to add support for the
largeBlobKey extension and the largeBlobs command in the test
configuration over USB.  See the fido-authenticator PR for more
information:
  Nitrokey/fido-authenticator#41
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.

Implement largeBlobKey extension
2 participants