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

Allow displaying hardware keys prompts when relogin is in progress #48813

Merged
merged 14 commits into from
Nov 15, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions lib/teleterm/daemon/hardwarekeyprompt.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,20 @@ import (

// NewHardwareKeyPromptConstructor returns a new hardware key prompt constructor
// for this service and the given root cluster URI.
// TODO(gzdunek): Improve multi-cluster and multi-hardware keys support.
// The code in yubikey.go doesn't really support using multiple hardware keys (like one per cluster):
// 1. We don't offer a choice which key should be used on the initial login.
// 2. Keys are cached per slot, not per physical key - it's not possible to use different keys with the same slot.
//
// Additionally, using the same hardware key for two clusters is not ideal too.
// Since we cache the keys per slot, if two clusters specify the same one,
// the user will always see the prompt for the same cluster URI. For example, if you are logged into both
// cluster-a and cluster-b, the prompt will always say "Unlock hardware key to access cluster-b."
Comment on lines +40 to +42
Copy link
Contributor Author

@gzdunek gzdunek Nov 12, 2024

Choose a reason for hiding this comment

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

I think that having a prompt per cluster was a mistake, since one prompt can be used for multiple clusters (I didn't realize that back then). While displaying a card name will require too much work, maybe we should just not display a cluster name in the dialog? I think a scenario where a user logs in with the same key to two clusters is more likely than the one where separate keys are used for different clusters.

// It seems that the better option would be to have a prompt per physical key, not per cluster.
// But I will leave that for the future, it's hard to say how common these scenarios will be in Connect.
//
// Because of the above, we don't have any mutex here. I don't expect receiving prompts
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
// from different hardware keys at the same time.
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
func (s *Service) NewHardwareKeyPromptConstructor(rootClusterURI uri.ResourceURI) keys.HardwareKeyPrompt {
return &hardwareKeyPrompter{s: s, rootClusterURI: rootClusterURI}
}
Expand All @@ -41,10 +55,6 @@ type hardwareKeyPrompter struct {

// Touch prompts the user to touch the hardware key.
func (h *hardwareKeyPrompter) Touch(ctx context.Context) error {
if err := h.s.singleImportantModalSemaphore.Acquire(ctx); err != nil {
return trace.Wrap(err)
}
defer h.s.singleImportantModalSemaphore.Release()
_, err := h.s.tshdEventsClient.PromptHardwareKeyTouch(ctx, &api.PromptHardwareKeyTouchRequest{
RootClusterUri: h.rootClusterURI.String(),
})
Expand All @@ -56,10 +66,6 @@ func (h *hardwareKeyPrompter) Touch(ctx context.Context) error {

// AskPIN prompts the user for a PIN.
func (h *hardwareKeyPrompter) AskPIN(ctx context.Context, requirement keys.PINPromptRequirement) (string, error) {
if err := h.s.singleImportantModalSemaphore.Acquire(ctx); err != nil {
return "", trace.Wrap(err)
}
defer h.s.singleImportantModalSemaphore.Release()
res, err := h.s.tshdEventsClient.PromptHardwareKeyPIN(ctx, &api.PromptHardwareKeyPINRequest{
RootClusterUri: h.rootClusterURI.String(),
PinOptional: requirement == keys.PINOptional,
Expand All @@ -74,10 +80,6 @@ func (h *hardwareKeyPrompter) AskPIN(ctx context.Context, requirement keys.PINPr
// The Electron app prompt must handle default values for PIN and PUK,
// preventing the user from submitting empty/default values.
func (h *hardwareKeyPrompter) ChangePIN(ctx context.Context) (*keys.PINAndPUK, error) {
if err := h.s.singleImportantModalSemaphore.Acquire(ctx); err != nil {
return nil, trace.Wrap(err)
}
defer h.s.singleImportantModalSemaphore.Release()
res, err := h.s.tshdEventsClient.PromptHardwareKeyPINChange(ctx, &api.PromptHardwareKeyPINChangeRequest{
RootClusterUri: h.rootClusterURI.String(),
})
Expand All @@ -93,10 +95,6 @@ func (h *hardwareKeyPrompter) ChangePIN(ctx context.Context) (*keys.PINAndPUK, e

// ConfirmSlotOverwrite asks the user if the slot's private key and certificate can be overridden.
func (h *hardwareKeyPrompter) ConfirmSlotOverwrite(ctx context.Context, message string) (bool, error) {
if err := h.s.singleImportantModalSemaphore.Acquire(ctx); err != nil {
return false, trace.Wrap(err)
}
defer h.s.singleImportantModalSemaphore.Release()
res, err := h.s.tshdEventsClient.ConfirmHardwareKeySlotOverwrite(ctx, &api.ConfirmHardwareKeySlotOverwriteRequest{
RootClusterUri: h.rootClusterURI.String(),
Message: message,
Expand Down
Loading