feat: Hardware Key Agent - initialize hardware key service at start of tsh daemon

[Joerger]

Changes:

• Initialize the hardware key agent at the start of tsh daemon
• Always use the direct PIV service since Teleport Connect's built in hardware key prompts provide the best UX.
  • This avoids some unexpected issues, such as tsh daemon connecting to the hardware key agent that it started due to the client connection forming in the background.
• Put tshdEventsClient into a protected lazy loading struct for easier usage outside of the daemon service that previously had full ownership.

[zmb3]

Can we avoid having an exported field with an unexported type?

[ravicious]

Why is that bad?

[Joerger]

I'm not sure there's a written rule but it's just sorta bad practice.

If you think about it, having it be a private struct does nothing since callers can still call NewTshdEventsClient and call its public methods. However you couldn't do things like add a TshdEventsClient field to structs from other packages, you'd actually need to create an interface to utilize the struct as a field. So IIUC this package should either provide that public interface, or provide a public struct. Or make it private of course.

[zmb3]

Can we replace this whole mutex+channel setup thing with sync.Once?

[ravicious]

At first I thought it could be replaced with syncflight.Group but I wasn't 100% sure. When I tried to do so, I realized that syncflight.Group requires both callsites (Connect and GetClient) to be able to instantiate a new client, but GetClient actually cannot do this because it doesn't know the server address. In that way, GetClient needs the channel to wait for the eventual call to Connect to set up the client.

So I think at best both syncflight.Group and sync.Once are only able to get rid of the mutex and then checking the channel in Connect, but that's about it. Though the code might end up expressing the intent a bit better, at first I wasn't sure why connectedChan needs to be read from in Connect.

But then again, I might be wrong as I'm not particularly good at writing concurrent code.

[zmb3]

but GetClient actually cannot do this because it doesn't know the server address

I see. sync.OnceValues would be a nice fit if we had the server address up front, but now that I understand things I think this is fine as-is.

[Joerger]

I initially was using a sync.Once in Connect, but a mutex prevents multiple callers of Connect from trying to connect a client at the same time. If we replaced it with a sync.Once, then we'd need to close unused connections which isn't easy to represent with a sync.Once:

	conn, err := grpc.NewClient(serverAddress, withCreds)
	if err != nil {
		return trace.Wrap(err)
	}

	// Successfully connected set the client and signal to any waiters.
	c.connectOnce.Do(func() {
		c.client = api.NewTshdEventsServiceClient(conn)
		close(c.connectedChan)
	})

	// conn leaks if the once condition has already been called.

[ravicious]

Should we wrap the error here?

[Joerger]

If the function already wraps the error, then there's no reason to wrap it unless we want to add a new message. See below trace.Wrap just returns the existing error.

	if traceErr, ok := err.(Error); ok {
		trace = traceErr
	} else {
		trace = newTrace(err)
	}

[ravicious]

All this time I've thought that trace.Wrap is important so that we can capture the stack trace. 🤦 But this is of course done with the runtime package.

[ravicious]

Why is that bad?

[ravicious]

I get a panic when the frontend app executes this through an RPC.

Logs

TSHD panic: runtime error: invalid memory address or nil pointer dereference
TSHD [signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x10684313c]
TSHD
TSHD goroutine 44 [running]:
TSHD github.com/gravitational/teleport/lib/teleterm/daemon.(*tshdEventsClient).Connect(0x1084f21e0?, {0x14000b961e0?, 0x0?})
TSHD 	github.com/gravitational/teleport/lib/teleterm/daemon/events_client.go:55 +0x2c
TSHD github.com/gravitational/teleport/lib/teleterm/daemon.(*Service).UpdateAndDialTshdEventsServerAddress(...)
TSHD 	github.com/gravitational/teleport/lib/teleterm/daemon/daemon.go:928
TSHD github.com/gravitational/teleport/lib/teleterm/apiserver/handler.(*Handler).UpdateTshdEventsServerAddress(0x10ac08900?, {0x10858c020?, 0x14000d1b808?}, 0x10457b168?)
TSHD 	github.com/gravitational/teleport/lib/teleterm/apiserver/handler/handler_tshd_events.go:30 +0x30
TSHD github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1._TerminalService_UpdateTshdEventsServerAddress_Handler.func1({0x10867c480?, 0x14000bee5a0?}, {0x1081b1c40?, 0x1400079cfc0?})
TSHD 	github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1/service_grpc.pb.go:958 +0xd0
TSHD github.com/gravitational/teleport/lib/teleterm/apiserver.New.withErrorHandling.func1({0x10867c480, 0x14000bee5a0}, {0x1081b1c40?, 0x1400079cfc0?}, 0x80?, 0x108423420?)
TSHD 	github.com/gravitational/teleport/lib/teleterm/apiserver/middleware.go:38 +0x40
TSHD github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1._TerminalService_UpdateTshdEventsServerAddress_Handler({0x10858c020, 0x140000c1eb0}, {0x10867c480, 0x14000bee5a0}, 0x1400034d180, 0x14000b83880)
TSHD 	github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1/service_grpc.pb.go:960 +0x148
TSHD google.golang.org/grpc.(*Server).processUnaryRPC(0x14000ba2800, {0x10867c480, 0x14000bee510}, 0x14000bf0300, 0x14000bcdf50, 0x10ac3a080, 0x0)
TSHD 	google.golang.org/grpc@v1.71.0/server.go:1405 +0xc9c
TSHD google.golang.org/grpc.(*Server).handleStream(0x14000ba2800, {0x10867e000, 0x14000bd84e0}, 0x14000bf0300)
TSHD 	google.golang.org/grpc@v1.71.0/server.go:1815 +0x900
TSHD google.golang.org/grpc.(*Server).serveStreams.func2.1()
TSHD 	google.golang.org/grpc@v1.71.0/server.go:1035 +0x84
TSHD created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 43
TSHD 	google.golang.org/grpc@v1.71.0/server.go:1046 +0x138
MAIN [Main] info: tshd exited with code 2
MAIN [Main] error: Could not fetch root clusters {"name":"TshdRpcError","message":"Connection dropped","stack":"RpcError: Connection dropped

[Joerger]

Yeah I forgot to actually set TshdEventsClient 💀

[ravicious]

The app seems to work fine, I tested tshd sending a relogin prompt to the frontend app. If there are any other issues we'll likely catch them during the test plan.