Simple TLS controller by squeed · Pull Request #347 · openshift/cluster-network-operator

squeed · 2019-10-10T17:28:15Z

This adds a PKI CRD and a controller to implement it. Given a simple request for a PKI, it creates a CA and a single certificate. It delegates this to library-go's CertificateRotationController.

A bit of ugliness is needed to duct-tape a typed and untyped controller library, but it works well enough.

This will be used for implementing TLS for OVN.

danwinship · 2019-10-10T17:46:47Z

This will be used for implementing TLS for OVN.

Why can't we use the existing certificate-y stuff?

Also, any chance this could be used for IPsec?

squeed · 2019-10-11T09:10:18Z

hey @deads2k, this uses the CertRotationController from library-go. Would you mind giving it a look-over to see that we're using it in an expected manner?

squeed · 2019-10-11T09:13:10Z

This will be used for implementing TLS for OVN.

Why can't we use the existing certificate-y stuff?

Because the existing certificate managers all rely on the network to be up. The exception to this is the kube CA / CSR endpoint itself, but I don't want to use that for two reasons. First of all, it doesn't really save us a ton of effort, because it doesn't handle rotation. Second of all, I'd rather not issue certificates that have any sort of kube API identity.

Also, any chance this could be used for IPsec?

Definitely. We may need to get a change in to library-go that allows us to set the ciphers.

danwinship · 2019-10-14T14:34:53Z

+// +k8s:openapi-gen=true
+type PKISpec struct {
+	// caCert is information about the CA certificate and key.
+	CACert CertSpec `json:"caCert"`


Is this information that the user provides or information that the controller fills in? If the former, what gets done with that information?

squeed · 2019-10-15T15:39:14Z

Thanks, as always, for the excellent review, updated.

I renamed PKI to OperatorPKI. Updated the documentation extensively. I also added in kubebuilder struct -> CRD conversion.

danwinship

New docs are great.
I don't really understand most of the actual certificate stuff but I'll trust you're using the library correctly...

danwinship · 2019-10-15T16:21:45Z

+	// commonName is the value in the certificate's CN
+	//
+	// +kubebuilder:validation:MinLength=1
+	CommonName string `json:"commonName"`


Is the CertSpec type still useful? Could we just put TargetCertCommonName in OperatorPKISpec?

I suspect we'll be adding more properties (e.g. validity), so I decided to keep it.

danwinship · 2019-10-15T16:33:51Z

+
+// toClientCert is a certificate "decorator" that adds ClientAuth to the
+// list of ExtendedKeyUsages. This allows the generated certificate to be
+// used for both client and server auth.


Oh, yeah, you didn't answer why you were doing this. Are you going to have the client and server using the same cert? Why not just make two certificates? That seems way more sane...

for OVN, we have to use the same cert, since the databases will be either servers or clients depending on the replication state.

Also, the internal library-go code currently only allows creating a single cert. Eventually we could improve this, but it's not necessary now.

pecameron · 2019-10-15T17:36:40Z

@squeed Your comment at the top says it creates 1 cert. Are we using the same cert/ca for both sdb and ndb, or are there two of these?

squeed · 2019-10-16T11:20:01Z

@squeed Your comment at the top says it creates 1 cert. Are we using the same cert/ca for both sdb and ndb, or are there two of these?

Yup. The intended use case for this is OVN. We're creating a single cert for all users.

squeed · 2019-10-16T12:07:57Z

/retest

(aws flake)

danwinship · 2019-10-16T12:18:27Z

I'm vaguely worried that there may be some cryptographic reason why it would be bad to have the client and server using the same keys, but the only discussion of that I can find is https://crypto.stackexchange.com/questions/7874/client-and-server-using-same-ssl-certificate-any-issues which suggests it's fine other than the obvious issues we don't care about.

squeed · 2019-10-16T13:19:39Z

Establishing a mTLS channel consists of the client sending Encrypt(Server, nonce) followed by Sign(Client, challenge). If there was a way to recover the key from one Encrypt and one Sign operation, then we're in big trouble.

Note: the challenge is not actually chosen, it's a hash of the TLS protocol state. So you can't use the client cert as an oracle. Clever.

squeed · 2019-10-16T13:25:05Z

Nice, the CVO is failing to apply the CRD:
Could not update customresourcedefinition \"operatorpkis.network.operator.openshift.io\" (369 of 445): the object is invalid, possibly due to local cluster configuration

Looking.

danwinship · 2019-10-16T13:52:01Z

Oh, right, the client certificate doesn't actually play any role in establishing the encryption. (I was worrying that if both sides started with the same secrets then some of the math might "cancel out" and result in a predictable symmetric encryption key being generated.)

squeed · 2019-10-16T13:54:36Z

Yeah, it's a good question. I suspect it might be a consideration for DH, though I don't actually think it's possible in TLS.

squeed · 2019-10-16T14:20:33Z

/hold
to stop the retesting

squeed · 2019-10-16T14:56:25Z

hurray, kube-builder doesn't create creatable CRDs:

The CustomResourceDefinition "operatorpkis.network.operator.openshift.io" is invalid: spec.versions: Invalid value: []apiextensions.CustomResourceDefinitionVersion{apiextensions.CustomResourceDefinitionVersion{Name:"v1", Served:true, Storage:true, Schema:(*apiextensions.CustomResourceValidation)(0xc01ffd42a8), Subresources:(*apiextensions.CustomResourceSubresources)(nil), AdditionalPrinterColumns:[]apiextensions.CustomResourceColumnDefinition(nil)}}: per-version schemas may not all be set to identical values (top-level validation should be used instead)

This also updates the update-codegen plumbing to generate the CRD directly from the type, via kubebuilder.

This uses the existing PKI implementation in library-go, wiring it up to the OperatorPKI CRD. Creating a OperatorPKI CR will create a CA and a single certificate.

squeed · 2019-10-16T16:17:34Z

Needed to pick up the latest (unreleased) controller-tools version.

squeed · 2019-10-16T16:32:40Z

/hold cancel

squeed · 2019-10-17T12:08:42Z

aws flake
/retest

squeed · 2019-10-17T12:40:08Z

registry flake
/retest

squeed · 2019-10-17T13:11:59Z

/test e2e-aws-upgrade

danwinship · 2019-10-17T16:17:08Z

/lgtm

squeed · 2019-10-17T17:23:51Z

/refresh

squeed · 2019-10-17T17:33:42Z

hey @danwinship looks like the event got lost - would you mind re-tagging?

danwinship · 2019-10-17T19:36:17Z

/lgtm

openshift-ci-robot · 2019-10-17T19:36:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, squeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [danwinship,squeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-10-17T21:14:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-10-17T21:40:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-10-18T01:21:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

squeed requested review from alexanderConstantinescu and danwinship October 10, 2019 17:28

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 10, 2019

openshift-ci-robot requested a review from JacobTanenbaum October 10, 2019 17:28

squeed commented Oct 14, 2019

View reviewed changes

Comment thread manifests/0000_70_cluster-network-operator_01_pki_crd.yaml Outdated

deads2k reviewed Oct 14, 2019

View reviewed changes

Comment thread pkg/controller/pki/pki_controller.go Outdated

openshift-ci-robot requested a review from sttts October 14, 2019 14:32

danwinship reviewed Oct 14, 2019

View reviewed changes

squeed added 2 commits October 15, 2019 17:38

Dockerfile: bump to go-1.12

c20eabd

dep ensure

4a50fd4

danwinship reviewed Oct 15, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2019

squeed added 2 commits October 16, 2019 18:16

Add OperatorPKI type and CRD

72d4709

This also updates the update-codegen plumbing to generate the CRD directly from the type, via kubebuilder.

Add simple OperatorPKI controller.

85c4fb8

This uses the existing PKI implementation in library-go, wiring it up to the OperatorPKI CRD. Creating a OperatorPKI CR will create a CA and a single certificate.

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2019

openshift-ci-robot assigned danwinship Oct 17, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2019

openshift-merge-robot merged commit 25bc5ea into openshift:master Oct 18, 2019

squeed deleted the tls-controller branch October 14, 2020 16:07

Uh oh!

Conversation

squeed commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danwinship commented Oct 10, 2019

Uh oh!

squeed commented Oct 11, 2019

Uh oh!

squeed commented Oct 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danwinship Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

squeed commented Oct 15, 2019

Uh oh!

danwinship left a comment

Choose a reason for hiding this comment

Uh oh!

danwinship Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

squeed Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

danwinship Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

squeed Oct 16, 2019 • 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!

pecameron commented Oct 15, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

danwinship commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

danwinship commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

squeed commented Oct 16, 2019

Uh oh!

squeed commented Oct 17, 2019

Uh oh!

squeed commented Oct 17, 2019

Uh oh!

squeed commented Oct 17, 2019

Uh oh!

danwinship commented Oct 17, 2019

Uh oh!

squeed commented Oct 17, 2019

Uh oh!

squeed commented Oct 17, 2019

Uh oh!

danwinship commented Oct 17, 2019

Uh oh!

openshift-ci-robot commented Oct 17, 2019

squeed commented Oct 10, 2019 •

edited

Loading

squeed Oct 16, 2019 •

edited

Loading