Session expired message for tsh proxy db by greedy52 · Pull Request #15398 · gravitational/teleport

greedy52 · 2022-08-10T20:01:59Z

No description provided.

greedy52 · 2022-08-26T18:54:38Z

@smallinsky @ravicious I have tested this for tsh proxy db and it works as expected when a new connection comes. Let me know what you think about the approach. If good I will open this for review.

smallinsky · 2022-08-29T13:28:02Z

 	}

+	if arg.onNewConnection != nil {
+		dbCert, err := certFromPath(arg.profile.DatabaseCertPathForCluster(arg.cliConf.SiteName, arg.routeToDatabase.ServiceName))


Does the cert can be reissued without restarting the local proxy ? If yes the dbCert will contains the stale cert data. Reading the cert from disk for each connection is not effective but maybe we can load cert periodically once per some timeout ?

Does the cert can be reissued without restarting the local proxy?

Yes, the cert can be reissued if the user runs other tsh commands (or if Teleport Connect tries to do something). However, the LocalProxy is also using the stale cert with today's implementation.

Do we also want to add the functionality to overwrite the cert LocalProxy is using (without restarting) in this PR? There are many approaches we can do too.

Can we cache the last cert loaded and check if it has expired? If it has expired then prompt user for credentials to reissue the cert.

ravicious

The approach works for me in general. I have a proof of concept of tshd -> Connect communication where Connect calls tshd to establish a server stream. Then I just put stuff on a channel within the callback and whatever is put on the channel is sent over the stream. I just need to figure out a couple of nuances around errors and reconnecting.

I haven't got around to replicating the cert expiration logic in Connect yet. I'm glad that I didn't because guessing from Marek's comment maybe we'll need to take a slightly different approach in Connect as well. 😏

ravicious · 2022-08-29T15:19:57Z

+	onNewConnection := func(dbCert *x509.Certificate, lp *alpnproxy.LocalProxy, conn net.Conn) {
+		if time.Now().After(dbCert.NotAfter) {
+			fmt.Fprintln(cf.Stdout())
+			fmt.Fprintln(cf.Stdout(), "Your database session is expired. Please restart the local proxy.")


Suggested change

fmt.Fprintln(cf.Stdout(), "Your database session is expired. Please restart the local proxy.")

fmt.Fprintln(cf.Stdout(), "Your database session has expired. Please restart the local proxy.")

I think "has" is more correct here though I'm not 100% sure.

ravicious · 2022-09-01T13:58:41Z

@greedy52 #16043 makes use of OnNewConnection, I'd appreciate if you could take a look at it some time next week!

GavinFrazar · 2022-09-07T03:35:22Z

+	//
+	// Note that the callback blocks handling of the connection.


I see this note but I just want to clarify that your intention was to block the LocalProxy.Start for {...} loop entirely or just an individual connection? We are doing the former - but if that was your intention could you explain why not call the callback inside the spawned goroutine just before calling l.handleDownstreamConnection?

good question. I guess this note is inaccurate. In current use cases, I think we want to block the LocalProxy.Start, meaning when detecting a certain situation by this callback, there is no point in accepting another new connection until the situation is resolved. For the MFA flow, feel free to do anything that works for you.

GavinFrazar · 2022-09-07T03:49:09Z

+	onNewConnection := func(dbCert *x509.Certificate, lp *alpnproxy.LocalProxy, conn net.Conn) {
+		if time.Now().After(dbCert.NotAfter) {
+			fmt.Fprintln(cf.Stdout())
+			fmt.Fprintln(cf.Stdout(), "Your database session is expired. Please restart the local proxy.")


Why should they restart the local proxy? If the database cli is providing the certs and the local proxy is just forwarding, can't they just use tsh db login and it will work? This works without --tunnel already, they just need to know to do it

This message is shown at the end of tsh proxy db command to ask the user to rerun whatever tsh proxy db command they are currently running.

Oh I see - does tsh proxy db not work with reissued certs if it is started with expired certs? I actually haven't tried that: I only tried waiting for the 1 minute cert to expire

ravicious · 2022-09-28T08:56:03Z

@greedy52 Any plans to merge this in? I might need a week or two more but I'm getting close to making a PR which utilizes this new callback.

GavinFrazar · 2022-09-28T16:03:37Z

@greedy52 Any plans to merge this in? I might need a week or two more but I'm getting close to making a PR which utilizes this new callback.

Callback will be added to close #12538 although we're waiting on RFD now - but I'll see if I can add that sooner than the RFD approval since the RFD is mainly about removing the 1 minute cert TTL for per-session-mfa db access

ravicious · 2022-10-03T15:54:26Z

Callback will be added to close #12538 although we're waiting on RFD now - but I'll see if I can add that sooner than the RFD approval since the RFD is mainly about removing the 1 minute cert TTL for per-session-mfa db access

I'd appreciate if you could add it sooner since I'll be adding stuff to handle expired certs before dealing with per-session MFA in Connect. Well, unless you think that on your side addressing both problems at the same time would be more beneficial.

greedy52 · 2022-10-03T16:39:41Z

Sorry guys for these troubles.

@GavinFrazar by any chance you have a prototype of how the callback will look like for your change?

@ravicious is it ok for you to include the changes from lib/srv/alpnproxy/local_proxy.go from this PR directly in your change? It's reasonably small and shouldn't break anything so i think it's safe for you to check it in, in case Gavin's change comes after yours.

GavinFrazar · 2022-10-03T19:00:27Z

I'm going to make the PR to add the callback for the local proxy today. I'm just really struggling with setting up an integration test for refreshing expired certs - that is what's blocking me. Basic problem is mocking time, because TLS handshakes use the real system clock.

GavinFrazar · 2022-10-04T01:46:55Z

PR adding callbacks: #16958

greedy52 · 2022-10-04T19:40:46Z

closed in favor of #16958

Session expired message for tsh proxy db

7ecff82

greedy52 added tsh tsh - Teleport's command line tool for logging into nodes running Teleport. database-access Database access related issues and PRs labels Aug 10, 2022

greedy52 self-assigned this Aug 10, 2022

greedy52 added 2 commits August 26, 2022 12:38

simplify message

4a2abba

beautify

b26d4e9

smallinsky reviewed Aug 29, 2022

View reviewed changes

ravicious reviewed Aug 29, 2022

View reviewed changes

ravicious mentioned this pull request Sep 1, 2022

tshd-initiated communication for Connect #16043

Closed

GavinFrazar reviewed Sep 7, 2022

View reviewed changes

greedy52 closed this Oct 4, 2022

greedy52 deleted the STeve/14180_connection_callback branch October 21, 2022 17:31

	fmt.Fprintln(cf.Stdout(), "Your database session is expired. Please restart the local proxy.")
	fmt.Fprintln(cf.Stdout(), "Your database session has expired. Please restart the local proxy.")

		//
		// Note that the callback blocks handling of the connection.

Conversation

greedy52 commented Aug 10, 2022

Uh oh!

greedy52 commented Aug 26, 2022

Uh oh!

smallinsky Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

greedy52 Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

GavinFrazar Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

ravicious Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

ravicious commented Sep 1, 2022

Uh oh!

GavinFrazar Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greedy52 Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

GavinFrazar Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greedy52 Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

GavinFrazar Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious commented Sep 28, 2022

Uh oh!

GavinFrazar commented Sep 28, 2022

Uh oh!

ravicious commented Oct 3, 2022

Uh oh!

greedy52 commented Oct 3, 2022

Uh oh!

GavinFrazar commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GavinFrazar commented Oct 4, 2022

Uh oh!

greedy52 commented Oct 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GavinFrazar Sep 7, 2022 •

edited

Loading

GavinFrazar Sep 7, 2022 •

edited

Loading

GavinFrazar Sep 7, 2022 •

edited

Loading

GavinFrazar commented Oct 3, 2022 •

edited

Loading