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

Remove cert and pem enforcement #150

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
15 changes: 8 additions & 7 deletions path_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ func pathConfig(b *kubeAuthBackend) *framework.Path {

"kubernetes_ca_cert": {
Type: framework.TypeString,
Description: "PEM encoded CA cert for use by the TLS client used to talk with the API.",
Description: "Optional PEM encoded CA cert for use by the TLS client used to talk with the API.",
DisplayAttrs: &framework.DisplayAttributes{
Name: "Kubernetes CA Certificate",
},
Required: false,
imthaghost marked this conversation as resolved.
Show resolved Hide resolved
},
"token_reviewer_jwt": {
Type: framework.TypeString,
Expand All @@ -56,6 +57,7 @@ extracted. Not every installation of Kubernetes exposes these keys.`,
DisplayAttrs: &framework.DisplayAttributes{
Name: "Service account verification keys",
},
Required: false,
},
"issuer": {
Type: framework.TypeString,
Expand Down Expand Up @@ -141,10 +143,6 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ
}
}

if disableLocalJWT && caCert == "" {
return logical.ErrorResponse("kubernetes_ca_cert must be given when disable_local_ca_jwt is true"), nil
}

config := &kubeConfig{
PublicKeys: make([]interface{}, len(pemList)),
PEMKeys: pemList,
Expand All @@ -161,11 +159,11 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ

// Determine if we load the local CA cert or the CA cert provided
// by the kubernetes_ca_cert path into the backend's HTTP client
certPool := x509.NewCertPool()
tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
}
if disableLocalJWT || len(caCert) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we don't use system certs if disableLocalJWT is true?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a more idiomatic check for empty strings:

Suggested change
if disableLocalJWT || len(caCert) > 0 {
if disableLocalJWT || caCert != "" {

certPool := x509.NewCertPool()
certPool.AppendCertsFromPEM([]byte(config.CACert))
tlsConfig.RootCAs = certPool

Expand All @@ -175,7 +173,10 @@ func (b *kubeAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Requ
if err != nil {
return nil, err
}

certPool, err := x509.SystemCertPool()
if err != nil {
certPool = x509.NewCertPool()
}
certPool.AppendCertsFromPEM([]byte(localCACert))
tlsConfig.RootCAs = certPool

Expand Down
59 changes: 59 additions & 0 deletions path_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,65 @@ func TestConfig_LocalJWTRenewal(t *testing.T) {
}
}

func TestConfig_SystemCa(t *testing.T) {
testCases := map[string]struct {
config map[string]interface{}
setupInClusterFiles bool
expected *kubeConfig
}{
"no CA default to system": {
config: map[string]interface{}{
"kubernetes_host": "host",
"disable_local_ca_jwt": false,
"kubernetes_ca_cert": "",
},
setupInClusterFiles: true,

expected: &kubeConfig{
PublicKeys: []interface{}{},
PEMKeys: []string{},
Host: "host",
CACert: testLocalCACert,
TokenReviewerJWT: testLocalJWT,
DisableISSValidation: true,
DisableLocalCAJwt: false,
},
},
Comment on lines +544 to +561
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test different than "no CA or JWT, default to local" in TestConfig_LocalCaJWT()? IIRC disable_local_ca_jwt defaults to false, and kubernetes_ca_cert defaults to "".

"no CA or JWT, default to local": {
config: map[string]interface{}{
"kubernetes_host": "host",
},
setupInClusterFiles: true,
expected: &kubeConfig{
PublicKeys: []interface{}{},
PEMKeys: []string{},
Host: "host",
CACert: testLocalCACert,
TokenReviewerJWT: testLocalJWT,
DisableISSValidation: true,
DisableLocalCAJwt: false,
},
},

}

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
b, storage := getBackend(t)

if tc.setupInClusterFiles {
cleanup := setupLocalFiles(t, b)
defer cleanup()
}

req := &logical.Request{
Operation: logical.CreateOperation,
Path: configPath,
Storage: storage,
Data: tc.config,
}

resp, err := b.HandleRequest(context.Background(), req)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%s resp:%#v\n", err, resp)
}

conf, err := b.(*kubeAuthBackend).loadConfig(context.Background(), storage)
if err != nil {
t.Fatal(err)
}

if !reflect.DeepEqual(tc.expected, conf) {
t.Fatalf("expected did not match actual: expected %#v\n got %#v\n", tc.expected, conf)
}
})
}
}

var testLocalCACert string = `-----BEGIN CERTIFICATE-----
MIIDVDCCAjwCCQDFiyFY1M6afTANBgkqhkiG9w0BAQsFADBsMQswCQYDVQQGEwJV
UzETMBEGA1UECAwKV2FzaGluZ3RvbjEQMA4GA1UEBwwHU2VhdHRsZTEgMB4GA1UE
Expand Down