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

PKI Directory should follow principle of least privilege #1691

Open
rcythr opened this issue Jul 28, 2019 · 10 comments
Open

PKI Directory should follow principle of least privilege #1691

rcythr opened this issue Jul 28, 2019 · 10 comments
Labels
area/security breaking-change kind/design Categorizes issue or PR as related to design. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Milestone

Comments

@rcythr
Copy link

rcythr commented Jul 28, 2019

FEATURE REQUEST

Problem
Currently kube-apiserver, kube-controller-manager, and etcd all mount in more of the files from /etc/kubernetes/pki than they need to perform their job. This is a violation of the principle of least privilege.

Proposed Fix
The certificates generation process should be adjusted to create separate directories for each combination of services which require files. An analysis of command line arguments shows that this can be done with 5 directories. The following lists files under the category based on what process is configured to use them.

  1. None
    /etc/kubernetes/pki/front-proxy-ca.key
    /etc/kubernetes/pki/etcd/ca.key

  2. Etcd only
    /etc/kubernetes/pki/etcd/peer.crt
    /etc/kubernetes/pki/etcd/peer.key
    /etc/kubernetes/pki/etcd/server.crt
    /etc/kubernetes/pki/etcd/server.key
    /etc/kubernetes/pki/etcd/healthcheck-client.crt
    /etc/kubernetes/pki/etcd/healthcheck-client.key
    /etcd/kubernetes/pki/etcd/ca.crt

  3. Apiserver only
    /etc/kubernetes/pki/apiserver-etcd-client.crt
    /etc/kubernetes/pki/apiserver-etcd-client.key
    /etc/kubernetes/pki/apiserver-kubelet-client.crt
    /etc/kubernetes/pki/apiserver-kubelet-client.key
    /etc/kubernetes/pki/apiserver.crt
    /etc/kubernetes/pki/apiserver.key
    /etc/kubernetes/pki/ca.crt
    /etc/kubernetes/pki/etcd/ca.crt
    /etc/kubernetes/pki/front-proxy-ca.crt
    /etc/kubernetes/pki/front-proxy-client.key
    /etc/kubernetes/pki/sa.pub

  4. Controller-Manager only
    /etc/kubernetes/pki/ca.key
    /etc/kubernetes/pki/sa.key

  5. Api-server + Controller Manager
    /etc/kubernetes/pki/ca.crt
    /etc/kubernetes/pki/front-proxy-client.crt

Notes
#1350 proposes that /etc/kubernetes/pki/ca.crt be split into a separate file for controller manager.

If it's agreed that this is a problem which we'd like to resolve, I'm happy to work on the code change to resolve it.

@neolit123 neolit123 added this to the Next milestone Jul 28, 2019
@neolit123 neolit123 added kind/design Categorizes issue or PR as related to design. area/security labels Jul 28, 2019
@neolit123
Copy link
Member

thanks for logging @rcythr

Currently kube-apiserver, kube-controller-manager, and etcd all mount in more of the files from /etc/kubernetes/pki than they need to perform their job. This is a violation of the principle of least privilege.

i agree with the problem statement.

The certificates generation process should be adjusted to create separate directories for each combination of services which require files.

can symlinks be used instead of duplicating files for the separate directories?

cc @fabriziopandini @randomvariable

@neolit123 neolit123 added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Jul 28, 2019
@rcythr
Copy link
Author

rcythr commented Jul 28, 2019

If we symlink we'd have to have the directory with the actual file mounted in as well or it would be a bad link. Mounting that other directory would include other files we don't need.

I was thinking of making subdirectories called :

/etc/kubernetes/pki/etcd/
/etc/kubernetes/pki/apiserver/
/etc/kubernetes/pki/controller-manager/
/etc/kubernetes/pki/apiserver_controller-manager/

That way no files need to be duplicated, they just exist in a directory mounted by both pods.

@rcythr
Copy link
Author

rcythr commented Jul 28, 2019

/assign
/lifecycle active

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Jul 28, 2019
@neolit123
Copy link
Member

I was thinking of making subdirectories called :

if these directories are on the host, wouldn't there will be duplication of /etc/kubernetes/pki/ca.crt between /etc/kubernetes/pki/apiserver/ and /etc/kubernetes/pki/apiserver_controller-manager/?

i think it might be better to simply mount only the required files in the Pods and keep the existing structure that we have.

xref for a list of mounts the Pods are doing right now:
#1367 (comment)

@rcythr
Copy link
Author

rcythr commented Jul 28, 2019

if these directories are on the host, wouldn't there will be duplication of /etc/kubernetes/pki/ca.crt between /etc/kubernetes/pki/apiserver/ and /etc/kubernetes/pki/apiserver_controller-manager/?
i think it might be better to simply mount only the required files in the Pods and keep the existing structure that we have.

I was thinking of modifying the structure so ca.crt would be moved into apiserver_controller-manager/. This keeps the number of mounts at a minimum. If keeping the structure the same is required it can be done, but the number of mounts will be larger.

I’m happy to try it the the way which involves no file reorganization first. Once we see the code and results we can revector a bit if we want/need to.

@neolit123
Copy link
Member

If keeping the structure the same is required it can be done, but the number of mounts will be larger.

i have concerns of introducing directory structure changes on the host, which might break a lot of setups.

I’m happy to try it the the way which involves no file reorganization first. Once we see the code and results we can revector a bit if we want/need to.

thanks. i'd prefer to see feedback from @fabriziopandini @randomvariable too, before proceeding with an implementation.

@rcythr
Copy link
Author

rcythr commented Jul 28, 2019

i have concerns of introducing directory structure changes on the host, which might break a lot of setups.

Agreed, it definitely creates a breaking change or a very complicated upgrade procedure.

thanks. i'd prefer to see feedback from @fabriziopandini @randomvariable too, before proceeding with an implementation.

Understood, I won't be able to work on it for a day or two anyway. I'll standby until we reach a consensus.

@neolit123 neolit123 removed the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Oct 13, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 11, 2020
@neolit123
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 11, 2020
@fabriziopandini
Copy link
Member

@rcythr @neolit123 I apologize for being super late on this thread
The topic is interesting, but as commented above, manging upgrades is far from being trivial.
It is also requires a good chunk of work on the documentation, E2E tests, and a massive communication plan to inform/coordinate all the high-level tool maintainers before the change goes lives.

Considering ^^^, I think this topic definitely requires a KEP, and that also we should explore a few options like e.g. "automatic upgrade" vs "dual-mode (support for both directory layouts) with manual opt-in for existing clusters"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security breaking-change kind/design Categorizes issue or PR as related to design. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

No branches or pull requests

5 participants