pkg/util: add interface for tls util #374

fanminshi · 2018-08-01T18:16:28Z

Add interface and config definition for the tls util.

ref design: #371

fanminshi · 2018-08-02T20:48:59Z

hasbro17 · 2018-08-06T23:24:29Z

pkg/util/tlsutil/tls.go

+	// Optional CAKey, if user wants to provide custom CA key path.
+	CAKey string
+	// Optional CA Certificate, if user wants to provide custom CA cert path.
+	CACert string


As per offline discussion we can have the users specify a CAKeySecretName and CACertConfigmapName that's easier to consume for the operator.

I have a concern that not all CAs are stored in this way. I would think a way to support multiple options by passing in the cert/key via path strings, and the user of the Operator would set up the mounts on the operator deployment?

@shawn-hurley I would think a way to support multiple options by passing in the cert/key via path strings.
Do you have a use case that prefers using cert file path on the operator pod over simply retrieving it via secret or ConfigMap. My initial philosophy on designing the operator util is the operator usually deals with Kubernete objects such as Pod, Deployment, Service, and so forth. Then it makes a lot of sense that the tls utility also returns Secret and Configmap so that the operator can just simply consume them without need for converting an external object into to a kubernete object.

Offline Comments:

I could also see that someone does not store the ca cert and specifically the key in the cluster and instead uses some other type of volume to store those values.

Offline comments:

Could also see that someone does not store the ca cert and specifically the key in the cluster and instead uses some other type of volume to store those values ie. NFS, aws, or azure for presistant storage.

@hasbro17 I agree with @shawn-hurley on making passing-in CA key and cert a bit more generic instead of forcing user to pass those in as secret/configmap. Suppose that the user mounts a aws volume that contain sensitive CA key, the user might not want to expose the key as secret into the kubernete cluster but simply want to use it to sign the private key and cert. Let me know what you think.

Agreed. Using a secret and configmap would restrict users from accessing the CA through a mounted volume.

Let's just keep CAKey and CACert as mounted filepaths then.

hasbro17 · 2018-08-06T23:29:14Z

pkg/util/tlsutil/tls.go

+	//   namespace: <cr-namespace>
+	//  data:
+	//   ca.key: ..
+	CACert(cr runtime.Object) (*v1.ConfigMap, *v1.Secret, error)


We can remove this and change the GenerateCert signature to return the CACert(configmap) and CAKey(secret) for that client/server cert. That's more explicit than the users calling CACert() themselves.

GenerateCert(cr runtime.Object, service *v1.Service, config *CertConfig) (*v1.Secret, v1.ConfigMap, v1.Secret, error)

I think it might be better to pass the actual TLS values: key *rsa.PrivateKey, caCert *x509.Certificate This will allow the user to have multiple ways of storing the custom CA per comment above.

@shawn-hurley CACert() function is going to be removed. The GenerateCert is going to return the CA instead.

I think it might be better to pass the actual TLS values. I have thought about this too and decided it is a bit too low level. I suspect operator developer doesn't really want to dealt with rsa key and cert. They might simply want a secret that containing the right encryption key and cert to construct their deployment manifest.

shawn-hurley

I had some concerns about the being too prescriptive on how a custom CA is stored, thoughts?

shawn-hurley · 2018-08-07T15:48:00Z

pkg/util/tlsutil/tls.go

+	// Optional CAKey, if user wants to provide custom CA key path.
+	CAKey string
+	// Optional CA Certificate, if user wants to provide custom CA cert path.
+	CACert string


I have a concern that not all CAs are stored in this way. I would think a way to support multiple options by passing in the cert/key via path strings, and the user of the Operator would set up the mounts on the operator deployment?

shawn-hurley · 2018-08-07T15:51:11Z

pkg/util/tlsutil/tls.go

+	//   namespace: <cr-namespace>
+	//  data:
+	//   ca.key: ..
+	CACert(cr runtime.Object) (*v1.ConfigMap, *v1.Secret, error)


I think it might be better to pass the actual TLS values: key *rsa.PrivateKey, caCert *x509.Certificate This will allow the user to have multiple ways of storing the custom CA per comment above.

fanminshi · 2018-08-07T21:54:34Z

PTAL cc/ @hasbro17 @shawn-hurley

shawn-hurley · 2018-08-08T15:24:06Z

@fanminshi Just too make sure I am following this, if a custom CA is set, then we create a secret and config map object to return to the user. But we do not create those objects in the cluster?

If that is true then LGTM

fanminshi · 2018-08-08T17:03:20Z

@shawn-hurley that's correct!

hasbro17 · 2018-08-08T21:53:32Z

pkg/tlsutil/tls.go

+	//
+	// The passed in "service" represents endpoint(s) of a communicating party. We assume that the
+	// deployed applications are typically communicated via the Kubernete Service. Therefore, the
+	// cert is generated to certify the endpoint(s) defined by the "service".


// The passed in "service" represents endpoint(s) of a communicating party. We assume that the // deployed applications are typically communicated via the Kubernete Service. Therefore, the // cert is generated to certify the endpoint(s) defined by the "service".

We need to make clearly explain exactly what the Service is being used for.

If I'm not wrong then all we're expecting to do with the passed in Service object is to use it's name and namespace to set the certificate's SAN(DNSName) to the Service FQDN name.namespace.svc.cluster.local

certTmpl := x509.Certificate{ ... DNSNames: []string{fmt.Sprintf("%s.%s.svc.cluster.local", service.Name, service.Namespace)}, ... }

So change this to something like:

// The passed in "service" is used to set the Subject Alternative Names(SAN) for the certificate. // We assume that the deployed applications are typically communicated with via a Kubernetes Service. // The SAN is set to the FQDN of the service `<service-name>.<service-namespace>.svc.cluster.local`.

yeah, i should make it a bit more explicit.

rithujohn191 · 2018-08-08T22:26:15Z

pkg/tlsutil/tls.go

+	CACert string
+}
+
+// CertGenerator is an operator specific TLS tool that generates TLS assets for the deploying application.


can we reword the end of the sentence to for deploying a user application, since we have not talked about the application anywhere before this.

rithujohn191 · 2018-08-08T22:29:40Z

pkg/tlsutil/tls.go

+	// cert is generated to certify the endpoint(s) defined by the "service".
+	//
+	// If CA is not passed in via CertConfig, GenerateCert creates a unique self signed CA
+	// corresponding to the "cr" for signing the TLS cert and also puts the self signed CA and TLS


This sentence is a little vague "and also puts the self signed CA and TLS key and into the Kubernetes cluster". Can we be a little more specific of where it is located or how the user can access it if need be.

yeah, i am reworking on it to make it more clear.

rithujohn191 · 2018-08-08T22:31:12Z

pkg/tlsutil/tls.go

+	// key and Certs into the Kubernetes cluster before returning.
+	// If CA is passed in, GenerateCert use it for signing the TLS cert but
+	// only puts TLS key and cert into the cluster not the passed in CA; the user has to
+	// figure out what to do with the returned CA object.


when you say "the user has to figure out what to do with the CA object" do you mean they can re-store it where they like?

kinda of. since when you pass-in the CA, we return a CA configmap and secret back to you. However, we are not going to puts them into the kubernetes. It is the user's job to figure out whether to put them into kubernete or not.

fanminshi · 2018-08-09T20:48:16Z

Modify the comments for the interface. PTAL cc/ @hasbro17 @rithujohn191

hasbro17 · 2018-08-09T23:30:16Z

pkg/tlsutil/tls.go

+	//  - 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 then put into the Cluster before returning then to
+	//    the user.


Slight rephrasing to make it clear exactly where they would be created:

The CA Secret and ConfigMap are created on the k8s cluster in the CR's namespace and returned to the user.

hasbro17 · 2018-08-09T23:37:05Z

pkg/tlsutil/tls.go

+	//  - The CA Secret and ConfigMap are returned to the user without putting them into the
+	//    cluster; the reason behind this is that we don't know how to manage the passed in
+	//    CA. Hence, we simply return them as in Kubernetes Objects, and let the user decides
+	//    what to do such as whether to put them into kubernte or not.


How about something a little shorter:

The CA Secret and Configmap are returned but not created on the k8s cluster. The operator does not manage the user specified CA.

we can, but i think we should still add a why component.

Alright how about:

// The CA Secret and Configmap are returned but not created on the k8s cluster. // The operator does not manage the CA because the user controls the lifecycle of the CA.

sounds better.

do you mean that CertGenerator does not manage?

Yeah the CertGenerator is more specific. Even though it is part of the operator.

hasbro17 · 2018-08-09T23:58:13Z

pkg/tlsutil/tls.go

+	//    what to do such as whether to put them into kubernte or not.
+	//
+	// TLS Key and Cert Creation and Management:
+	// - A unique TLS key is generated per CR + CertConfig.CertName. For instance, calling


I think you meant A unique TLS cert and key. Since both are unique to a CR. Not just the key.

yeah, that's right.

fanminshi · 2018-08-10T00:03:41Z

@hasbro17 I add few sentences to stress the point that the CertGenerator manages CA and secret. Let me know what you think.

hasbro17 · 2018-08-10T00:09:59Z

pkg/tlsutil/tls.go

+	//
+	// TLS Key and Cert Creation and Management:
+	// - A unique TLS cert and key pair is generated per CR + CertConfig.CertName.
+	// - We use the CA determined from the above to generate and to sign the cert of the TLS key.


Since the CA is always used(user specified or newly generated) how about we make this more concise:

The CA is used to generate and sign the TLS cert.

Either that or reword CA determined from the above since it doesn't quite sound right to me.

yeah, not a problem. I was just trying to be more specific on where the CA is from.

hasbro17 · 2018-08-10T00:16:57Z

LGTM after nit.
@rithujohn191 PTAL

shawn-hurley · 2018-08-10T12:50:48Z

pkg/tlsutil/tls.go

+	// - 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


this might be a silly question, but did we consider that these returns are nil?

I think that it would be less confusing for a user if they just didn't get those return values?

Also with 4 return values should we consider a wrapper struct?

this might be a silly question, but did we consider that these returns are nil?
I have not considered return it as nil because I think it is still quite useful to return the CA Secret and ConfigMap. For instance, I might want to mount the CA Cert ConfigMap to a pod so that we can use that to verify the cert that was generated from this CA. If we just simply return nil, the user has to somehow transfer the passed in cert to the designated pod.

Also with 4 return values should we consider a wrapper struct?
I think the 4 values returned is fine for now; it is still quite clear on what's is returning. If we decided to add more return values, we might want to have a wrapper struct.

I might want to mount the CA Cert ConfigMap to a pod so that we can use that to verify the cert that was generated from this CA

Sorry, I am little confused, if the ConfigMap is not created in the cluster because the CA was a custom CA, then this would fail right?

@shawn-hurley If the CA is a custom CA passed in via CertConfig then GenerateCert() will only package the CA cert up as a Configmap and return that object, but not actually create it on the cluster. So it wouldn't really fail in that case.

In the custom CA case it's up to the users whether or not they should create the CA Configmap and Secret returned by GenerateCert() since the operator does not mange the lifecycle of the CA.

rithujohn191

👍

…e-ansible-collections OCPBUGS-30095: Bump kubernetes.core to v2.4.2

fanminshi force-pushed the implement_tls_util branch 8 times, most recently from 0e9b1f3 to eb32a91 Compare August 2, 2018 20:48

fanminshi changed the title ~~[WIP] pkg/util: add interface for tls util~~ pkg/util: add interface for tls util Aug 2, 2018

hasbro17 reviewed Aug 6, 2018

View reviewed changes

shawn-hurley requested changes Aug 7, 2018

View reviewed changes

fanminshi force-pushed the implement_tls_util branch from 50321eb to 553ba2f Compare August 7, 2018 21:53

hasbro17 reviewed Aug 8, 2018

View reviewed changes

rithujohn191 reviewed Aug 8, 2018

View reviewed changes

fanminshi force-pushed the implement_tls_util branch 3 times, most recently from 27dc460 to 97b68ef Compare August 9, 2018 20:46

hasbro17 reviewed Aug 9, 2018

View reviewed changes

fanminshi force-pushed the implement_tls_util branch 2 times, most recently from e2d5068 to fe5eb4f Compare August 10, 2018 00:02

hasbro17 reviewed Aug 10, 2018

View reviewed changes

pkg/tlsutil: add tls util interface and config definition

4609807

fanminshi force-pushed the implement_tls_util branch from fe5eb4f to 4609807 Compare August 10, 2018 00:17

shawn-hurley reviewed Aug 10, 2018

View reviewed changes

shawn-hurley approved these changes Aug 10, 2018

View reviewed changes

rithujohn191 approved these changes Aug 13, 2018

View reviewed changes

fanminshi merged commit 1f9f02d into operator-framework:master Aug 13, 2018

hasbro17 mentioned this pull request Aug 13, 2018

doc: Add TLS utilities proposal #371

Merged

m1kola pushed a commit to m1kola/operator-sdk that referenced this pull request Jun 7, 2024

Merge pull request operator-framework#374 from everettraven/bug/updat…

1234572

…e-ansible-collections OCPBUGS-30095: Bump kubernetes.core to v2.4.2

pkg/util: add interface for tls util #374

pkg/util: add interface for tls util #374

Uh oh!

Conversation

fanminshi commented Aug 1, 2018

Uh oh!

fanminshi commented Aug 2, 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!

fanminshi Aug 7, 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!

shawn-hurley left a comment

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 Aug 7, 2018

Uh oh!

shawn-hurley commented Aug 8, 2018

Uh oh!

fanminshi commented Aug 8, 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!

fanminshi commented Aug 9, 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!

fanminshi Aug 9, 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!

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

fanminshi Aug 7, 2018 •

edited

Loading

fanminshi Aug 9, 2018 •

edited

Loading

fanminshi commented Aug 10, 2018 •

edited

Loading