Group replication SSL #115

simonlord · 2018-05-29T09:45:42Z

SSL with autogenerated certs as standard (a feature of mysql, certs valid for 10 years)
Confirgurable with your own CA cert, tls cert and tls key via a tls secret

Limitations:

Currently not verifying server cert against ca cert (VERIFY_CA mode).
Only supports 1 server cert/key pair that will be mounted to all mysql containers
- Really you'd want a new cert/key pair per pod and then turn on VERIFY_IDENTITY
  to enforce hostname/common name checking. This would require vault/cert-manager

Resolves: #89

Changelog:

Group communication connections as are now secured using SSL with support for specifying your own certificate [#115].

prydie

A few comments but looking good 👍

prydie · 2018-05-29T11:09:05Z

pkg/apis/mysql/v1/cluster.go

+	// SSLSecretRef allows a user to specify custom CA certificate, server certificate
+	// and server key for group replication ssl
+	// +optional
+	SSLSecretRef *corev1.LocalObjectReference `json:"sslSecretRef,omitempty"`


Maybe it's bikeshedding but I'm not sure we made the right decision re {Foo}Ref I'd rather SSLSecret, RootPasswordSecret and MyCNF or similar. Probably out of scope though.

This should be a separate issue to do them all at once

Agree with you both. I'm not a huge fan. Let's do this afterwards.

prydie · 2018-05-29T11:11:09Z

pkg/resources/statefulsets/statefulset.go

+								LocalObjectReference: v1.LocalObjectReference{
+									Name: cluster.Spec.SSLSecretRef.Name,
+								},
+								Items: []v1.KeyToPath{


Can we use a TLS secret?

If we can use it i'd like to - unfortunately we have to provide the CA cert file too for MySQL to be happy - tls secrets only have the server cert and key pair. Also it isn't tls, it's ssl still.

So we could 1) add a ca.crt field to the tls secret. 2) have the ca.crt in a different secret or 3) leave it how it is.

Thoughts?

Ah, shame. I’d follow the naming scheme from the TLS secret and add the ca.crt as you suggest.

prydie · 2018-05-29T11:45:18Z

pkg/resources/statefulsets/statefulset.go

@@ -211,6 +219,10 @@ func mysqlServerContainer(cluster *api.MySQLCluster, mysqlServerImage string, ro
 		"--log-error-verbosity=3",
 	}

+	if cluster.RequiresCustomSSLSetup() {
+		args = append(args, "--ssl-ca=/etc/ssl/mysql/ca.crt", "--ssl-cert=/etc/ssl/mysql/server.crt", "--ssl-key=/etc/ssl/mysql/server.key")


nit: would probably split into an arg per line

prydie · 2018-05-29T11:46:35Z

examples/custom-ssl-secret.yaml

+  cacert: <base64'd Root CA certifacte> 
+  servercert: <base64'd server certificate> 
+  serverkey:  <base64'd server private key>
+


prydie · 2018-05-29T11:53:18Z

pkg/resources/statefulsets/statefulset_test.go

+		}
+	}
+
+	if !hasExpectedVolumeMount {


assert.True(t, hasExpectedVolumeMount, "Cluster is missing expected volume mount for custom ssl certs")

(github.com/stretchr/testify/assert)

prydie · 2018-05-29T11:54:43Z

pkg/apis/mysql/v1/cluster_test.go

+		},
+	}
+
+	if !cluster.RequiresCustomSSLSetup() {


assert.True(t, cluster.RequiresCustomSSLSetup(), "Cluster with sslSecretRef should require custom ssl setup")

(github.com/stretchr/testify/assert)

prydie · 2018-05-29T11:55:30Z

pkg/apis/mysql/v1/cluster_test.go

+	cluster := &MySQLCluster{}
+	cluster.EnsureDefaults()
+
+	if cluster.RequiresCustomSSLSetup() {


assert.False(t, cluster.RequiresCustomSSLSetup(), "Cluster without sslSecretRef should not require custom ssl setup")

(github.com/stretchr/testify/assert)

owainlewis · 2018-05-30T10:35:19Z

examples/custom-ssl-secret.yaml

+  name: mysql-ssl-secret
+type: Opaque
+data:
+  ca.crt: <base64'd Root CA certifacte>


Typo certifact => certificate

owainlewis · 2018-05-30T10:36:02Z

pkg/apis/mysql/v1/cluster.go

@@ -95,6 +95,11 @@ type MySQLClusterSpec struct {
 	// ConfigRef allows a user to specify a custom configuration file for MySQL.
 	// +optional
 	ConfigRef *corev1.LocalObjectReference `json:"configRef,omitempty"`
+
+	// SSLSecretRef allows a user to specify custom CA certificate, server certificate
+	// and server key for group replication ssl


owainlewis · 2018-05-30T10:36:36Z

pkg/apis/mysql/v1/cluster.go

@@ -203,6 +208,13 @@ func (c *MySQLCluster) RequiresSecret() bool {
 	return c.Spec.SecretRef == nil
 }

+// RequiresCustomSSLSetup returns true is the user has provided a secret
+// that contains CA cert, server cert and server key for group replication
+// ssl support


owainlewis · 2018-05-30T10:39:55Z

pkg/apis/mysql/v1/cluster_test.go

@@ -84,3 +85,20 @@ func TestRequiresConfigMount(t *testing.T) {
 		t.Errorf("Cluster with configRef should require a config mount")
 	}
 }
+
+func TestRequiresCustomSSLSetup(t *testing.T) {
+	cluster := &MySQLCluster{}


Not relevant to this PR but I'd be in favour of abstracting this given how often it comes up in the codebase and how easy it would be to make a mistake by not setting defaults.

cluster := NewMySQLCluster() | NewMySQLClusterWithDefaults()

owainlewis · 2018-05-30T10:40:23Z

pkg/apis/mysql/v1/cluster_test.go

+		},
+	}
+
+	assert.True(t, cluster.RequiresCustomSSLSetup(), "Cluster with sslSecretRef should require custom ssl setup")


owainlewis · 2018-05-30T10:42:33Z

pkg/resources/statefulsets/statefulset_test.go

+		}
+	}
+
+	assert.True(t, hasExpectedVolumeMount, "Cluster is missing expected volume mount for custom ssl certs")


owainlewis

Looks good to me. Other than a minor typo this is good to go IMO

- SSL with autogenerated certs as standard (a feature of mysql, certs valid for 10 years) - Confirgurable with your own CA cert, tls cert and tls key via a tls secret

prydie

A couple of missing full stops but not reason enough to block merge. 👍

simonlord requested review from prydie and owainlewis May 29, 2018 09:45

prydie added the enhancement label May 29, 2018

prydie added this to the 0.2.0 milestone May 29, 2018

prydie suggested changes May 29, 2018

View reviewed changes

simonlord force-pushed the group-replication-ssl branch 3 times, most recently from 1e98125 to a6bd303 Compare May 30, 2018 09:59

simonlord changed the title ~~WIP: Group replication SSL~~ Group replication SSL May 30, 2018

owainlewis self-assigned this May 30, 2018

owainlewis reviewed May 30, 2018

View reviewed changes

owainlewis suggested changes May 30, 2018

View reviewed changes

Group replication SSL

72b2396

- SSL with autogenerated certs as standard (a feature of mysql, certs valid for 10 years) - Confirgurable with your own CA cert, tls cert and tls key via a tls secret

simonlord force-pushed the group-replication-ssl branch from a6bd303 to 72b2396 Compare May 30, 2018 10:50

owainlewis approved these changes May 30, 2018

View reviewed changes

prydie approved these changes May 30, 2018

View reviewed changes

prydie merged commit d42c413 into master May 30, 2018

prydie deleted the group-replication-ssl branch May 30, 2018 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group replication SSL #115

Group replication SSL #115

simonlord commented May 29, 2018 •

edited by prydie

Loading

prydie left a comment

prydie May 29, 2018

simonlord May 29, 2018

owainlewis May 30, 2018

prydie May 29, 2018

simonlord May 29, 2018

prydie May 29, 2018

simonlord May 30, 2018

prydie May 29, 2018

prydie May 29, 2018

prydie May 29, 2018

prydie May 29, 2018

prydie May 29, 2018

owainlewis May 30, 2018

owainlewis May 30, 2018

owainlewis May 30, 2018

owainlewis May 30, 2018

owainlewis May 30, 2018

owainlewis May 30, 2018

owainlewis left a comment •

edited

Loading

prydie left a comment

Group replication SSL #115

Group replication SSL #115

Conversation

simonlord commented May 29, 2018 • edited by prydie Loading

prydie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis left a comment • edited Loading

Choose a reason for hiding this comment

prydie left a comment

Choose a reason for hiding this comment

simonlord commented May 29, 2018 •

edited by prydie

Loading

owainlewis left a comment •

edited

Loading