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

Implement FIDO2 credential picker #14033

Merged
merged 5 commits into from
Jul 1, 2022
Merged

Conversation

codingllama
Copy link
Contributor

Add a credential picker to the tsh FIDO2/WebAuthn backend.

The PR pulls a recent patch in our go-libfido2 fork that makes it correctly return multiple assertions from the authenticator. This allows us to implement the credential picker for FIDO2, simplify our implementation and provide the exact same UX that browsers use (always 1-touch for bio, touch->PIN->touch otherwise).

I've dropped concepts like "optimistic assertions" and "eager PIN prompts" in favor of a simple, uniform implementation.

Issue #13901.

@github-actions github-actions bot requested review from ravicious and strideynet June 30, 2022 22:34
@codingllama
Copy link
Contributor Author

FYI @kimlisa @zmb3 @xinding33, this is what we talked about last week.

I'm tempted to backport to v10.0.0, as this is actually better in various ways.

@xinding33
Copy link
Contributor

I'll leave it to @zmb3 to decide whether or not we should backport this to v10.0.0. This is a great UX improvement though, love it!

Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

LGTM!

@codingllama
Copy link
Contributor Author

Thanks folks for the feedback and quick review.

Friendly ping @ravicious @strideynet ?

@codingllama codingllama force-pushed the codingllama/credential-picker branch from f635e2a to 3d3bae3 Compare July 1, 2022 14:40
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I wanted to test this myself but I'd need to make a tag I could get signed & notarized version of tsh, wouldn't it? After that I don't think I could add more than one passwordless device other than TouchID since I only have the regular yubikeys 5C.

lib/auth/webauthncli/fido2_prompt_test.go Show resolved Hide resolved
@codingllama
Copy link
Contributor Author

codingllama commented Jul 1, 2022

@ravicious no need to sign/notarize, but you do need libfido2 installed and visible via pkg-config libfido2. Then, go build -tags=libfido2 ./tool/tsh should do the trick. Hit me up on Slack if you have problems getting it to work.

@codingllama codingllama enabled auto-merge (squash) July 1, 2022 15:03
@codingllama codingllama merged commit e005f60 into master Jul 1, 2022
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

@codingllama See the table below for backport results.

Branch Result
branch/v10 Create PR

@codingllama codingllama deleted the codingllama/credential-picker branch July 1, 2022 16:58
codingllama added a commit that referenced this pull request Jul 1, 2022
Backport #14033 to branch/v10

Add a credential picker to the tsh FIDO2/WebAuthn backend.

The PR pulls a recent patch in our go-libfido2 fork that makes it correctly
return multiple assertions from the authenticator. This allows us to implement
the credential picker for FIDO2, simplify our implementation and provide the
exact same UX that browsers use (always 1-touch for bio, touch->PIN->touch
otherwise).

I've dropped concepts like "optimistic assertions" and "eager PIN prompts" in
favor of a simple, uniform implementation.

Issue #13901.

* Prompt for credentials in LoginPrompt
* Update go-libfido2
* Implement FIDO2 credential picker
* Drop optimistic assertions, only set user if explicit
* Add license to fido2_prompt_test.go
codingllama added a commit that referenced this pull request Jul 7, 2022
codingllama added a commit that referenced this pull request Jul 8, 2022
Add newline missing from PR #14033.

Backport #14167 to branch/v10
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.

4 participants