hardening openshift-apiserver default policy resources on upgrade clusters #312
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vareti The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
vareti
force-pushed
the
anon-user
branch
2 times, most recently
from
36fdc00
to
43155ba
Compare
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| Because of the earlier releases of OpenShift allowing access to “/” endpoint, some customer implementations might be expecting this privileged access to anonymous users. Removing unauthenticated subject on an upgrade might break some of these implementations. Informing customers beforehand of this change would help the customer to decide better. Customers can either opt-out of reconciliation or change their implementation to match the new requirements. A customer can opt-out of reconciliation by setting the annotation to "false" before the upgrade. |
There was a problem hiding this comment.
Maybe list the list of CRBs that the customer might need to change and the CLI invocations required to set the annotation on one or more of those resources? This will aid in development of documentation for the proposed change.
There was a problem hiding this comment.
Good idea. Add more content based on your comments. Do you think it is better now?
|
|
||
| ## Alternatives | ||
|
|
||
| An alternate implementation is to add an alert to warn of potentially insecure bootstrap policy with instructions to fix. This would not break existing clusters and ensures that they could fix the problem. |
There was a problem hiding this comment.
You've mentioned some pros of this alternative. Maybe summarize the cons as well?
There was a problem hiding this comment.
Added something that I thought is relevant. Please let me know if that is not sufficient.
vareti
changed the title
vareti
force-pushed
the
anon-user
branch
2 times, most recently
from
5c9e947
to
59770c9
Compare
|
@marun Thanks for taking the time to do a detailed review. I tried to address your comments. Please take a look. |
|
|
||
| ## Summary | ||
|
|
||
| * In earlier releases of OpenShift, the authorization module would allow anonymous users to access discovery endpoint. |
| * This enhancement proposes a way to remove unauthenticated subjects from cluster role bindings after an upgrade from this earlier release. | ||
| * It is possible that a customer is relying on anonymous user behavior in an implementation. | ||
| * This enhancement also proposes a way to opt-out of this reconciliation mechanism. Thus helping cases where changes are not desirable to customer implementations. | ||
| * Later releases of OpenShift removed this access for anonymous users. So, this enhancement would not affect the customers who install a fresh cluster using these later releases. |
There was a problem hiding this comment.
can you link where we changes that? Was either me or Mo, probably in origin repository.
|
|
||
| ### Goals | ||
|
|
||
| > 1. Upgrading an OpenShift cluster removes unauthenticated subjects from cluster role bindings that give access to discovery API. |
| ### Goals | ||
|
|
||
| > 1. Upgrading an OpenShift cluster removes unauthenticated subjects from cluster role bindings that give access to discovery API. | ||
| > 2. Providing user a way to opt-out of this reconciliation. |
| ## Proposal | ||
|
|
||
| * Background | ||
| * A user who does not provide enough details for authentication with the kube-apiserver is considered anonymous and will be part of "system:unauthenticated" group. |
There was a problem hiding this comment.
s/enough details/authentication information (like a client certificate or a bearer token)/
| * A user who does not provide enough details for authentication with the kube-apiserver is considered anonymous and will be part of "system:unauthenticated" group. | ||
| * Both kube-apiserver and openshift-apiserver create a set of cluster roles and cluster role bindings during bootstrap. | ||
| * This [link](https://kubernetes.io/docs/reference/access-authn-authz/rbac/#default-roles-and-role-bindings) has the list of default cluster roles and cluster role bindings in Kubernetes. | ||
| * This [link](https://docs.openshift.com/container-platform/4.4/authentication/using-rbac.html#default-roles_using-rbac) has the list of default cluster roles in OpenShift. |
There was a problem hiding this comment.
would rather prefer a link to the source code. Docs might have diverged.
| * This [link](https://kubernetes.io/docs/reference/access-authn-authz/rbac/#default-roles-and-role-bindings) has the list of default cluster roles and cluster role bindings in Kubernetes. | ||
| * This [link](https://docs.openshift.com/container-platform/4.4/authentication/using-rbac.html#default-roles_using-rbac) has the list of default cluster roles in OpenShift. | ||
| * `rbac.authorization.kubernetes.io/autoupdate=true` annotation is also added to default policy resources. | ||
| * On start-up both openshift-apiserver and kube-apiserver reconcile their default policy resources as follows |
There was a problem hiding this comment.
make it clear what is the current logic and what you propose
| * add annotation to the RBAC resource. | ||
| * add missing permissions to cluster roles. | ||
| * add missing subjects to cluster role bindings. | ||
| * if the annotation is `rbac.authorization.kubernetes.io/autoupdate=false` |
| * An admin to opt-out of reconciliation for a specific resource by setting annotation `rbac.authorization.kubernetes.io/autoupdate=false`. | ||
| * Detailed behavior can be found in this [link](https://kubernetes.io/docs/reference/access-authn-authz/rbac/#auto-reconciliation). | ||
|
|
||
| * Openshift-apiserver operator implements a new controller to the reconcile following cluster role bindings created by openshift-apiserver. |
| ### Risks and Mitigations | ||
|
|
||
| * Because of OpenShift 4.1 allowing access to discovery endpoint, some customer implementations might be expecting this privileged access to anonymous users. Removing unauthenticated subject on an upgrade might break some of these implementations. | ||
| * Informing customers beforehand of this change would help the customer to decide better. Customers can either opt-out of reconciliation or change their implementation to match the new requirements. A customer can opt-out of reconciliation by setting the annotation `rbac.authorization.kubernetes.io/autoupdate` to `false` before the upgrade. |
There was a problem hiding this comment.
@mfojtik @marun @deads2k want to hear your take on this change and the necessary, explicit opt-out, after we had the trouble with the SCCs breaking customers.
If we follow the recommended approach, you'll have to find a way to count anonymous access to these endpoints before trying to tighten it up.
Also, don't have them prevent reconciliation. Instead, suggest that they create their own binding.
There was a problem hiding this comment.
Agree, we need to do to research first, I would like that to be a pre-requirement described in this enhancement and as such include steps how we learn how many clusters in fleet are affected.
There was a problem hiding this comment.
Also, don't have them prevent reconciliation. Instead, suggest that they create their own binding.
I understand that upstream already allows that. But I also hesitate to add this to our resources without a good reason.
|
/close in favor of openshift/openshift-docs#25048 |
|
@vareti: Closed this PR. In response to this:
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. |
This PR proposes a way to fix https://bugzilla.redhat.com/show_bug.cgi?id=1821771