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.

Sign up for GitHub

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

Fix validation of references to CA certificate in TLS config #1119

Merged
merged 5 commits into from
Oct 21, 2022
Merged

Fix validation of references to CA certificate in TLS config #1119

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6a973b
Fix validation of references to CA certificate in TLS config
adamliesko Sep 17, 2022
eb8711f
convert tests to table driven
adamliesko Oct 3, 2022
883995a
Merge branch 'master' of https://github.com/mongodb/mongodb-kubernete…
adamliesko Oct 5, 2022
ad7e21e
Fix incorrect test fixtures without CAcert values
adamliesko Oct 5, 2022
a4d6508
reuse mgr in reconciler
adamliesko Oct 5, 2022
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
7 changes: 6 additions & 1 deletion controllers/mongodb_tls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,14 +162,19 @@ func getCaCrt(cmGetter configmap.Getter, secretGetter secret.Getter, mdb mdbv1.M
if mdb.Spec.Security.TLS.CaCertificateSecret != nil {
caResourceName = mdb.TLSCaCertificateSecretNamespacedName()
caData, err = secret.ReadStringData(secretGetter, caResourceName)
} else {
} else if mdb.Spec.Security.TLS.CaConfigMap != nil {
caResourceName = mdb.TLSConfigMapNamespacedName()
caData, err = configmap.ReadData(cmGetter, caResourceName)
}

if err != nil {
return "", err
}

if caData == nil {
return "", fmt.Errorf("TLS field requires a reference to the CA certificate which signed the server certificates. Neither secret (field caCertificateSecretRef) not configMap (field CaConfigMap) reference present")
}

if cert, ok := caData[tlsCACertName]; !ok || cert == "" {
return "", fmt.Errorf(`CA certificate resource "%s" should have a CA certificate in field "%s"`, caResourceName, tlsCACertName)
} else {
Expand Down
62 changes: 62 additions & 0 deletions controllers/mongodb_tls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,69 @@ func TestPemSupport(t *testing.T) {
err = r.ensureTLSResources(mdb)
assert.Error(t, err)
assert.Contains(t, err.Error(), `if all of "tls.crt", "tls.key" and "tls.pem" are present in the secret, the entry for "tls.pem" must be equal to the concatenation of "tls.crt" with "tls.key"`)
})
}

func TestTLSConfig_ReferencesToCACertAreValidated(t *testing.T) {
t.Run("Success if reference to CA cert provided via secret", func(t *testing.T) {
mdb := newTestReplicaSetWithTLSCaCertificateReferences(&mdbv1.LocalObjectReference{
Name: "certificateKeySecret"}, nil)

mgr := kubeClient.NewManager(&mdb)
cli := mdbClient.NewClient(mgr.GetClient())
err := createTLSSecret(cli, mdb, "CERT", "KEY", "")

assert.NoError(t, err)

r := NewReconciler(mgr)

_, err = r.validateTLSConfig(mdb)
assert.Nil(t, err)
})
t.Run("Success if reference to CA cert provided via config map", func(t *testing.T) {
mdb := newTestReplicaSetWithTLSCaCertificateReferences(nil, &mdbv1.LocalObjectReference{
Name: "caConfigMap"})

mgr := kubeClient.NewManager(&mdb)
cli := mdbClient.NewClient(mgr.GetClient())
err := createTLSSecret(cli, mdb, "CERT", "KEY", "")

assert.NoError(t, err)

r := NewReconciler(mgr)

_, err = r.validateTLSConfig(mdb)
assert.Nil(t, err)
})
t.Run("Succes if reference to CA cert provided both via secret and configMap", func(t *testing.T) {
mdb := newTestReplicaSetWithTLSCaCertificateReferences(&mdbv1.LocalObjectReference{
Name: "certificateKeySecret"}, &mdbv1.LocalObjectReference{
Name: "caConfigMap"})
mgr := kubeClient.NewManager(&mdb)
cli := mdbClient.NewClient(mgr.GetClient())
err := createTLSSecret(cli, mdb, "CERT", "KEY", "")

assert.NoError(t, err)

r := NewReconciler(mgr)

_, err = r.validateTLSConfig(mdb)
assert.Nil(t, err)
})
t.Run("Failure if reference to CA cert is missing", func(t *testing.T) {
mdb := newTestReplicaSetWithTLSCaCertificateReferences(nil, nil)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a lot of code duplication within this test. Can I ask you to re-implement it to the parameters-driven test, like https://github.com/mongodb/mongodb-kubernetes-operator/blob/master/test/e2e/replica_set_arbiter/replica_set_arbiter_test.go#L28 ?

Copy link
Contributor Author

@adamliesko adamliesko Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes sure. I initially started it as a table-driven test, but skimming through the codebase it didn't seem to me like a prevalent pattern.

mgr := kubeClient.NewManager(&mdb)
cli := mdbClient.NewClient(mgr.GetClient())
err := createTLSSecret(cli, mdb, "CERT", "KEY", "")

assert.NoError(t, err)

r := NewReconciler(mgr)

_, err = r.validateTLSConfig(mdb)
assert.Error(t, err)
assert.Contains(t, err.Error(), "TLS field requires a reference to the CA certificate which signed the server certificates. Neither secret (field caCertificateSecretRef) not configMap (field CaConfigMap) reference present")
})
}

Expand Down
1 change: 0 additions & 1 deletion controllers/replica_set_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ func NewReconciler(mgr manager.Manager) *ReplicaSetReconciler {
mgrClient := mgr.GetClient()
secretWatcher := watch.New()
configMapWatcher := watch.New()

return &ReplicaSetReconciler{
client: kubernetesClient.NewClient(mgrClient),
scheme: mgr.GetScheme(),
Expand Down
16 changes: 12 additions & 4 deletions controllers/replicaset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,15 @@ func newScramReplicaSet(users ...mdbv1.MongoDBUser) mdbv1.MongoDBCommunity {
}

func newTestReplicaSetWithTLS() mdbv1.MongoDBCommunity {
return newTestReplicaSetWithTLSCaCertificateReferences(&mdbv1.LocalObjectReference{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please help me understand what this change bring to the table? I'm not sure if it changes anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It allows us to create test fixtures, where the caller passes custom arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, understood. Thanks

Name: "caConfigMap",
},
&mdbv1.LocalObjectReference{
Name: "certificateKeySecret",
})
}

func newTestReplicaSetWithTLSCaCertificateReferences(caConfigMap, caCertificateSecret *mdbv1.LocalObjectReference) mdbv1.MongoDBCommunity {
return mdbv1.MongoDBCommunity{
ObjectMeta: metav1.ObjectMeta{
Name: "my-rs",
Expand All @@ -101,10 +110,9 @@ func newTestReplicaSetWithTLS() mdbv1.MongoDBCommunity {
Modes: []mdbv1.AuthMode{"SCRAM"},
},
TLS: mdbv1.TLS{
Enabled: true,
CaConfigMap: &mdbv1.LocalObjectReference{
Name: "caConfigMap",
},
Enabled: true,
CaConfigMap: caConfigMap,
CaCertificateSecret: caCertificateSecret,
CertificateKeySecret: mdbv1.LocalObjectReference{
Name: "certificateKeySecret",
},
Expand Down