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

Update ctap-types and fido-authenticator #16

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

robin-nitrokey
Copy link
Member

This patch updates the ctap-types and fido-authenticator dependencies to
add support for the pin protocl field in the hmac-secret extension data
so that the authenticatorGetAssertion command works with newer clients.

Fixes #14.

This is a draft as it requires changes to the fido-authenticator (trussed-dev/fido-authenticator#1) and ctap-types (trussed-dev/ctap-types#2) dependencies. We can either wait for upstream to merge our changes or use forks instead.

@robin-nitrokey robin-nitrokey added the upstream This pull request could be upstreamed label Nov 4, 2021
@szszszsz
Copy link
Member

This one is still draft, so I understand the review is not required yet?
Anyway, let's use Nitrokey space for the final references in Cargo.

@nickray
Copy link

nickray commented Nov 24, 2021

sneaks into your PRs
I'd very much prefer if we can work together to have shared dependencies on crates.io releases (where applicable, e.g. clearly for ctap-types, less clearly for fido-authenticator although that should be "vendor extendable" via dependency injection over time, probably less so in something like admin-app), vs. Nitrokey forking everything like in the C days. While I acknowledge not all of the parts have "good" releases yet, there was a lot of effort put into modularizing things sufficiently for vendor forks not to be necessary in general.

@robin-nitrokey
Copy link
Member Author

@nickray That is definitely our intention and we share that goal. But there might be situations where we want to push out a release with functionality that is not yet accepted in upstream (as is the case here). That’s where we would use forks – with the goal to move back to the upstream crate as soon as possible.

@robin-nitrokey robin-nitrokey removed the upstream This pull request could be upstreamed label Nov 26, 2021
@robin-nitrokey
Copy link
Member Author

Our changes have been merged into the upstream crates, so we can now use them directly. I’m waiting for #20 before updating this PR to avoid dependency duplications.

@szszszsz
Copy link
Member

@robin-nitrokey PR #20 updated

This patch updates the ctap-types and fido-authenticator dependencies to
add support for the pin protocol field in the hmac-secret extension data
so that the authenticatorGetAssertion command works with newer clients.

Fixes Nitrokey#14.
@robin-nitrokey
Copy link
Member Author

Updated and rebased onto nitrokey-main. @szszszsz Can you please review this?

@robin-nitrokey robin-nitrokey marked this pull request as ready for review December 1, 2021 12:31
@@ -2,7 +2,16 @@

## Bugfixes

- admin-app: Fix CTAPHID command dispatch (#8).
- admin-app: Fix CTAPHID command dispatch ([#8][]).
Copy link
Member

@szszszsz szszszsz Dec 1, 2021

Choose a reason for hiding this comment

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

nit: I believe the format for the referenced link is just [link-name], not [link-name][]

Copy link
Member Author

@robin-nitrokey robin-nitrokey Dec 1, 2021

Choose a reason for hiding this comment

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

That shortcut only works in CommonMark, not in plain Markdown, see the Markdown spec.

@robin-nitrokey robin-nitrokey merged commit 6b81de3 into Nitrokey:nitrokey-main Dec 1, 2021
@robin-nitrokey robin-nitrokey deleted the hmac-secret branch December 1, 2021 12:51
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.

fido-authenticator: authenticatorGetAssertion does not work with hmac-secret extension
3 participants