Skip to content

Conversation

@s-urbaniak
Copy link
Contributor

Starting with OpenShift 4.10 we are introducing PodSecurity admission (https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/2579-psp-replacement).

Currently, all pods are marked as privileged, however, over time we want to enforce at least baseline, admirably restricted as default. In order not to break control plane workloads this allows workloads in openshift-cluster-machine-approver namespace to run privileged pods.

See openshift/enhancements#899 for more details (and excuse the eventual consistency of updates).

/cc @stlaz

@openshift-ci openshift-ci bot requested a review from stlaz October 1, 2021 11:30
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 1, 2021

@s-urbaniak: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-disruptive d3fb5d1 link false /test e2e-aws-disruptive

Full PR test history. Your PR dashboard.

Details

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.

@s-urbaniak
Copy link
Contributor Author

/cc @deads2k

@openshift-ci openshift-ci bot requested a review from deads2k October 1, 2021 14:24
@enxebre
Copy link
Member

enxebre commented Oct 4, 2021

In order not to break control plane workloads this allows workloads in openshift-cluster-machine-approver namespace to run privileged pods.

Why does this need to be privileged? CSRs for master machines during bootstrap are auto approved.

@s-urbaniak
Copy link
Contributor Author

@enxebre your deployment specifies hostNetwork: true, see

.

The policy violation against baseline states: host namespaces (hostNetwork=true), hostPort (container \"kube-rbac-proxy\" uses hostPort 9192)

@enxebre
Copy link
Member

enxebre commented Oct 4, 2021

The policy violation against baseline states: host namespaces (hostNetwork=true), hostPort (container "kube-rbac-proxy" uses hostPort 9192)

Thanks! I question this is actually needed. I can't think of anything that would require the host network for it to work. cc @alexander-demichev @JoelSpeed

@deads2k
Copy link
Contributor

deads2k commented Oct 4, 2021

Thanks! I question this is actually needed. I can't think of anything that would require the host network for it to work. cc @alexander-demichev @JoelSpeed

It's an ordering problem. the machine approver needs to approve a machine so that the kubelet can get a client-cert so that the kubelet can run the CNI plugin daemonset

@enxebre
Copy link
Member

enxebre commented Oct 4, 2021

It's an ordering problem. the machine approver needs to approve a machine so that the kubelet can get a client-cert so that the kubelet can run the CNI plugin daemonset

mm the control plane csr are automatically approved by a bash script. Then the machine approver runs there and can start interacting with the kas to approve any upcoming CSR for workers, why does it need to be through the host network?

@enxebre
Copy link
Member

enxebre commented Oct 4, 2021

David pointed rebootstrapping as a valid scenario. But I'm still not completely sure, if your control plane certs went expired wouldn’t you need to manually approve the new CSRs anyway, since the kas won’t be able to run the mapprover and so the mapprover won’t be able to operate even with host network in that case?

@enxebre
Copy link
Member

enxebre commented Oct 4, 2021

As a general rule we agree "machine approver needs to move towards being earlier in the chain, not being later. So host-network is a good idea and static pods are better"

But I question this is actually practical for this component since it’s not responsible for control plane bootstrapping approval and it’d be good to dissect controversial scenarios. That can be done and prioritised separately though.
Defering to cloud team to chime in and merging.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2021
@elmiko
Copy link
Contributor

elmiko commented Oct 5, 2021

@enxebre i think your reasoning makes good sense. i wonder if we shouldn't approve this PR now, and then make a jira card to go back and investigate lowering the security level once we can determine it is safe. wdyt?

@elmiko
Copy link
Contributor

elmiko commented Oct 7, 2021

i'm going to assume we'll go back and evaluate this, i'll make a card for it. (https://issues.redhat.com/browse/OCPCLOUD-1305)
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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:

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 Oct 7, 2021
@openshift-merge-robot openshift-merge-robot merged commit 88d160e into openshift:master Oct 7, 2021
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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants