Skip to content
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.

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

cert rotation #1091

Merged
merged 2 commits into from
Feb 3, 2020
Merged

cert rotation #1091

merged 2 commits into from
Feb 3, 2020

Conversation

mhenriks
Copy link
Member

@mhenriks mhenriks commented Jan 26, 2020

What this PR does / why we need it:

Currently certs expire after a year. This fixes that by having the operator create/refresh certs. The apiserver and uploadproxy mount certs as secrets and watch for updates to update TLSConfig appropriately.

The operator also creates/manages CA certs for upload server/client.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

The apiserver doesn't support non disruptive update of their CA certs. This would be a pretty easy add on, though. Have to watch CA bundles and reregister apiserver when it changes.

Release note:

Cert rotation

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. size/XXL labels Jan 26, 2020
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jan 26, 2020
@mhenriks
Copy link
Member Author

/hold

Still want to think on a couple things

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.2%) to 60.496% when pulling bb42b35 on mhenriks:cert-rotation into 5b3365f on kubevirt:master.

@coveralls
Copy link

coveralls commented Jan 26, 2020

Coverage Status

Coverage decreased (-0.8%) to 61.724% when pulling d9d8d3c on mhenriks:cert-rotation into 4bceec8 on kubevirt:master.

@maya-r
Copy link
Contributor

maya-r commented Jan 27, 2020

(I might be misreading the code...) Having the private and public keys files in the same directory and accessing it by mounting the directory probably means that the pods verifying keys can also impersonate that key.
Is this a concern?

@maya-r
Copy link
Contributor

maya-r commented Jan 27, 2020

It would be nice to write more of the design somewhere, at least in the commit message
for example:

keys are stored on the master node's filesystem in
/var/run/certs/cdi-uploadproxy-server-cert
... several other places ...
readers watch them for change using fsnotify
writers modify the file

@mhenriks
Copy link
Member Author

mhenriks commented Jan 27, 2020

(I might be misreading the code...) Having the private and public keys files in the same directory and accessing it by mounting the directory probably means that the pods verifying keys can also impersonate that key.
Is this a concern?

@maya-r Not quite sure I understand your concern. But the paths we are protecting are as follows. And in no case does the client have direct access to the server key. In all cases, verification is done via the CA bundles which are stored in configmaps and separate from the keys/certs stored in secrets.

upload proxy -> upload server: upload proxy has upload server CA bundle configmap mounted
clone source -> upload server: clone source has upload server CA bundle passed in as env var
kube apiserver -> cdi apiserver: kube apiserver get apiserver CA bundle in apiserver and webhook registrations
openshift outer -> upload proxy: upload proxy CA bundle is included Route resource

@mhenriks mhenriks force-pushed the cert-rotation branch 4 times, most recently from 5a810a2 to 90bc52f Compare January 29, 2020 00:45
@mhenriks
Copy link
Member Author

/test pull-containerized-data-importer-e2e-k8s-1.16.2-ceph

1 similar comment
@mhenriks
Copy link
Member Author

/test pull-containerized-data-importer-e2e-k8s-1.16.2-ceph

@@ -61,6 +64,7 @@ require (
replace (
github.com/openshift/api => github.com/openshift/api v0.0.0-20191219222812-2987a591a72c
github.com/openshift/client-go => github.com/openshift/client-go v0.0.0-20191125132246-f6563a70e19a
github.com/openshift/library-go => github.com/mhenriks/library-go v0.0.0-20200116194830-9fcc1a687a9d
Copy link
Member Author

Choose a reason for hiding this comment

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

using fork until this is merged: openshift/library-go#540

Copy link
Member

Choose a reason for hiding this comment

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

If we merge this could you create a gh issue so we don't forget to back to the trunk?

@mhenriks mhenriks force-pushed the cert-rotation branch 3 times, most recently from feb4c94 to 9687298 Compare January 30, 2020 16:55
@mhenriks
Copy link
Member Author

/test pull-containerized-data-importer-e2e-k8s-1.16.2-upg

@mhenriks
Copy link
Member Author

/retest

@mhenriks
Copy link
Member Author

/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2020
"sigs.k8s.io/controller-runtime/pkg/predicate"
)

// LeaderAlectionAnnotation is the annotatation used on resources for leader election
Copy link
Member

Choose a reason for hiding this comment

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

LeaderElectionAnnotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

lol yup

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
@awels
Copy link
Member

awels commented Feb 3, 2020

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2020
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2020
@awels
Copy link
Member

awels commented Feb 3, 2020

/test pull-containerized-data-importer-e2e-k8s-1.15.1-ceph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants