Cache PIV connections to share across the program execution

[gzdunek]

Closes #46372
I think this fix should close the linked issue, but unfortunately I couldn't reproduce it.
This also provides a foundation for the hardware keys support in Connect which issues many requests asynchronously.

This PR is in 90% based on @Joerger work on caching PIV connections. Thanks to it, subsequent calls to the YubiKey don't have to constantly open & close a connection (which is time-consuming and blocks the key), but instead use a shared connection directly.
However, the shared connection can't be kept open for the lifetime of the program, because it would block access to the YubiKey for other programs (for example, another instance of tsh or tctl). Therefore, the connection is released after 5 seconds.

In addition to Brian's changes, I added a "warm up" call to the YubiKey, as proposed by Nic. This is mainly needed for Connect so that we can wait indefinitely for the user to enter a PIN/touch. But what's more important, if the user cancels the prompt, we can return early, before we start dialing. Otherwise, the gRPC retry mechanism will try to establish a connection, showing the same prompt to the user over and over again.
I think "warming up" the key is useful for tsh too.

Best to review commit by commit.

changelog: Resolved an issue that caused false positive errors incorrectly indicating that the YubiKey was in use by another application, while only tsh was accessing it

[gzdunek]

I believe this timeout needs to greater than the timeout for releasing a connection. Otherwise, a caller that doesn't use the shared connection has basically no chance to open the YubiKey.

Unfortunately, this also made the tests take much longer (6-11 seconds for a test). I tried to reuse the same YubiKey object as much as possible in the tests, but keys.GetYubiKeyPrivateKey() opens its own connection, and I'm not able to easily pass YubiKey that I get from keys.FindYubiKey() earlier.

[gzdunek]

Friendly ping @nklaassen, @Joerger

[Joerger]

Tested it with tsh, LGTM, barring some minor comments.

[Joerger]

One fix and a couple nits.

fix: We should actually sleep from the start, before decrementing active connections. Otherwise, we could have the following situation: client connects, client disconnects. 4 seconds later, second client connects, and disconnects. 1 second later, the first client closes the connection. Instead, we should wait 5 seconds from the second client disconnect before closing. So we need to run the goroutine+sleep before decrementing the active connections.

nit: Now that we are decrementing and closing under the same lock, we can safely remove the c.conn != nil check, this should never be the case anymore.

nit2: We also don't need the if c.activeConnections > 0 check. Every connect should be accompanied by release, so active connections will always be > 0 in this check, before it's decremented to its minimum value 0.

IMO the nits make it easier to implement bugs in the future, I'd rather the system fail loudly with faulty code (panic) so we fix it promptly.

Suggested change

	release := func() {
	c.mu.Lock()
	defer c.mu.Unlock()
	if c.activeConnections > 0 {
	c.activeConnections--
	}
	// If this is the last active connection, close after a delay,
	// giving other callers a chance to claim the connection first.
	if c.activeConnections == 0 {
	go func() {
	time.Sleep(releaseConnectionDelay)
	c.mu.Lock()
	defer c.mu.Unlock()
	// Close the shared connection if there are still no new active connections,
	// and the connection hasn't been yet closed by another goroutine.
	if c.activeConnections == 0 && c.conn != nil {
	c.conn.Close()
	c.conn = nil
	}
	}()
	}
	}
	release := func() {
	go func() {
	time.Sleep(releaseConnectionDelay)
	c.mu.Lock()
	defer c.mu.Unlock()
	c.activeConnections--
	if c.activeConnections == 0 {
	c.conn.Close()
	c.conn = nil
	}
	}
	}

[gzdunek]

Otherwise, we could have the following situation: client connects, client disconnects. 4 seconds later, second client connects, and disconnects. 1 second later, the first client closes the connection. Instead, we should wait 5 seconds from the second client disconnect before closing.

Yeah, I thought about it, but for some reason it seemed to me that the correct behavior is to close the connection only after waiting for the first client to release 🤦. But I agree, it makes much more sense to extend the duration with each connected client.
Fix applied.

[nklaassen]

lgtm assuming Brian's suggesting here gets accepted/resolved https://github.com/gravitational/teleport/pull/47091/files#r1797232730

[public-teleport-github-review-bot]

@gzdunek See the table below for backport results.

Branch	Result
branch/v14	Failed
branch/v15	Failed
branch/v16	Failed