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.

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

Jump to bottom

API-1438: Split the global trust with kube-control-plane client certs #870

Merged
merged 4 commits into from
Aug 24, 2022
Merged

API-1438: Split the global trust with kube-control-plane client certs #870

merged 4 commits into from
Aug 24, 2022

Conversation

stlaz
Copy link
Member

@stlaz stlaz commented Aug 18, 2022 •
edited

This PR attempts to split away the client certs for kube-scheduler and KCM from the global trust provided by the root CA.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 18, 2022
@stlaz stlaz force-pushed the kube-control-plane-signer-trust branch 2 times, most recently from 94e3cee to 48e47c5 Compare August 18, 2022 13:37
@stlaz stlaz changed the title [WIP] Split the global trust for client certs Split the global trust with kube-control-plane client certs Aug 18, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 18, 2022
@stlaz
Copy link
Member Author

stlaz commented Aug 18, 2022

It seems that this "just" works, unless the 2 remaining test jobs fail. Nevertheless, this might be good for review.

cc @deads2k @ibihim

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2022
@dhellmann
Copy link
Contributor

/kind cleanup

@openshift-ci openshift-ci bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 21, 2022
@stlaz stlaz force-pushed the kube-control-plane-signer-trust branch from 48e47c5 to 3aabaf1 Compare August 22, 2022 07:26
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2022
@dhellmann
Copy link
Contributor

dhellmann commented Aug 22, 2022 •
edited by openshift-ci bot

Linking to USHIFT-278

oglok
oglok reviewed Aug 23, 2022
View reviewed changes
Copy link
Contributor

@oglok oglok left a comment

Choose a reason for hiding this comment

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

Couple of questions. Otherwise, it looks good to me.

In order to understand this PR, it's good to have the following link as reference:

https://github.com/openshift/api/blob/master/tls/docs/kube-apiserver%20Client%20Certificates/README.md#kube-control-plane-signer

and this is just the first change to a better alignment with OpenShift's certificate management.

pkg/cryptomaterial/certpaths.go Outdated Show resolved Hide resolved
pkg/controllers/kube-apiserver.go Outdated Show resolved Hide resolved
pkg/cmd/init.go Show resolved Hide resolved
@stlaz
split KCM and kube-scheduler chains of trust
b3bb55b 
Let's start splitting away the global trust from the root CA:
- generate a kube-control-plane-signer
- have it generate client certs for KCM and kube-scheduler
- create a CA bundle only for the client CA certs

Currently, the client CAs bundle contains the new signer plus the
root CA since we're going to be chipping away the client certs one
by one.
@stlaz
don't ever pass the util-internal rootCA variable around
b6e401e 
The GetRootCA() function allowed retrieval of the util-module internal
rootCA variable and it could've been therefore easily modified by
anyone.
@stlaz
move cryptomaterial module as an util submodule
97b9952
@stlaz stlaz force-pushed the kube-control-plane-signer-trust branch from 3aabaf1 to 07f3636 Compare August 24, 2022 08:48
@stlaz stlaz changed the title Split the global trust with kube-control-plane client certs API-1438: Split the global trust with kube-control-plane client certs Aug 24, 2022
oglok
oglok requested changes Aug 24, 2022
View reviewed changes
pkg/controllers/etcd.go Show resolved Hide resolved
@stlaz stlaz force-pushed the kube-control-plane-signer-trust branch from 07f3636 to b0fc30d Compare August 24, 2022 10:31
@oglok
Copy link
Contributor

oglok commented Aug 24, 2022

Fatal!

F0824 13:37:55.998994 19013 kubelet.go:150] Error in fetching depenedencies%!(EXTRA *fmt.wrapError=unable to load client CA file /var/lib/microshift/ca-bundle/ca-bundle.crt: open /var/lib/microshift/ca-bundle/ca-bundle.crt: no such file or directory) goroutine 34528 [running]:

@stlaz stlaz force-pushed the kube-control-plane-signer-trust branch from b0fc30d to d109a47 Compare August 24, 2022 11:44
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 24, 2022

@stlaz: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

oglok
oglok approved these changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@oglok oglok left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oglok, stlaz

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2022
@openshift-merge-robot openshift-merge-robot merged commit b95f863 into openshift:main Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibihim ibihim ibihim left review comments

@oglok oglok oglok approved these changes

@dhellmann dhellmann Awaiting requested review from dhellmann

@mangelajo mangelajo Awaiting requested review from mangelajo

Assignees

@oglok oglok

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stlaz @dhellmann @oglok @ibihim @openshift-merge-robot