pkg/tlsutil: add support for custom CA #470

rithujohn191 · 2018-09-10T22:39:28Z

this PR adds support for users to provide a custom CA Key and Cert to generate other TLS assets. A unit test has also been added to test the functionality.

fanminshi · 2018-09-10T23:08:44Z

pkg/tlsutil/tls.go

@@ -161,11 +165,15 @@ func (scg *SDKCertGenerator) GenerateCert(cr runtime.Object, service *v1.Service
 	hasAppSecret := appSecret != nil
 	hasCASecretAndConfigMap := caSecret != nil && caConfigMap != nil
 	// TODO: handle passed in CA
+


It appears to me that the code can be simpler if we just check if ca is passed in and package them into ca secret and config before we get the CA from the cluster using getCASecretAndConfigMapInCluster at line 160. Once you have done that, the rest of the code should take care of itself.

yes but then we would have to call the code from 247 onwards in two locations since we would have to create the configMap and secret in the cluster.

if we have CA config and secret are created from the passed in CA, the hasCASecretAndConfigMap is always true. Then the code never hits the case case: both CA and Application TLS assets don't exist.. It will only be either the case hasAppSecret && hasCASecretAndConfigMap or !hasAppSecret && hasCASecretAndConfigMap.

What I meant to say was if we have to create a CA config and secret from the passed in CA, the flow would be as follows:

Read the PEM bytes from the input file for Key and Cert

Parse PEM encoded bytes

Call toCASecretAndConfigmap to generate secret and ConfigMap

Call scg.KubeClient.CoreV1().Secrets(ns).Create to create secret and configMap

Since the above steps are already being done in the case where both CA and Application TLS assets don't exist, I thought it would be more clear if we combined the custom CA case with it.

I thought that we should skip 4 for custom CA's?

@rithujohn191 we can skip step 4. so that we don't put ca configmap and secret into the cluster which is in agreement of the interface definition.

see:

// - If CA is given: // - CA's key is packaged into a Secret as shown below. // - CA's cert is packaged in a ConfigMap as shown below. // - The CA Secret and ConfigMap are returned but not created in the K8s cluster in the CR's // namespace. The CertGenerator doesn't manage the CA because the user controls the lifecycle // of the CA.

https://github.com/operator-framework/operator-sdk/blob/master/pkg/tlsutil/tls.go#L79-L81

ah I see. Looks like this was finalized when I was out. Okay I will make the change

I have made the changes according to the discussion

shawn-hurley

I think that we are missing the logic about not creating the config map and secret in the cluster when a custom CA is passed in.

shawn-hurley · 2018-09-11T15:15:39Z

pkg/tlsutil/tls.go

-		caKey, err := newPrivateKey()
-		if err != nil {
-			return nil, nil, nil, err
+		if config.CAKey != "" && config.CACert != "" {


I think this chain of if and if/else could be clearer if in a switch? Thoughts?

I switched this from an if/else to only if cause I thought it would be cleared. You would just have to look at the conditional statement as opposed to calculate which case is remaining

shawn-hurley · 2018-09-11T15:47:52Z

pkg/tlsutil/tls.go

 		}
-		caSecret, caConfigMap := toCASecretAndConfigmap(caKey, caCert, caSecretAndConfigMapName)
+
+		caSecret, caConfigMap := toCASecretAndConfigmap(caKeyData, caCertData, caSecretAndConfigMapName)


This code would create the config map/secret for the custom CA. I thought we decided we should not do this?

fanminshi · 2018-09-11T19:59:18Z

pkg/tlsutil/tls.go

@@ -158,14 +162,49 @@ func (scg *SDKCertGenerator) GenerateCert(cr runtime.Object, service *v1.Service
 		return nil, nil, nil, err
 	}

+	var customCAKey *rsa.PrivateKey


you can have

var ( caSecret *v1.Secret caConfigMap *v1.ConfigMap )

here instead of var customCAKey *rsa.PrivateKey

And In your if config.CAKey != "" && config.CACert != "" { block, you can wrap customCAKey and customCAKey into caSecret and caConfigMap by calling toCASecretAndConfigmap. then the rest of code should take care of itself.

The steps can be:

check if custom ca is passed in.

if passed in, transform those into caSecret and caConfigMap in your if config.CAKey != "" && config.CACert != "" block.

if not passed, call getCASecretAndConfigMapInCluster to get CA from cluster.

rest of code doesn't need to be modify.

but why would we want to transform a custom CA provided to us in PEM format to the type *v1.Secret and *v1.ConfigMap. Its a just an extra overhead

rithujohn191 · 2018-09-11T20:37:11Z

One down side of this PR is that if the user passes in a custom CA while a CA ConfigMap and secret exists the the custom CA takes preference. I have added a note in the code to highlight this:

"// Note: if a custom CA is passed in my the user it takes preference over an already
// generated CA secret and CA configmap that might exist in the cluster
"

fanminshi · 2018-09-11T21:04:37Z

pkg/tlsutil/tls.go

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

+	if config.CAKey != "" && config.CACert != "" {


We can have a boolean value hasPassedInCA := config.CAKey != "" && config.CACert != "".
And we only call getCASecretAndConfigMapInCluster if hasPassedInCA is false. e.g

if hasPassedInCA { ... } if config.CAKey != "" || config.CACert != "" { // if only one of the custom CA Key or Cert is provided return nil, nil, nil, ErrCAKeyAndCACertReq } if !hasPassedInCA { caSecret, caConfigMap, err = getCASecretAndConfigMapInCluster(scg.KubeClient, caSecretAndConfigMapName, ns) if err != nil { return nil, nil, nil, err } }

It doesnt really matter cause if getCASecretAndConfigMapInCluster is called before and no configMap exists the values of caSecret & caConfigMap will be nil. Then in the if config.CAKey != "" && config.CACert != "" { block the values get updated

I think this makes sense if I was using a generated cert but now have a custom CA I would expect that to take precedence.

I could see us making the change @fanminshi suggested only if we are worried about two extra get calls to the API server that we know will fail/don't care about. I personally believe that the way it is more readable and would like to keep it the way it is, but I could see a reason to change if we are worried about too many calls to the api server.

@rithujohn191 your code is correct. My suggestion is just an optimization because getCASecretAndConfigMapInCluster makes calls to k8s cluster which is unnecessary if custom ca is passed.

I had the same thought process, I think code readability is better the way it is right now and 2 GET calls from the api server is not heavy weight at all. I think the way the code is currently written makes it more clear that the custom CA takes preference. But if you insist I could change it. Either way its not much of a difference.

@rithujohn191 let's just keep the code as it is for readability. we can revisit the code if we want to optimize it.

shawn-hurley · 2018-09-12T13:16:33Z

pkg/tlsutil/tls.go

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

+	if config.CAKey != "" && config.CACert != "" {


I think this makes sense if I was using a generated cert but now have a custom CA I would expect that to take precedence.

I could see us making the change @fanminshi suggested only if we are worried about two extra get calls to the API server that we know will fail/don't care about. I personally believe that the way it is more readable and would like to keep it the way it is, but I could see a reason to change if we are worried about too many calls to the api server.

shawn-hurley · 2018-09-12T13:22:29Z

pkg/tlsutil/tls.go

+		caSecret, caConfigMap = toCASecretAndConfigmap(customCAKey, customCACert, caSecretAndConfigMapName)
+	}
+
+	if config.CAKey != "" || config.CACert != "" {


Just thinking through this, I am a little worried about this line. I believe that both if statements could be true and we could be exiting early.

config.CAKey != "" && config.CACert != ""
and
config.CAKey != "" || config.CACert != ""

I think this should be changed to an else if block on the above if statement?

yeah agreed. we need an else if here. cc. @rithujohn191

fanminshi · 2018-09-12T21:04:16Z

test/e2e/tls_util_test.go

+	cg := tlsutil.NewSDKCertGenerator(f.KubeClient)
+
+	ccfg.CAKey = "./testdata/ca.key"
+	ccfg.CACert = "./testdata/ca.crt"


let's keep the test variable the same for all tests. I think modifying the ccfgwill cause other tests to error out.

Let's just create a local *tlsutil.CertConfig for this test only.

Oh ok will do

fanminshi · 2018-09-13T23:11:28Z

CI is failing due to current dir is changed in TestMemcached which resulting the TestCustomCA not able to resolve the test/e2e/testdata folder ref: #483.

rithujohn191 · 2018-09-14T19:01:30Z

All tests pass and code has been updated. Please take a second look.

fanminshi · 2018-09-14T19:09:54Z

test/e2e/tls_util_test.go

+		t.Fatal(err)
+	}
+
+	verifyAppSecret(t, appSecret, namespace)


We need to check that no ca is being put into k8s cluster.

Do you mean that we should explicitly try to 'GET' the ca secret and Configmap with the given name and ensure nothing is returned?

yes, that's correct.

shawn-hurley

LGTM after adding that ca is not added to cluster in the test

fanminshi · 2018-09-14T21:27:20Z

test/e2e/tls_util_test.go

+
+	// ensure caConfigMap does not exist in k8s cluster.
+	_, err = framework.Global.KubeClient.CoreV1().Secrets(namespace).Get(caConfigMapAndSecretName, metav1.GetOptions{})
+	if !strings.Contains(err.Error(), "not found") {


you can use functions within the k8s errors pkg to check if the err is a not found error. e.g

import apiErrors "k8s.io/apimachinery/pkg/api/errors" func TestCustomCA(t *testing.T) { ... if !apiErrors.IsNotFound(err) { t.Fatal(err) } } ``

fanminshi · 2018-09-14T21:27:28Z

test/e2e/tls_util_test.go

+
+	// ensure caConfigMap does not exist in k8s cluster.
+	_, err = framework.Global.KubeClient.CoreV1().Secrets(namespace).Get(caConfigMapAndSecretName, metav1.GetOptions{})
+	if !strings.Contains(err.Error(), "not found") {


use apiErrors.IsNotFound(err) instead.

fanminshi · 2018-09-14T21:28:00Z

lgtm after nits.

rithujohn191 requested a review from fanminshi September 10, 2018 22:39

fanminshi reviewed Sep 10, 2018

View reviewed changes

shawn-hurley requested changes Sep 11, 2018

View reviewed changes

rithujohn191 force-pushed the custom_CA branch from cad07d3 to 9adb7f3 Compare September 11, 2018 19:47

fanminshi reviewed Sep 11, 2018

View reviewed changes

rithujohn191 force-pushed the custom_CA branch from 9adb7f3 to 86a4ec4 Compare September 11, 2018 20:35

fanminshi reviewed Sep 11, 2018

View reviewed changes

shawn-hurley reviewed Sep 12, 2018

View reviewed changes

fanminshi reviewed Sep 12, 2018

View reviewed changes

rithujohn191 force-pushed the custom_CA branch 12 times, most recently from 6deb3e5 to 625e18f Compare September 13, 2018 22:37

rithujohn191 force-pushed the custom_CA branch 4 times, most recently from a8368e2 to cd40ee5 Compare September 14, 2018 18:35

fanminshi reviewed Sep 14, 2018

View reviewed changes

shawn-hurley approved these changes Sep 14, 2018

View reviewed changes

rithujohn191 force-pushed the custom_CA branch 3 times, most recently from 0a0427e to 97c4be3 Compare September 14, 2018 20:41

fanminshi reviewed Sep 14, 2018

View reviewed changes

pkg/tlsutil: add support for custom CA

636b3fa

rithujohn191 force-pushed the custom_CA branch from 97c4be3 to 636b3fa Compare September 17, 2018 16:01

rithujohn191 merged commit 190f8a9 into operator-framework:master Sep 17, 2018

rithujohn191 deleted the custom_CA branch September 17, 2018 17:03

pkg/tlsutil: add support for custom CA #470

pkg/tlsutil: add support for custom CA #470

Uh oh!

Conversation

rithujohn191 commented Sep 10, 2018

Uh oh!

fanminshi Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanminshi Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawn-hurley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rithujohn191 Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rithujohn191 commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanminshi commented Sep 13, 2018

Uh oh!

rithujohn191 commented Sep 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawn-hurley left a comment

Choose a reason for hiding this comment

Uh oh!

fanminshi Sep 10, 2018 •

edited

Loading

fanminshi Sep 11, 2018 •

edited

Loading

rithujohn191 Sep 11, 2018 •

edited

Loading