Fix kubernetes_service nil ptr dereference

[lxea]

Based off of the stack trace in #9721 (havent successfuly reproduced the issue), it appears that the nil ptr dereference happens when creds.tlsConfig is nil.

#9721

[russjones]

@codingllama @jimbishopp Can you guys take a look?

[codingllama]

@codingllama @jimbishopp Can you guys take a look?

I have an open question above, but happy to approve if it's too much trouble.

[jimbishopp]

@russjones I was waiting for Alan's question to be answered as well. I was also concerned that it could not be reproduced.

[jimbishopp]

@lxea LMK if this is your understanding of this issue.

@codingllama This may answer the question.

It looks like the k8s lib is bypassing the TLS config under certain conditions resulting in the TLS config being set to nil.

k8s.io/client-go/transport/transport.go:60

 func TLSConfigFor(c *Config) (*tls.Config, error) {
	if !(c.HasCA() || c.HasCertAuth() || c.HasCertCallback() || c.TLS.Insecure || len(c.TLS.ServerName) > 0 || len(c.TLS.NextProtos) > 0) {
		return nil, nil
	}
...
}

We then panic while writing a debug log message:
kube/proxy/forwarder.go:1499

func (f *Forwarder) newClusterSessionLocal(ctx authContext) (*clusterSession, error) {
...
     f.log.Debugf("local Servername: %v", creds.tlsConfig.ServerName)

I don't know enough about our k8s implementation yet, but I assume by the way this is written that client auth is not required?

[lxea]

@lxea LMK if this is your understanding of this issue.
....

Yeah, this is my current understanding however I wasnt able to reproduce the error unfortunatly.

[russjones]

@r0mant What do you think? Should we merge this without a repro?

[r0mant]

It may be fine to merge without the repro but I'm not sure the current fix checks for nil in the best place. Looking at the code, it seems like tlsConfig is returned as nil in the extractNewCreds function from rest.TLSConfigFor:

https://github.com/gravitational/teleport/blob/v7.3.2/lib/kube/proxy/auth.go#L159

I think that would be a more appropriate place to check that tlsConfig is not nil (in addition to checking err).

As for the repro, I think getting a kubeconfig from the user experiencing this issue will help. Looking at rest.TLSConfigFor implementation (it's a method from K8s client lib) it returns "nil, nil" in a few specific cases based on the kubeconfig as @jimbishopp mentioned above.

[r0mant]

Also please add test coverage.

[r0mant]

@lxea I'm not sure this will help much in troubleshooting tbh. Can we dump the entire clientCfg in the log (or rather, it's TLS section)? But make sure to redact fields like CAData, CertData, KeyData.

[r0mant]

@codingllama @jimbishopp Can one of you give this another look please? We couldn't get the customer to share their kubeconfig with us so far so we want to merge this so it fixes the panic and logs the config (with sensitive fields redacted) which would help in troubleshooting if it happens again.

[codingllama]

Do we really need to echo the entire config? It seems a bit dangerous to do.

Maybe use rest.AnonymousClientConfig? Although I'd rather not echo everything if possible.