-
Notifications
You must be signed in to change notification settings - Fork 0
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
Reduce ID length for new credentials #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far
For the sake of coherency in GA - can't you just echo back the incoming credential ID, if validated? |
For resident keys, the sever does not have to send the credential ID. We could use it as an additional check for non-resident keys but I’m not sure if this is worth the implementation effort. (We already sort of do that for non-resident keys because the credential ID passed in the allow list determines if the credential is deserialized to a |
This patch implements the following changes to reduce the ID length for new credentials: - Rename the old Credential type to FullCredential and introduce a StrippedCredential type and a Credential enum to differentiate between full and reduced credential data. - Flatten the credential data to reduce encoding overhead. - Remove the RP id from the credential data to reduce the total length. - Add a marker field use_short_id to FullCredential so that we don’t change the credential ID for existing RKs. Fixes: Nitrokey#29
633b257
to
d318c11
Compare
I think this should be ready to merge. Potentially, we could drop some more fields from the stripped credentials, but I don’t think that’s necessary atm. |
Nitrokey/nitrokey-3-tests@a70cad3 regular and upgrade tests passed when using this version of fido-authenticator on top of Nitrokey/nitrokey-3-firmware#350. |
This patch implements the following changes to reduce the ID length for new credentials:
To do:
StrippedCredential
inregister
(authenticate
automatically handles both by using theCredential
enum).Fixes: #29