Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

adds tls certificate to tls config #681

Merged
merged 1 commit into from
Mar 23, 2018
Merged

adds tls certificate to tls config #681

merged 1 commit into from
Mar 23, 2018

Conversation

mikebrow
Copy link
Member

@mikebrow mikebrow commented Mar 19, 2018

Addresses issue #665

Adds a tls config comprising a newly generated x509 certificate from a newly generated public/private key pair.

Signed-off-by: Mike Brown [email protected]

@mikebrow
Copy link
Member Author

/retest

@Random-Liu
Copy link
Member

Random-Liu commented Mar 21, 2018

Can we use https://github.com/kubernetes/client-go/blob/master/util/cert/cert.go#L63?

It seems that we only need cert.NewPrivateKey + cert.NewSelfSignedCACert + tls.X509KeyPair.
I searched pem.EncodeToMemory in Kubernetes code and found this library.

@mikebrow
Copy link
Member Author

mikebrow commented Mar 21, 2018

Can we use https://github.com/kubernetes/client-go/blob/master/util/cert/cert.go#L63?

It seems that we only need cert.NewPrivateKey + cert.NewSelfSignedCACert + tls.X509KeyPair.
I searched pem.EncodeToMemory in Kubernetes code and found this library.

Yes, if/when we want to generate a "self signed" Certificate Authority (CA) certificate to use when creating our self signed user certificate, thus creating the CA chain I mentioned over chat, we could use this code, or something close to it, to do that.

As for using these cert. apis "now" those particular cert. apis are just thin wrappers with a few predefined constants that we may or may not want. If we did use these cert. apis we'd also be using cert.NewSignedCert to consume our new CACert when generating the cert that we need for the streaming encryption.

*** However, after reading through cert.GenerateSelfSignedCertKey, that looks to me to be (possibly) the more useful api for our needs :-) Pretty sure I could recode to use that api, if we decide to go the full self signed CA cert chain route.

Let's leave this question as a potential route to go depending on what auth requirements get set for the CRI API. For example, self signed CA cert might be an admin decision not a runtime one.

We should open an auth design issue for that here in containerd/cri and another over in the kubernetes CRI api.

Copy link
Member

@Random-Liu Random-Liu left a comment

Choose a reason for hiding this comment

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

LGTM with nits.


// Generate pem block using the private key
keyPem := pem.EncodeToMemory(&pem.Block{
Type: "RSA PRIVATE KEY",
Copy link
Member

Choose a reason for hiding this comment

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

Use a const.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


// Generate a pem block with the certificate
certPem := pem.EncodeToMemory(&pem.Block{
Type: "CERTIFICATE",
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

NotAfter: time.Now().AddDate(years, 0, 0),
SerialNumber: serialNumber,
Subject: pkix.Name{
CommonName: fmt.Sprintf("system:node:%s", hostName),
Copy link
Member

Choose a reason for hiding this comment

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

Let's use something else. This is for kubelet I think.

containerd?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -112,3 +130,80 @@ func handleResizing(resize <-chan remotecommand.TerminalSize, resizeFunc func(si
}
}()
}

func newTLSCert() (tls.Certificate, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Add comment to explain what we are doing here, and add TODO.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@mikebrow
Copy link
Member Author

comments addressed @Random-Liu

"k8s.io/kubernetes/pkg/kubelet/server/streaming"
"k8s.io/utils/exec"

ctrdutil "github.com/containerd/cri/pkg/containerd/util"
)

const (
Copy link
Member

Choose a reason for hiding this comment

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

Prefer private const. I can take care of that.

@Random-Liu
Copy link
Member

/lgtm

@Random-Liu Random-Liu merged commit 5ae4de1 into containerd:master Mar 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants