Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[receiver/kubeletstats][internal/kubelet] Fix tls config for service account auth #27070

Merged
merged 5 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions .chloggen/26319-kubeletclient-tls-verify.yaml
Original file line number Diff line number Diff line change
@@ -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: 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.

# 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 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.
# 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]
20 changes: 11 additions & 9 deletions internal/kubelet/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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) {
Expand All @@ -167,7 +169,7 @@ func (p *saClientProvider) BuildClient() (Client, error) {
tr := defaultTransport()
tr.TLSClientConfig = &tls.Config{
RootCAs: rootCAs,
InsecureSkipVerify: true,
InsecureSkipVerify: p.insecureSkipVerify,
TylerHelmuth marked this conversation as resolved.
Show resolved Hide resolved
}
endpoint, err := buildEndpoint(p.endpoint, true, p.logger)
if err != nil {
Expand Down
35 changes: 30 additions & 5 deletions internal/kubelet/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
29 changes: 29 additions & 0 deletions internal/kubelet/testdata/mismatch.crt
Original file line number Diff line number Diff line change
@@ -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-----
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ data:
receivers:
kubeletstats:
auth_type: "serviceAccount"
insecure_skip_verify: true
endpoint: "https://${env:K8S_NODE_NAME}:10250"
metric_groups:
- node
Expand Down