From a217f417ad952807d440914491cdfbbbe5ea647d Mon Sep 17 00:00:00 2001 From: Jina Jain Date: Thu, 21 Sep 2023 19:12:57 -0700 Subject: [PATCH 1/3] fix kubeclient tls verify setting --- internal/kubelet/client.go | 20 ++++++----- internal/kubelet/client_test.go | 35 ++++++++++++++++--- internal/kubelet/testdata/mismatch.crt | 29 +++++++++++++++ .../testdata/e2e/collector/configmap.yaml | 1 - 4 files changed, 70 insertions(+), 15 deletions(-) create mode 100644 internal/kubelet/testdata/mismatch.crt diff --git a/internal/kubelet/client.go b/internal/kubelet/client.go index 263d6d8f48bc..df16aa3b3c84 100644 --- a/internal/kubelet/client.go +++ b/internal/kubelet/client.go @@ -42,10 +42,11 @@ func NewClientProvider(endpoint string, cfg *ClientConfig, logger *zap.Logger) ( }, nil case k8sconfig.AuthTypeServiceAccount: return &saClientProvider{ - endpoint: endpoint, - caCertPath: svcAcctCACertPath, - tokenPath: svcAcctTokenPath, - logger: logger, + endpoint: endpoint, + caCertPath: svcAcctCACertPath, + tokenPath: svcAcctTokenPath, + insecureSkipVerify: cfg.InsecureSkipVerify, + logger: logger, }, nil case k8sconfig.AuthTypeNone: return &readOnlyClientProvider{ @@ -149,10 +150,11 @@ func (p *tlsClientProvider) BuildClient() (Client, error) { } type saClientProvider struct { - endpoint string - caCertPath string - tokenPath string - logger *zap.Logger + endpoint string + caCertPath string + tokenPath string + insecureSkipVerify bool + logger *zap.Logger } func (p *saClientProvider) BuildClient() (Client, error) { @@ -167,7 +169,7 @@ func (p *saClientProvider) BuildClient() (Client, error) { tr := defaultTransport() tr.TLSClientConfig = &tls.Config{ RootCAs: rootCAs, - InsecureSkipVerify: true, + InsecureSkipVerify: p.insecureSkipVerify, } endpoint, err := buildEndpoint(p.endpoint, true, p.logger) if err != nil { diff --git a/internal/kubelet/client_test.go b/internal/kubelet/client_test.go index 45e1eb1831a0..1e0ee7c5cc75 100644 --- a/internal/kubelet/client_test.go +++ b/internal/kubelet/client_test.go @@ -31,6 +31,7 @@ import ( const certPath = "./testdata/testcert.crt" const keyFile = "./testdata/testkey.key" +const errSignedByUnknownCA = "tls: failed to verify certificate: x509: certificate signed by unknown authority" func TestClient(t *testing.T) { tr := &fakeRoundTripper{} @@ -109,17 +110,18 @@ func TestSvcAcctClient(t *testing.T) { _, err := rw.Write([]byte(`OK`)) require.NoError(t, err) })) - cert, err := tls.LoadX509KeyPair("./testdata/testcert.crt", "./testdata/testkey.key") + cert, err := tls.LoadX509KeyPair(certPath, keyFile) require.NoError(t, err) server.TLS = &tls.Config{Certificates: []tls.Certificate{cert}} server.StartTLS() defer server.Close() p := &saClientProvider{ - endpoint: server.Listener.Addr().String(), - caCertPath: "./testdata/testcert.crt", - tokenPath: "./testdata/token", - logger: zap.NewNop(), + endpoint: server.Listener.Addr().String(), + caCertPath: certPath, + tokenPath: "./testdata/token", + insecureSkipVerify: false, + logger: zap.NewNop(), } client, err := p.BuildClient() require.NoError(t, err) @@ -128,6 +130,29 @@ func TestSvcAcctClient(t *testing.T) { require.Equal(t, []byte(`OK`), resp) } +func TestSAClientBadTLS(t *testing.T) { + server := httptest.NewUnstartedServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + _, _ = rw.Write([]byte(`OK`)) + })) + cert, err := tls.LoadX509KeyPair(certPath, keyFile) + require.NoError(t, err) + server.TLS = &tls.Config{Certificates: []tls.Certificate{cert}} + server.StartTLS() + defer server.Close() + + p := &saClientProvider{ + endpoint: server.Listener.Addr().String(), + caCertPath: "./testdata/mismatch.crt", + tokenPath: "./testdata/token", + insecureSkipVerify: false, + logger: zap.NewNop(), + } + client, err := p.BuildClient() + require.NoError(t, err) + _, err = client.Get("/") + require.ErrorContains(t, err, errSignedByUnknownCA) +} + func TestNewKubeConfigClient(t *testing.T) { server := httptest.NewUnstartedServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { // Check if call is authenticated using provided kubeconfig diff --git a/internal/kubelet/testdata/mismatch.crt b/internal/kubelet/testdata/mismatch.crt new file mode 100644 index 000000000000..4cbbe6362d6d --- /dev/null +++ b/internal/kubelet/testdata/mismatch.crt @@ -0,0 +1,29 @@ +-----BEGIN CERTIFICATE----- +MIIFCzCCAvOgAwIBAgIUSzAK6jTGoQ58bOM6sc3OV+9AxAwwDQYJKoZIhvcNAQEL +BQAwFTETMBEGA1UEAwwKdGVzdG9yZy5pbzAeFw0yMzA5MjIwMTM1MjNaFw0zMzA5 +MTkwMTM1MjNaMBUxEzARBgNVBAMMCnRlc3RvcmcuaW8wggIiMA0GCSqGSIb3DQEB +AQUAA4ICDwAwggIKAoICAQDd0W5AAcAqomUlVOqDOHEpHv089u9JXqd8MTu0EqDg +bZgddXrB52LTzsn6Kun2bDQtmEYvgamgfj1FkI7o2RmS+NPcXQBuBikbjbdZCMYQ +pQL9zqtdVmwWwhI9fAAcLWL59sPZNp065NVUBr5w2VByQDoMCYUNwmsW+jgvW+2+ +nPrluQoINo+1zG0W+PGLkcFnaBen3SzsFK0IQPh2YBBO3K5uIVWrpBn4HA4PE0CZ +aW4fZDX2rfzBNYMbypIbQRebgiG57AeUX7CL8PPGoYugKK+NnPz3P4MbeBGi13xa +xqVR1vX0GrKgr+uzzvIzzBk5Wr77GcpKgTnOwzXaeFTsy48rLmy96Guz70aHlhP+ +tnVHvGGypLrgmfifNigrUgG0ZqYRBQgeC8/FlocaAhrVl/RdS/WDprDAvUS/vUkz +OKeGviqD60cPyCHwznorQpf40OKJ54hVmfhXTkf4CvlHbWVf5laJ4hTIZmi4nB7+ +SwUUVYDmrVLp6BfO2QaODx2Fh2BUdx1pOpT6ve40DVrNnOOfvJCGJMjIwTeoP7EX +Kru75EevhuNjRNlWKhlygDyI66Vi0MBPNGCNjugWBoUYdBiuKN+SdZbocZViBXnC +GozCiqiZBjjIkb/i2+ntHYPCc3JajxNLtNTIvFB6gGQdhZXDht/CAw7jMLlVbTvm +LQIDAQABo1MwUTAdBgNVHQ4EFgQUmumL/KesQ0Rhf9XRo+DiF62aQdQwHwYDVR0j +BBgwFoAUmumL/KesQ0Rhf9XRo+DiF62aQdQwDwYDVR0TAQH/BAUwAwEB/zANBgkq +hkiG9w0BAQsFAAOCAgEArZ2QqQQiXYWOEQkPpN2Li2qMdzN4LagHckVGCSPGSt03 +49gxEjyzJBnDpQo81wq75GnVizMG5t4TuQ0/g9Xg41wEw4gU/rlYbPl1+aF4rDed +lGsjWm7neC28mMscGPVVl8jhHzW3AMnAXAxEjgbnHWzgep47CgjRhxr75G4n2fhz +E0gJdl/wbX355N9XXBM+5kKFzOypkiW3d5mCt4RGIdzxBE0si+51pT0LBTLc2jwz +vyBXp3adDkapoPrr7g16+0QbYHpU9/Odby3MbNqNHoDbaP54CDRhf+YvoMDu5Oqt +YgScuipaStevcOFTX1jUgBS74Tdsjm9YmheWZ81MEXVlTANuYQX2U0Xdjstnao2e +tJzJI5TXi9dRcNE1dKrZ4bA/kdHoIvjw6Wd+Q0PAIov83cNwJDfXr0YrHn9HOvDC +KGdYV1GLrkD51g4roHYJqxNMw9PXfjZv7rveYYajusOlMZh1nP1uEU0vTvZ8HNIA +Wops66yZiahgtMN9xI0Ingg5AIIwin6B8rExtoOi3NLUMc3ZEpgnHK+z406VOlVQ +IHAp6ZL0GqGX86EHlvU5Cf7l9TnzIHSbWZvdNUkkva1V4hvhVVrnxoaNy+ifuCEF +XHXxPDcyLN7LxymgRpximDlupMHhlpB4sUTM3pdb0UKOqSSIbWavW2YKf2Ei8+g= +-----END CERTIFICATE----- diff --git a/receiver/kubeletstatsreceiver/testdata/e2e/collector/configmap.yaml b/receiver/kubeletstatsreceiver/testdata/e2e/collector/configmap.yaml index 039e5908faa3..e2f7acc8d665 100644 --- a/receiver/kubeletstatsreceiver/testdata/e2e/collector/configmap.yaml +++ b/receiver/kubeletstatsreceiver/testdata/e2e/collector/configmap.yaml @@ -16,7 +16,6 @@ data: receivers: kubeletstats: auth_type: "serviceAccount" - insecure_skip_verify: true endpoint: "https://${env:K8S_NODE_NAME}:10250" metric_groups: - node From 74f814ec9413158d8cccfe21bfc3a9e16ca07950 Mon Sep 17 00:00:00 2001 From: Jina Jain Date: Thu, 21 Sep 2023 19:30:46 -0700 Subject: [PATCH 2/3] add changelog --- .chloggen/26319-kubeletclient-tls-verify.yaml | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 .chloggen/26319-kubeletclient-tls-verify.yaml diff --git a/.chloggen/26319-kubeletclient-tls-verify.yaml b/.chloggen/26319-kubeletclient-tls-verify.yaml new file mode 100644 index 000000000000..0cf1e77388b8 --- /dev/null +++ b/.chloggen/26319-kubeletclient-tls-verify.yaml @@ -0,0 +1,29 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: receiver/kubeletstats + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fixes a bug where the "insecure_skip_verify" config was not being honored when auth_type is "serviceAccount" in kubelet client. + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [26319] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + Before the fix, the kubelet client was not verifying kubelet's certificate. The default value of the config is false, + so with the fix the client will start verifying tls cert unless the config is set to true. + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [user] From bb667c0891b1a6ce40a7aa5fcaeb665a334b8fbe Mon Sep 17 00:00:00 2001 From: Jina Jain Date: Fri, 22 Sep 2023 14:55:08 -0700 Subject: [PATCH 3/3] update changelog --- .chloggen/26319-kubeletclient-tls-verify.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.chloggen/26319-kubeletclient-tls-verify.yaml b/.chloggen/26319-kubeletclient-tls-verify.yaml index 0cf1e77388b8..dc5e92bf1dce 100644 --- a/.chloggen/26319-kubeletclient-tls-verify.yaml +++ b/.chloggen/26319-kubeletclient-tls-verify.yaml @@ -1,13 +1,13 @@ # Use this changelog template to create an entry for release notes. # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: bug_fix +change_type: breaking # The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) component: receiver/kubeletstats # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Fixes a bug where the "insecure_skip_verify" config was not being honored when auth_type is "serviceAccount" in kubelet client. +note: Fixes a bug where the "insecure_skip_verify" config was not being honored when "auth_type" is "serviceAccount" in kubelet client. # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. issues: [26319] @@ -17,7 +17,7 @@ issues: [26319] # Use pipe (|) for multiline entries. subtext: | Before the fix, the kubelet client was not verifying kubelet's certificate. The default value of the config is false, - so with the fix the client will start verifying tls cert unless the config is set to true. + so with the fix the client will start verifying tls cert unless the config is explicitly set to true. # If your change doesn't affect end users or the exported elements of any package, # you should instead start your pull request title with [chore] or use the "Skip Changelog" label.