Skip to content

Commit

Permalink
Revert "Avoid nil dereferencing when tlsConfig is nil. (#9788)"
Browse files Browse the repository at this point in the history
This reverts commit 4a42cae.
  • Loading branch information
tigrato committed May 25, 2022
1 parent 13b501c commit 953e1c8
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 87 deletions.
11 changes: 4 additions & 7 deletions lib/kube/proxy/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,17 +170,14 @@ func extractKubeCreds(ctx context.Context, cluster string, clientCfg *rest.Confi
if err != nil {
return nil, trace.Wrap(err)
}

// tlsConfig can be nil and still no error is returned.
// This happens when no `certificate-authority-data` is provided in kubeconfig because one is expected to use
// the the system default CA pool.
tlsConfig, err := rest.TLSConfigFor(clientCfg)
if err != nil {
return nil, trace.Wrap(err, "failed to generate TLS config from kubeconfig: %v", err)
}
if tlsConfig == nil {
cc := rest.AnonymousClientConfig(clientCfg)
if len(cc.CAData) != 0 {
cc.CAData = []byte("REDACTED")
}
return nil, trace.BadParameter("failed to generate TLS config from kubeConfig. clientConfig: %s", cc.String())
}
transportConfig, err := clientCfg.TransportConfig()
if err != nil {
return nil, trace.Wrap(err, "failed to generate transport config from kubeconfig: %v", err)
Expand Down
86 changes: 8 additions & 78 deletions lib/kube/proxy/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,9 @@ limitations under the License.
package proxy

import (
"bufio"
"bytes"
"context"
"crypto/tls"
"errors"
"fmt"
"io/fs"
"os"
"path/filepath"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -135,49 +128,12 @@ func failsForCluster(clusterName string) ImpersonationPermissionsChecker {
func TestGetKubeCreds(t *testing.T) {
ctx := context.TODO()
const teleClusterName = "teleport-cluster"
testDir := t.TempDir()

cert, err := utils.GenerateSelfSignedCert([]string{"localhost"})
tmpFile, err := os.CreateTemp("", "teleport")
require.NoError(t, err)

certFilePath := filepath.Join(testDir, "certfile")
require.NoError(t, os.WriteFile(certFilePath, cert.Cert, fs.ModePerm))
keyFilePath := filepath.Join(testDir, "keyfile")
require.NoError(t, os.WriteFile(keyFilePath, cert.PrivateKey, fs.ModePerm))
tlsConfig, err := utils.CreateTLSConfiguration(certFilePath, keyFilePath, utils.DefaultCipherSuites())
require.NoError(t, err)

kubeconfigPath := filepath.Join(testDir, "kubeconf")
require.NoError(t, os.WriteFile(kubeconfigPath, []byte(fmt.Sprintf(`
apiVersion: v1
kind: Config
clusters:
- cluster:
server: https://example.com:3026
name: example
contexts:
- context:
cluster: example
user: creds
name: foo
- context:
cluster: example
user: creds
name: bar
- context:
cluster: example
user: creds
name: baz
users:
- name: creds
user:
client-certificate: %s
client-key: %s
current-context: foo
`, certFilePath, keyFilePath)), fs.ModePerm))

kubeconfigbadPath := filepath.Join(testDir, "kubeconfbad")
require.NoError(t, os.WriteFile(kubeconfigbadPath, []byte(`
defer os.Remove(tmpFile.Name())
kubeconfigPath := tmpFile.Name()
_, err = tmpFile.Write([]byte(`
apiVersion: v1
kind: Config
clusters:
Expand All @@ -200,12 +156,9 @@ contexts:
users:
- name: creds
current-context: foo
`), fs.ModePerm))

logger := utils.NewLoggerForTests()
buf := bytes.NewBuffer([]byte{})
logger.SetOutput(buf)
sc := bufio.NewScanner(buf)
`))
require.NoError(t, err)
require.NoError(t, tmpFile.Close())

tests := []struct {
desc string
Expand Down Expand Up @@ -240,19 +193,16 @@ current-context: foo
impersonationCheck: alwaysSucceeds,
want: map[string]*kubeCreds{
"foo": {
tlsConfig: tlsConfig,
targetAddr: "example.com:3026",
transportConfig: &transport.Config{},
kubeClient: &kubernetes.Clientset{},
},
"bar": {
tlsConfig: tlsConfig,
targetAddr: "example.com:3026",
transportConfig: &transport.Config{},
kubeClient: &kubernetes.Clientset{},
},
"baz": {
tlsConfig: tlsConfig,
targetAddr: "example.com:3026",
transportConfig: &transport.Config{},
kubeClient: &kubernetes.Clientset{},
Expand All @@ -273,7 +223,6 @@ current-context: foo
impersonationCheck: alwaysSucceeds,
want: map[string]*kubeCreds{
teleClusterName: {
tlsConfig: tlsConfig,
targetAddr: "example.com:3026",
transportConfig: &transport.Config{},
kubeClient: &kubernetes.Clientset{},
Expand All @@ -287,45 +236,27 @@ current-context: foo
impersonationCheck: failsForCluster("bar"),
want: map[string]*kubeCreds{
"foo": {
tlsConfig: tlsConfig,
targetAddr: "example.com:3026",
transportConfig: &transport.Config{},
kubeClient: &kubernetes.Clientset{},
},
"bar": {
tlsConfig: tlsConfig,
targetAddr: "example.com:3026",
transportConfig: &transport.Config{},
kubeClient: &kubernetes.Clientset{},
},
"baz": {
tlsConfig: tlsConfig,
targetAddr: "example.com:3026",
transportConfig: &transport.Config{},
kubeClient: &kubernetes.Clientset{},
},
},
assertErr: require.NoError,
}, {
desc: "kubernetes_service, bad kube creds",
serviceType: KubeService,
kubeconfigPath: kubeconfigbadPath,
impersonationCheck: alwaysSucceeds,
assertErr: func(tt require.TestingT, err error, i ...interface{}) {
findErr := "failed to generate TLS config from kubeConfig. clientConfig"
for sc.Scan() {
if strings.Contains(sc.Text(), findErr) {
return
}
}
t.Fatalf("Failed to find error %q in the logs", findErr)
},
want: map[string]*kubeCreds{},
},
}
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
got, err := getKubeCreds(ctx, logger, teleClusterName, "", tt.kubeconfigPath, tt.serviceType, tt.impersonationCheck)
got, err := getKubeCreds(ctx, utils.NewLoggerForTests(), teleClusterName, "", tt.kubeconfigPath, tt.serviceType, tt.impersonationCheck)
tt.assertErr(t, err)
if err != nil {
return
Expand All @@ -334,7 +265,6 @@ current-context: foo
cmp.AllowUnexported(kubeCreds{}),
cmp.Comparer(func(a, b *transport.Config) bool { return (a == nil) == (b == nil) }),
cmp.Comparer(func(a, b *kubernetes.Clientset) bool { return (a == nil) == (b == nil) }),
cmp.Comparer(func(a, b *tls.Config) bool { return (a == nil) == (b == nil) }),
))
})
}
Expand Down
5 changes: 3 additions & 2 deletions lib/kube/proxy/forwarder.go
Original file line number Diff line number Diff line change
Expand Up @@ -1579,7 +1579,6 @@ func (f *Forwarder) newClusterSessionLocal(ctx authContext) (*clusterSession, er
if !ok {
return nil, trace.NotFound("kubernetes cluster %q not found", ctx.kubeCluster)
}
f.log.Debugf("local Servername: %v", creds.tlsConfig.ServerName)

f.log.Debugf("Handling kubernetes session for %v using local credentials.", ctx)
return &clusterSession{
Expand Down Expand Up @@ -1630,7 +1629,9 @@ func (f *Forwarder) makeSessionForwarder(sess *clusterSession) (*forward.Forward
if err := http2.ConfigureTransport(transport); err != nil {
return nil, trace.Wrap(err)
}
} else {
} else if sess.tlsConfig != nil {
// when certificate-authority-data is not provided in kubeconfig the tlsConfig can be nil,
// meaning that we will use the system default CA store.
sess.tlsConfig.NextProtos = nil
}

Expand Down

0 comments on commit 953e1c8

Please sign in to comment.