Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add SNI override for k8s 1.18 config file #5001

Closed
klizhentas opened this issue Nov 28, 2020 · 11 comments
Closed

Add SNI override for k8s 1.18 config file #5001

klizhentas opened this issue Nov 28, 2020 · 11 comments
Assignees
Milestone

Comments

@klizhentas
Copy link
Contributor

kubectl added support for SNI overrides in 1.18: kubernetes/kubernetes#88769

we should update tsh to set the tls-server-name field in kubeconfig which should, theoretically, solve the problem of 600+ clusters solved in #3870 for tsh

probably requires updating kube proxy GetTLSConfigForClient to match logic in middleware

verify that solution solves the problem

@klizhentas klizhentas added this to the 5.1 "San Diego" milestone Nov 28, 2020
@klizhentas klizhentas added the bug label Nov 28, 2020
@awly
Copy link
Contributor

awly commented Nov 30, 2020

@klizhentas should we backport this to 4.4/4.3/4.2?

@russjones russjones added the c-ha Internal Customer Reference label Dec 1, 2020
@awly
Copy link
Contributor

awly commented Dec 2, 2020

@russjones @andrejtokarcik let's try to fix this soon-ish (before 5.1 is fully ready)
Let me know if you'd like to sync to get more details.

@deusxanima
Copy link
Contributor

Will this fix apply only to tsh client and be supported across releases (so for example, a client running tsh client 4.2 w/ 5.1.2 root cluster)?

@awly
Copy link
Contributor

awly commented Feb 17, 2021

Yes, the fix will apply to tsh only.
It will work for any client version we support (at this point, probably 4.3/5/6) and server versions as old as 4.2 if not older.

@awly
Copy link
Contributor

awly commented Feb 17, 2021

so for example, a client running tsh client 4.2 w/ 5.1.2 root cluster

Users should not be using 4.2 clients against 5.1 servers. See https://goteleport.com/teleport/docs/admin-guide/#component-compatibility

@deusxanima deusxanima added the C0 label Mar 3, 2021
@russjones russjones modified the milestones: Runway Milestone, 7.0 Mar 10, 2021
@russjones russjones assigned tcsc and unassigned russjones and andrejtokarcik Mar 10, 2021
@russjones russjones modified the milestones: 7.0, 6.2 "Buffalo" Apr 15, 2021
@russjones russjones assigned awly and unassigned tcsc Apr 15, 2021
@travelton
Copy link
Contributor

Just to be clear, we are back porting this to 4.2, correct? We have a customer needing this in 4.2.

@awly
Copy link
Contributor

awly commented Apr 16, 2021

4.2 is no longer a supported version of Teleport (see https://goteleport.com/docs/faq/#which-version-of-teleport-is-supported)
4.4 is the oldest supported version for now, until v7 comes out.

@awly
Copy link
Contributor

awly commented Apr 20, 2021

Ugh, there's a wrinkle in implementing this: the k8s endpoint serving cert doesn't have the necessary SAN with a cluster name. So simply setting the same SNI value as we do when talking to the Auth API does not work (without disabling client-side TLS verification)

The "correct" way to solve this is to update the SANs we generate for k8s (and all other TLS endpoints) to contain the encoded cluster name. However, that's a bit of a migration. The clients can't set the SNI values until all servers get updated and rotate their serving certs. So backporting it will be a no-go.

Need to think through some other possibilities:

  • use a more clever TLS validation that doesn't rely on SNI to figure out the cert issuer
  • some hacky usage of an existing SAN to signal "i'm a user, only load the CA of the current cluster"
  • check proxy version via the Ping endpoint and set SNI if it's a version containing the fix
  • if >N CAs are loaded (like >100), only use the current cluster CA; this is most likely the root cluster so the calling user must have a cert from the root cluster

@awly
Copy link
Contributor

awly commented Apr 20, 2021

The "fix" is here #6519
Assuming this is approved, backporting all the way to 4.2 should be easy

@awly
Copy link
Contributor

awly commented May 5, 2021

All backports merged except 4.3 (pending some unrelated test failures).

@awly awly closed this as completed May 5, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

7 participants