Skip to content

pkg/tlsutil: add support for custom CA #470

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

Merged
merged 1 commit into from
Sep 17, 2018
+105 −11
Merged

pkg/tlsutil: add support for custom CA #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion pkg/tlsutil/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ package tlsutil
import "errors"

var (
ErrCANotFound = errors.New("ca secret and configMap are not found")
ErrCANotFound = errors.New("ca secret and configMap are not found")
ErrCAKeyAndCACertReq = errors.New("ca key and ca cert need to be provided when requesting a custom CA.")
ErrInternal = errors.New("internal error while generating TLS assets.")
// TODO: add other tls util errors.
)
66 changes: 58 additions & 8 deletions pkg/tlsutil/tls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"crypto/x509"
"errors"
"fmt"
"io/ioutil"
"strings"

"k8s.io/api/core/v1"
Expand Down Expand Up @@ -138,6 +139,9 @@ type SDKCertGenerator struct {
KubeClient kubernetes.Interface
}

// GenerateCert returns a secret containing the TLS encryption key and cert,
// a ConfigMap containing the CA Certificate and a Secret containing the CA key or it
// returns a error incase something goes wrong.
func (scg *SDKCertGenerator) GenerateCert(cr runtime.Object, service *v1.Service, config *CertConfig) (*v1.Secret, *v1.ConfigMap, *v1.Secret, error) {
if err := verifyConfig(config); err != nil {
return nil, nil, nil, err
Expand All @@ -153,19 +157,57 @@ func (scg *SDKCertGenerator) GenerateCert(cr runtime.Object, service *v1.Service
return nil, nil, nil, err
}
caSecretAndConfigMapName := ToCASecretAndConfigMapName(k, n)
caSecret, caConfigMap, err := getCASecretAndConfigMapInCluster(scg.KubeClient, caSecretAndConfigMapName, ns)

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

caSecret, caConfigMap, err = getCASecretAndConfigMapInCluster(scg.KubeClient, caSecretAndConfigMapName, ns)
if err != nil {
return nil, nil, nil, err
}

if config.CAKey != "" && config.CACert != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
 }
}

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// custom CA provided by the user.
customCAKeyData, err := ioutil.ReadFile(config.CAKey)
if err != nil {
return nil, nil, nil, fmt.Errorf("error reading CA Key from the given file name: %v", err)
}

customCACertData, err := ioutil.ReadFile(config.CACert)
if err != nil {
return nil, nil, nil, fmt.Errorf("error reading CA Cert from the given file name: %v", err)
}

customCAKey, err := parsePEMEncodedPrivateKey(customCAKeyData)
if err != nil {
return nil, nil, nil, fmt.Errorf("error parsing CA Key from the given file name: %v", err)
}

customCACert, err := parsePEMEncodedCert(customCACertData)
if err != nil {
return nil, nil, nil, fmt.Errorf("error parsing CA Cert from the given file name: %v", err)
}
caSecret, caConfigMap = toCASecretAndConfigmap(customCAKey, customCACert, caSecretAndConfigMapName)
} else if config.CAKey != "" || config.CACert != "" {
// if only one of the custom CA Key or Cert is provided
return nil, nil, nil, ErrCAKeyAndCACertReq
}

hasAppSecret := appSecret != nil
hasCASecretAndConfigMap := caSecret != nil && caConfigMap != nil
// TODO: handle passed in CA
if hasAppSecret && hasCASecretAndConfigMap {

switch {
case hasAppSecret && hasCASecretAndConfigMap:
return appSecret, caConfigMap, caSecret, nil
} else if hasAppSecret && !hasCASecretAndConfigMap {

case hasAppSecret && !hasCASecretAndConfigMap:
return nil, nil, nil, ErrCANotFound
} else if !hasAppSecret && hasCASecretAndConfigMap {

case !hasAppSecret && hasCASecretAndConfigMap:
// 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
caKey, err := parsePEMEncodedPrivateKey(caSecret.Data[TLSPrivateCAKeyKey])
if err != nil {
return nil, nil, nil, err
Expand All @@ -187,8 +229,9 @@ func (scg *SDKCertGenerator) GenerateCert(cr runtime.Object, service *v1.Service
return nil, nil, nil, err
}
return appSecret, caConfigMap, caSecret, nil
} else {
// case: both CA and Application TLS assets don't exist.

case !hasAppSecret && !hasCASecretAndConfigMap:
// If no custom CAKey and CACert are provided we have to generate them
caKey, err := newPrivateKey()
if err != nil {
return nil, nil, nil, err
Expand All @@ -197,6 +240,7 @@ func (scg *SDKCertGenerator) GenerateCert(cr runtime.Object, service *v1.Service
if err != nil {
return nil, nil, nil, err
}

caSecret, caConfigMap := toCASecretAndConfigmap(caKey, caCert, caSecretAndConfigMapName)
caSecret, err = scg.KubeClient.CoreV1().Secrets(ns).Create(caSecret)
if err != nil {
Expand All @@ -219,6 +263,8 @@ func (scg *SDKCertGenerator) GenerateCert(cr runtime.Object, service *v1.Service
return nil, nil, nil, err
}
return appSecret, caConfigMap, caSecret, nil
default:
return nil, nil, nil, ErrInternal
}
}

Expand Down Expand Up @@ -252,7 +298,11 @@ func getAppSecretInCluster(kubeClient kubernetes.Interface, name, namespace stri
}

// getCASecretAndConfigMapInCluster gets CA secret and configmap of the given name and namespace.
// it only returns both if they are found and nil if both are not found. In the case if only one of them is found, then we error out because we expect either both CA secret and configmap exit or not.
// it only returns both if they are found and nil if both are not found. In the case if only one of them is found,
// then we error out because we expect either both CA secret and configmap exit or not.
//
// NOTE: both the CA secret and configmap have the same name with template `<cr-kind>-<cr-name>-ca` which is what the
// input parameter `name` refers to.
func getCASecretAndConfigMapInCluster(kubeClient kubernetes.Interface, name, namespace string) (*v1.Secret, *v1.ConfigMap, error) {
hasConfigMap := true
cm, err := kubeClient.CoreV1().ConfigMaps(namespace).Get(name, metav1.GetOptions{})
Expand Down
46 changes: 44 additions & 2 deletions test/e2e/tls_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
framework "github.com/operator-framework/operator-sdk/test/e2e/framework"

"k8s.io/api/core/v1"
apiErrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
)
Expand Down Expand Up @@ -121,7 +122,10 @@ func TestBothAppAndCATLSAssetsExist(t *testing.T) {
}
}

// TestOnlyAppSecretExist tests a case where the application TLS asset exists but its correspoding CA asset doesn't. In this case, CertGenerator can't genereate a new CA because it won't verify the existing application TLS cert. Therefore, CertGenerator can't proceed and returns an error to the caller.
// TestOnlyAppSecretExist tests a case where the application TLS asset exists but its
// correspoding CA asset doesn't. In this case, CertGenerator can't genereate a new CA because
// it won't verify the existing application TLS cert. Therefore, CertGenerator can't proceed
// and returns an error to the caller.
func TestOnlyAppSecretExist(t *testing.T) {
f := framework.Global
ctx := f.NewTestCtx(t)
Expand All @@ -146,7 +150,7 @@ func TestOnlyAppSecretExist(t *testing.T) {
}
}

// TestOnlyCAExist ensures that at the case where only the CA exists in the cluster;
// TestOnlyCAExist tests the case where only the CA exists in the cluster;
// GenerateCert can retrieve the CA and uses it to create a new application secret.
func TestOnlyCAExist(t *testing.T) {
f := framework.Global
Expand Down Expand Up @@ -197,6 +201,44 @@ func TestNoneOfCaAndAppSecretExist(t *testing.T) {
verifyCASecret(t, caSecret, namespace)
}

// TestCustomCA ensures that if a user provides a custom Key and Cert and the CA and Application TLS assets
// do not exist, the GenerateCert method can use the custom CA to generate the TLS assest.
func TestCustomCA(t *testing.T) {
f := framework.Global
ctx := f.NewTestCtx(t)
defer ctx.Cleanup(t)
namespace, err := ctx.GetNamespace()
if err != nil {
t.Fatal(err)
}

cg := tlsutil.NewSDKCertGenerator(f.KubeClient)

customConfig := &tlsutil.CertConfig{
CertName: certName,
CAKey: "testdata/ca.key",
CACert: "testdata/ca.crt",
}
appSecret, _, _, err := cg.GenerateCert(newDummyCR(namespace), newAppSvc(namespace), customConfig)
if err != nil {
t.Fatal(err)
}

verifyAppSecret(t, appSecret, namespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


// ensure caConfigMap does not exist in k8s cluster.
_, err = framework.Global.KubeClient.CoreV1().Secrets(namespace).Get(caConfigMapAndSecretName, metav1.GetOptions{})
if !apiErrors.IsNotFound(err) {
t.Fatal(err)
}

// ensure caConfigMap does not exist in k8s cluster.
_, err = framework.Global.KubeClient.CoreV1().Secrets(namespace).Get(caConfigMapAndSecretName, metav1.GetOptions{})
if !apiErrors.IsNotFound(err) {
t.Fatal(err)
}
}

func verifyCASecret(t *testing.T, caSecret *v1.Secret, namespace string) {
// check if caConfigMap has the correct fields.
if caConfigMapAndSecretName != caSecret.Name {
Expand Down