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

K8SPSMDB-813: Fail TLS configuration if provided certificates do not exist #1254

Merged
merged 14 commits into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from 13 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
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@ spec:
- /opt/percona/mongodb-healthcheck
- k8s
- liveness
- --ssl
- --sslInsecure
- --sslCAFile
- /etc/mongodb-ssl/ca.crt
- --sslPEMKeyFile
- /tmp/tls.pem
- --startupDelaySeconds
- "7200"
failureThreshold: 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@ spec:
- /opt/percona/mongodb-healthcheck
- k8s
- liveness
- --ssl
- --sslInsecure
- --sslCAFile
- /etc/mongodb-ssl/ca.crt
- --sslPEMKeyFile
- /tmp/tls.pem
- --startupDelaySeconds
- "7200"
failureThreshold: 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@ spec:
- /opt/percona/mongodb-healthcheck
- k8s
- liveness
- --ssl
- --sslInsecure
- --sslCAFile
- /etc/mongodb-ssl/ca.crt
- --sslPEMKeyFile
- /tmp/tls.pem
- --startupDelaySeconds
- "7200"
failureThreshold: 4
Expand Down
6 changes: 0 additions & 6 deletions e2e-tests/one-pod/compare/statefulset_one-pod-rs0-oc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ spec:
- /opt/percona/mongodb-healthcheck
- k8s
- liveness
- --ssl
- --sslInsecure
- --sslCAFile
- /etc/mongodb-ssl/ca.crt
- --sslPEMKeyFile
- /tmp/tls.pem
- --startupDelaySeconds
- "7200"
failureThreshold: 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ spec:
- /opt/percona/mongodb-healthcheck
- k8s
- liveness
- --ssl
- --sslInsecure
- --sslCAFile
- /etc/mongodb-ssl/ca.crt
- --sslPEMKeyFile
- /tmp/tls.pem
- --startupDelaySeconds
- "7200"
failureThreshold: 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ spec:
- /opt/percona/mongodb-healthcheck
- k8s
- liveness
- --ssl
- --sslInsecure
- --sslCAFile
- /etc/mongodb-ssl/ca.crt
- --sslPEMKeyFile
- /tmp/tls.pem
- --startupDelaySeconds
- "7200"
failureThreshold: 4
Expand Down
6 changes: 0 additions & 6 deletions e2e-tests/one-pod/compare/statefulset_one-pod-rs0.yml
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@ spec:
- /opt/percona/mongodb-healthcheck
- k8s
- liveness
- --ssl
- --sslInsecure
- --sslCAFile
- /etc/mongodb-ssl/ca.crt
- --sslPEMKeyFile
- /tmp/tls.pem
- --startupDelaySeconds
- "7200"
failureThreshold: 4
Expand Down
63 changes: 28 additions & 35 deletions healthcheck/tools/db/ssl.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,56 +48,49 @@ func LastSSLError() error {
}

func (cnf *Config) configureTLS() error {
config := &tls.Config{
InsecureSkipVerify: cnf.SSL.Insecure,
}

if len(cnf.SSL.PEMKeyFile) == 0 || len(cnf.SSL.CAFile) == 0 {
if !cnf.SSL.Enabled {
return nil
}

pemOk, err := isFileExists(cnf.SSL.PEMKeyFile)
if err != nil {
return errors.Wrapf(err, "check if file with name %s exists", cnf.SSL.PEMKeyFile)
}

caOk, err := isFileExists(cnf.SSL.CAFile)
if err != nil {
return errors.Wrapf(err, "check if file with name %s exists", cnf.SSL.CAFile)
config := &tls.Config{
InsecureSkipVerify: cnf.SSL.Insecure,
}

if !pemOk || !caOk {
cnf.SSL = nil
return nil
}
// Configure client cert
if len(cnf.SSL.PEMKeyFile) != 0 {
if err := isFileExists(cnf.SSL.PEMKeyFile); err != nil {
return errors.Wrapf(err, "check if file with name %s exists", cnf.SSL.PEMKeyFile)
}

log.Debugf("Loading SSL/TLS PEM certificate: %s", cnf.SSL.PEMKeyFile)
log.Debugf("Loading SSL/TLS PEM certificate: %s", cnf.SSL.PEMKeyFile)
certificates, err := tls.LoadX509KeyPair(cnf.SSL.PEMKeyFile, cnf.SSL.PEMKeyFile)
if err != nil {
return errors.Wrapf(err, "load key pair from '%s' to connect to server '%s'", cnf.SSL.PEMKeyFile, cnf.Hosts)
}

certificates, err := tls.LoadX509KeyPair(cnf.SSL.PEMKeyFile, cnf.SSL.PEMKeyFile)
if err != nil {
return errors.Wrapf(err, "load key pair from '%s' to connect to server '%s'", cnf.SSL.PEMKeyFile, cnf.Hosts)
config.Certificates = []tls.Certificate{certificates}
}

config.Certificates = []tls.Certificate{certificates}
// Configure CA cert
if len(cnf.SSL.CAFile) != 0 {
if err := isFileExists(cnf.SSL.CAFile); err != nil {
return errors.Wrapf(err, "check if file with name %s exists", cnf.SSL.CAFile)
}

log.Debugf("Loading SSL/TLS Certificate Authority: %s", cnf.SSL.CAFile)
ca, err := cnf.SSL.loadCaCertificate()
if err != nil {
return errors.Wrapf(err, "load client CAs from %s", cnf.SSL.CAFile)
log.Debugf("Loading SSL/TLS Certificate Authority: %s", cnf.SSL.CAFile)
ca, err := cnf.SSL.loadCaCertificate()
if err != nil {
return errors.Wrapf(err, "load client CAs from %s", cnf.SSL.CAFile)
}

config.RootCAs = ca
}

config.RootCAs = ca
cnf.TLSConf = config

return nil
}

func isFileExists(name string) (bool, error) {
func isFileExists(name string) error {
_, err := os.Stat(name)
if os.IsNotExist(err) {
return false, nil
} else if err != nil {
return false, err
}
return true, nil
return err
}
86 changes: 86 additions & 0 deletions healthcheck/tools/db/ssl_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package db

import (
"fmt"
"testing"
)

const (
notExistingFilePath = "not-existing-file-path"
)

func TestSSLNotEnabled(t *testing.T) {
cfg := &Config{
SSL: &SSLConfig{
Enabled: false,
},
}

if err := cfg.configureTLS(); err != nil {
t.Fatalf("TLS configuration failed: %s", err)
}

if cfg.TLSConf != nil {
t.Error("Expected TLSConf to be nil")
}
}

func TestSSLEnabled(t *testing.T) {
cfg := &Config{
SSL: &SSLConfig{
Enabled: true,
},
}

if err := cfg.configureTLS(); err != nil {
t.Fatalf("TLS configuration failed: %s", err)
}

if cfg.TLSConf == nil {
t.Error("Expected TLSConf to not be nil")
}
}

func TestPEMKeyFileDoesNotExists(t *testing.T) {
cfg := &Config{
SSL: &SSLConfig{
Enabled: true,
PEMKeyFile: notExistingFilePath,
},
}

err := cfg.configureTLS()
if err == nil {
t.Fatal("Expected TLS config to fail, but it returned no error")
}

expectedErrorMessage := fmt.Sprintf(
"check if file with name %s exists: stat %s: no such file or directory",
notExistingFilePath, notExistingFilePath,
)
if err.Error() != expectedErrorMessage {
t.Errorf("error message '%s' does not match expected '%s'", err.Error(), expectedErrorMessage)
}
}

func TestCAFileDoesNotExists(t *testing.T) {
cfg := &Config{
SSL: &SSLConfig{
Enabled: true,
CAFile: notExistingFilePath,
},
}

err := cfg.configureTLS()
if err == nil {
t.Fatal("Expected TLS config to fail, but it returned no error")
}

expectedErrorMessage := fmt.Sprintf(
"check if file with name %s exists: stat %s: no such file or directory",
notExistingFilePath, notExistingFilePath,
)
if err.Error() != expectedErrorMessage {
t.Errorf("error message '%s' does not match expected '%s'", err.Error(), expectedErrorMessage)
}
}
25 changes: 14 additions & 11 deletions pkg/apis/psmdb/v1/psmdb_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,8 @@ func (cr *PerconaServerMongoDB) CheckNSetDefaults(platform version.Platform, log
},
}

if cr.CompareVersion("1.7.0") >= 0 {
if (cr.CompareVersion("1.7.0") >= 0 && cr.CompareVersion("1.15.0") < 0) ||
cr.CompareVersion("1.15.0") >= 0 && !cr.Spec.UnsafeConf {
cr.Spec.Sharding.Mongos.LivenessProbe.Exec.Command =
append(cr.Spec.Sharding.Mongos.LivenessProbe.Exec.Command,
"--ssl", "--sslInsecure",
Expand Down Expand Up @@ -236,7 +237,8 @@ func (cr *PerconaServerMongoDB) CheckNSetDefaults(platform version.Platform, log
},
}

if cr.CompareVersion("1.7.0") >= 0 {
if (cr.CompareVersion("1.7.0") >= 0 && cr.CompareVersion("1.15.0") < 0) ||
cr.CompareVersion("1.15.0") >= 0 && !cr.Spec.UnsafeConf {
Comment on lines +240 to +241
Copy link
Contributor

Choose a reason for hiding this comment

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

we seem to changed the behavior, with these we'll add these flags to probe command for all clusters <1.15

Copy link
Contributor

Choose a reason for hiding this comment

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

We should check for unsafe option only for clusters with >=1.15.0 version to maintain the old behavior for older cluster versions.

It seems that you checked the diff of only my changes, and not those of the entire pull request.

cr.Spec.Sharding.Mongos.ReadinessProbe.Exec.Command =
append(cr.Spec.Sharding.Mongos.ReadinessProbe.Exec.Command,
"--ssl", "--sslInsecure",
Expand Down Expand Up @@ -362,7 +364,8 @@ func (cr *PerconaServerMongoDB) CheckNSetDefaults(platform version.Platform, log

if cr.CompareVersion("1.6.0") >= 0 {
replset.LivenessProbe.Probe.Exec.Command[0] = "/data/db/mongodb-healthcheck"
if cr.CompareVersion("1.7.0") >= 0 {
if (cr.CompareVersion("1.7.0") >= 0 && cr.CompareVersion("1.15.0") < 0) ||
cr.CompareVersion("1.15.0") >= 0 && !cr.Spec.UnsafeConf {
replset.LivenessProbe.Probe.Exec.Command =
append(replset.LivenessProbe.Probe.Exec.Command,
"--ssl", "--sslInsecure",
Expand Down Expand Up @@ -643,14 +646,14 @@ func (nv *NonVotingSpec) SetDefaults(cr *PerconaServerMongoDB, rs *ReplsetSpec)
}
if nv.LivenessProbe.ProbeHandler.Exec == nil {
nv.LivenessProbe.Probe.ProbeHandler.Exec = &corev1.ExecAction{
Command: []string{
"/data/db/mongodb-healthcheck",
"k8s",
"liveness",
"--ssl", "--sslInsecure",
"--sslCAFile", "/etc/mongodb-ssl/ca.crt",
"--sslPEMKeyFile", "/tmp/tls.pem",
},
Command: []string{"/data/db/mongodb-healthcheck", "k8s", "liveness"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

@egegunes, maybe we need to do it only for cr >= 1.15?

}

if !cr.Spec.UnsafeConf || cr.CompareVersion("1.15.0") < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it correct? before we were adding ssl flags if UnsafeConf is false but with these changes we'll add flags if crVersion is < 1.15 no matter unsafe flag value

Copy link
Contributor

Choose a reason for hiding this comment

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

Before the PR, we had this code without any checks:

 Command: []string{
				"/data/db/mongodb-healthcheck",
				"k8s",
				"liveness",
				"--ssl", "--sslInsecure",
				"--sslCAFile", "/etc/mongodb-ssl/ca.crt",
				"--sslPEMKeyFile", "/tmp/tls.pem",
			},

We should add these flags to crs with < 1.15.0 versions to maintain the old behavior

nv.LivenessProbe.Probe.ProbeHandler.Exec.Command = append(
nv.LivenessProbe.Probe.ProbeHandler.Exec.Command,
"--ssl", "--sslInsecure", "--sslCAFile", "/etc/mongodb-ssl/ca.crt", "--sslPEMKeyFile", "/tmp/tls.pem",
)
}

if cr.CompareVersion("1.14.0") >= 0 {
Expand Down
Loading