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

Support hardware keys prompts in Connect #47652

Merged
merged 11 commits into from
Oct 23, 2024
Merged

Conversation

gzdunek
Copy link
Contributor

@gzdunek gzdunek commented Oct 17, 2024

Closes #30030 #17311 #34415

This is the last PR for a hardware keys support in Connect.

Since yubikey.go now supports custom prompts, we can pass it a one that shows modals in the Electron app.
The prompts are displayed as important modals. The UX of this is not perfect, since if you have to unlock the hardware key during log-in, you will see a dialog on top of other dialog. Earlier I considered integrating these prompts with a login dialog, but that would require some significant changes on the frontend side (if there’s a login in progress the prompt should be a part of the login dialog, otherwise it should be a standalone dialog). Because of the additional complexity, I kept this out of scope.

TODO: Add hardware keys to the test plan.

Best to review commit-by-commit.

How to test it

  1. Build tsh with PIV: PIV=dynamic make build/tsh (same for teleport)
  2. Require a hardware key in your role (I set it to hardware_key_touch_and_pin to have all possible prompts).

changelog: Add support for hardware keys in Teleport Connect

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.

The happy path seems to work fine, I was testing with hardware_key_touch_and_pin. I found one edge case though which seems worth addressing.

proto/teleport/lib/teleterm/v1/tshd_events_service.proto Outdated Show resolved Hide resolved
proto/teleport/lib/teleterm/v1/tshd_events_service.proto Outdated Show resolved Hide resolved
proto/teleport/lib/teleterm/v1/tshd_events_service.proto Outdated Show resolved Hide resolved
proto/teleport/lib/teleterm/v1/tshd_events_service.proto Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I found a weird edge case that I'm able to consistently reproduce:

  1. Log out of the cluster, remove your key and plug it back in.
  2. Make sure the cluster uses hardware_key_touch_and_pin.
  3. Provide credentials for the cluster.
  4. Tap the key to get through the regular MFA dialog.
  5. Provide the PIN to get through the next one.
  6. Close the PIV dialog asking you to tap the key.

At this point, the UI aborts the operation, but it looks like the key keeps blinking as if waiting for a touch. Matter of fact, if I try to log in again, when I get to the regular MFA dialog I have to tap the key twice. It seems like when the PIV touch dialog is closed, the request for a tap on tshd side is not properly canceled maybe?

FWIW, I can reproduce this with tsh (provide PIN, tap to get through "Tap any security key", then CTRL+C on "Tap your YubiKey") which indicates that it's not related to the JS side of things.

Copy link
Contributor Author

@gzdunek gzdunek Oct 22, 2024

Choose a reason for hiding this comment

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

This is possible. The problem comes from a fact that the signer.Sign interface doesn't accept a context so we can't just cancel the entire call that prompts for PIN/touch. Because of that, I had to introduce abandonableSign so at least I can abandon the call.

I thought that perhaps an option would be to close the entire PIV connection when the context is cancelled (although I'd like to avoid it) but it doesn't seem to change anything, the key still blinks for a few seconds. This may be YubiKey's behavior, where the key physcially waits for PIN/touch for some time.

Copy link
Member

Choose a reason for hiding this comment

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

Could you capture this in an issue so that we have something to refer to when someone runs into that as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #47810

@gzdunek gzdunek requested a review from ravicious October 22, 2024 12:21
@gzdunek
Copy link
Contributor Author

gzdunek commented Oct 22, 2024

Friendly ping @Joerger

@gzdunek gzdunek added this pull request to the merge queue Oct 23, 2024
Merged via the queue into master with commit 22b5014 Oct 23, 2024
44 checks passed
@gzdunek gzdunek deleted the gzdunek/connect-hardware-keys-ui branch October 23, 2024 07:39
@public-teleport-github-review-bot

@gzdunek See the table below for backport results.

Branch Result
branch/v16 Failed

gzdunek added a commit that referenced this pull request Dec 3, 2024
* Add new protos for hardware key prompts

* Implement hardware key prompts on the daemon side

* Show prompts in the UI

* `PromptHardwareKeyPINAsk` -> `PromptHardwareKeyPIN`

* Improve proto docs

* `PromptHardwareKeySlotOverwrite` -> `ConfirmHardwareKeySlotOverwrite`

* Fix typo

* Remove unnecessary `form`

* Pass an enum to `AskPIN` instead of the entire message

* Remove an invalid restriction of PIN/PUK to numbers only

* Improve the copy

(cherry picked from commit 22b5014)
github-merge-queue bot pushed a commit that referenced this pull request Dec 4, 2024
* Support hardware keys prompts in Connect (#47652)

* Add new protos for hardware key prompts

* Implement hardware key prompts on the daemon side

* Show prompts in the UI

* `PromptHardwareKeyPINAsk` -> `PromptHardwareKeyPIN`

* Improve proto docs

* `PromptHardwareKeySlotOverwrite` -> `ConfirmHardwareKeySlotOverwrite`

* Fix typo

* Remove unnecessary `form`

* Pass an enum to `AskPIN` instead of the entire message

* Remove an invalid restriction of PIN/PUK to numbers only

* Improve the copy

(cherry picked from commit 22b5014)

* Update generated proto files

* Regenerate proto file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yubikey error blocks using Teleport Connect
3 participants