Add permission for checking the kubeadmin user

[JAORMX]

This adds the necessary RBAC permission for checking if the kubeadmin secret exists. The intent is to verify that it has been removed, as done in the accompanying CaC PR [1]. This is possible thanks to the recent addition that enables us to persist if kube returned NotFound for a specific object [2].

[1] https://github.com/ComplianceAsCode/content/pull/7723 [2] Commit f200ae00f32ea29670cfa67ee802109b0a0709e2

Signed-off-by: Juan Antonio Osorio Robles jaosorior@redhat.com

[jhrozek]

From the functional point of view the rule along with the content works and detects that I have kubeadmin on my cluster.

What I want to have a second opinion on from @mrogers950 and @Vincent056 is if it's OK to read secrets in the api-resource-collector and if some precautions would make sense.

My opinion is that we would need to add these rules sooner or later because our users and customers simply expect the checks and remediations to work out of the box. So what I'm wondering about is if there is something we can do to make this safer. I wish RBAC had an "access" permission to just check if an object exists, but it doesn't. We could emulate that by modifying api-resource-collector to throw away contents of secrets and just dump placeholder files for any secrets API resources, but I'm not sure this accomplishes much -- it would at best prevent someone with access to the namespace from just looking at the API resource dumps. But if someone has access to the namespace and thus the ability to use the api-resource-collector SA, then they automatically have the permissions to read those secrets. So I'm not sure this truncation of secret payloads accomplishes much. Also, at some point we might /want/ to inspect the secrets' bodies (check certificates for whatever..)

In the end, I would probably accept this PR and just let's make sure we always only allow specific resources by name.

Thoughts?

[Vincent056]

From the functional point of view the rule along with the content works and detects that I have kubeadmin on my cluster.

What I want to have a second opinion on from @mrogers950 and @Vincent056 is if it's OK to read secrets in the api-resource-collector and if some precautions would make sense.

My opinion is that we would need to add these rules sooner or later because our users and customers simply expect the checks and remediations to work out of the box. So what I'm wondering about is if there is something we can do to make this safer. I wish RBAC had an "access" permission to just check if an object exists, but it doesn't. We could emulate that by modifying api-resource-collector to throw away contents of secrets and just dump placeholder files for any secrets API resources, but I'm not sure this accomplishes much -- it would at best prevent someone with access to the namespace from just looking at the API resource dumps. But if someone has access to the namespace and thus the ability to use the api-resource-collector SA, then they automatically have the permissions to read those secrets. So I'm not sure this truncation of secret payloads accomplishes much. Also, at some point we might /want/ to inspect the secrets' bodies (check certificates for whatever..)

In the end, I would probably accept this PR and just let's make sure we always only allow specific resources by name.

Thoughts?

Can we create the kubeadmin secret with the same name, since it won't be allowed to create if there is an existing kubeadmin secret, we will know if there is kubeadmin secret without getting it

[JAORMX]

/retest

[JAORMX]

Can we create the kubeadmin secret with the same name, since it won't be allowed to create if there is an existing kubeadmin secret, we will know if there is kubeadmin secret without getting it

@Vincent056 I'm not sure this is something we can do a check for in SCAP. Did can you elaborate on this?

[jhrozek]

On Tue, Oct 12, 2021 at 12:02:16AM -0700, Juan Osorio Robles wrote:

Can we create the kubeadmin secret with the same name, since it won't be allowed to create if there is an existing kubeadmin secret, we will know if there is kubeadmin secret without getting it

@Vincent056 I'm not sure this is something we can do a check for in SCAP. Did can you elaborate on this?

It is a good idea in general and the usual way to get rid of TOCTOU conditions when handling files. So with a file, instead of checking with access() if a file exists and then calling open(O_CREAT) to create the file, you'd just call open and handle the errors as appropriate.

I guess we could use this in a way (special casing the handling of secrets inside the api-resource-collector), but it still comes with a similar caveat as the read-then-null approach: anyone with access to the namespace would have create permissions for the secrets we enumerate. In case of secrets that are /not/ supposed to be there (like kubeadmin) I would argue this is also dangerous, because then someone can create a secret with their contents.

But I'd like to hear what others think, because it can also be argued that creating a secret should ring a bell and be audited better than reading a secret, so maybe it's worth exploring. At the same time, the idea of giving the operator write access to the secrets gives me the chills, even if 'just' create and not update.

[JAORMX]

/assign @mrogers950

[Vincent056]

On Tue, Oct 12, 2021 at 12:02:16AM -0700, Juan Osorio Robles wrote: > Can we create the kubeadmin secret with the same name, since it won't be allowed to create if there is an existing kubeadmin secret, we will know if there is kubeadmin secret without getting it @Vincent056 I'm not sure this is something we can do a check for in SCAP. Did can you elaborate on this? It is a good idea in general and the usual way to get rid of TOCTOU conditions when handling files. So with a file, instead of checking with access() if a file exists and then calling open(O_CREAT) to create the file, you'd just call open and handle the errors as appropriate. I guess we could use this in a way (special casing the handling of secrets inside the api-resource-collector), but it still comes with a similar caveat as the read-then-null approach: anyone with access to the namespace would have create permissions for the secrets we enumerate. In case of secrets that are /not/ supposed to be there (like kubeadmin) I would argue this is also dangerous, because then someone can create a secret with their contents. But I'd like to hear what others think, because it can also be argued that creating a secret should ring a bell and be audited better than reading a secret, so maybe it's worth exploring. At the same time, the idea of giving the operator write access to the secrets gives me the chills, even if 'just' create and not update.

Yeah, I think that's a good idea. Will allow people to create secret also compromise the cluster in some ways?

[mrogers950]

There's a comment above the ClusterRole that mentions it's a copy of cluster-reader, we should change that too since it has diverged.

On Tue, Oct 12, 2021 at 12:02:16AM -0700, Juan Osorio Robles wrote: > Can we create the kubeadmin secret with the same name, since it won't be allowed to create if there is an existing kubeadmin secret, we will know if there is kubeadmin secret without getting it @Vincent056 I'm not sure this is something we can do a check for in SCAP. Did can you elaborate on this? It is a good idea in general and the usual way to get rid of TOCTOU conditions when handling files. So with a file, instead of checking with access() if a file exists and then calling open(O_CREAT) to create the file, you'd just call open and handle the errors as appropriate. I guess we could use this in a way (special casing the handling of secrets inside the api-resource-collector), but it still comes with a similar caveat as the read-then-null approach: anyone with access to the namespace would have create permissions for the secrets we enumerate. In case of secrets that are /not/ supposed to be there (like kubeadmin) I would argue this is also dangerous, because then someone can create a secret with their contents. But I'd like to hear what others think, because it can also be argued that creating a secret should ring a bell and be audited better than reading a secret, so maybe it's worth exploring. At the same time, the idea of giving the operator write access to the secrets gives me the chills, even if 'just' create and not update.

We shouldn't need to worry about the re-creation of kubeadmin from a practical security angle because once you delete the kubeadmin secret once the user is disabled (if you try re-creating kubeadmin secret you'll see). Though I'm not sure I follow the need for creation permissions. As long as the collector's SA exists in the namespace, using the permissions is possible by any other pod in the namespace, to my knowledge there's not really a suitable trick around that. The danger is primarily access to the SA by other workloads, not so much what the api-collector knows about the secret when it's looking (in our case, just a GET to check existence and no other processing).

Also, having secrets named kubeadmin in other namespaces would be weird. It might be good to flag on that anyways. In any cluster there's only one real kubeadmin secret. It could indicate that the secret was copied to another namespace, perhaps.

[JAORMX]

Pull-request updated, HEAD is now 2ea74deef21a100af252fec73045125d56c5dba7

[JAORMX]

There's a comment above the ClusterRole that mentions it's a copy of cluster-reader, we should change that too since it has diverged.

Done

[openshift-ci[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, jhrozek

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: - ~~[OWNERS](https://github.com/openshift/compliance-operator/blob/master/OWNERS)~~ [JAORMX,jhrozek] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment