Skip to content

Dynamic identity file reloading support for API Client#29139

Merged
strideynet merged 15 commits intomasterfrom
strideynet/dynamic-identity-file-credential
Aug 28, 2023
Merged

Dynamic identity file reloading support for API Client#29139
strideynet merged 15 commits intomasterfrom
strideynet/dynamic-identity-file-credential

Conversation

@strideynet
Copy link
Copy Markdown
Contributor

@strideynet strideynet commented Jul 14, 2023

Part of #23341

For now, this just introduces the new credential loader type. It doesn't implement the usage of this. I've left the reload strategy as an exercise to the consumer for now - we can wrap around this with inotify or an interval based reloader - but it'll be more natural to work out how to use this in the context of the consumer. I also didn't want to bring any further dependencies into the API package (e.g https://github.com/fsnotify/fsnotify)

Example usage:

func main() {
	utils.InitLogger(utils.LoggingForCLI, logrus.DebugLevel, true)
	ctx := context.Background()

	cred, err := client.NewDynamicIdentityFileCreds(
		"./identity",
	)
	if err != nil {
		panic(err)
	}
	go func() {
		for {
			logrus.Info("reloading credentials")
			if err := cred.Reload(); err != nil {
				panic(err)
			}
			logrus.Info("reloaded credentials")
			time.Sleep(5 * time.Minute)
		}
	}()

	clt, err := client.New(ctx, client.Config{
		Addrs: []string{"leaf.tele.ottr.sh:443"},
		Credentials: []client.Credentials{
			cred,
		},
	})
	if err != nil {
		panic(err)
	}


	for {
		logrus.Info("Fetching nodes")
		_, err := clt.GetNodes(ctx, apidefaults.Namespace)
		if err != nil {
			logrus.WithError(err).Error("Fetching nodes: ERROR")
		} else {
			logrus.Info("Fetching nodes: OK")
		}
		time.Sleep(1 * time.Second)
	}
}

I manually tested by:

  • Configured Machine ID to produce a short-lived identity
  • Starting the above script
  • Waiting until it was meant to have expired.
  • Severing the connection between the client and the Auth Server. This triggers a reconnect.
  • Observing that it is able to reconnect and continue fetching nodes.

This is in contrast to using the old client.LoadIdentityFile - which upon severing the connection resulted in the errors regarding trying to connect with an expired certificate.

@strideynet strideynet changed the title Dynamic identity file reloaidng support for API Client Dynamic identity file reloading support for API Client Jul 14, 2023
@strideynet strideynet marked this pull request as ready for review August 23, 2023 15:38
@github-actions github-actions Bot requested review from greedy52 and r0mant August 23, 2023 15:42
Comment thread api/client/credentials.go Outdated
Comment thread api/client/credentials.go
Comment on lines +444 to +453
cert, err := keys.X509KeyPair(id.Certs.TLS, id.PrivateKey)
if err != nil {
return trace.Wrap(err)
}
pool := x509.NewCertPool()
for _, caCerts := range id.CACerts.TLS {
if !pool.AppendCertsFromPEM(caCerts) {
return trace.BadParameter("invalid CA cert PEM")
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: just personal preference. I would prefer calling id.TLSConfig and taking cert and pool from there or maybe saving the identityTLSConfig and using that later instead of "manually" doing it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I'd prefer to keep it as is. The reason for this is that tls.Config is a poor way of passing just a key/cert/CAs around - it has other fields that the underlying implementation could start using instead of the Certificates or RootCAs fields and I can easily see a future refactor to underlying parts breaking the DynamicIdentityFileCreds - the .CACerts and .Certs fields are much stricter and it's much more likely to be clearer that changing this risks breaking an upstream.

I have a somewhat long-term plan to try and convince folks that we should stop returning tls.Config and ssh.ClientConfig from the client.Credentials type. These configs are for more than just carrying certs/keys around and it's creating a conflict of responsibility for what the Credential should do and what the Client should do.

Comment thread api/client/credentials.go Outdated
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from r0mant August 28, 2023 11:35
@strideynet strideynet added this pull request to the merge queue Aug 28, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 28, 2023
@strideynet strideynet added this pull request to the merge queue Aug 28, 2023
Merged via the queue into master with commit 7cc2e28 Aug 28, 2023
@strideynet strideynet deleted the strideynet/dynamic-identity-file-credential branch August 28, 2023 12:23
@public-teleport-github-review-bot
Copy link
Copy Markdown

@strideynet See the table below for backport results.

Branch Result
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants