search

openshift / compliance-operator

Operator providing OpenShift cluster compliance checks
Apache License 2.0
110 stars 111 forks source link

Bug 1969620: compliancescan: Fill the <target> element and the urn:xccdf:fact:identifier for node checks #688

Closed jhrozek closed 3 years ago

jhrozek commented 3 years ago

The element ought to contain the hostname of the system being scanned, but for the node scan results produced by oscap-chroot, it was just saying unknown. This is because openscap normally reads /etc/hostname, which is not present on RHCOS.

While there is a bug filed against openscap (https://bugzilla.redhat.com/show_bug.cgi?id=1977668) to read alternative sources, let's work around that issue in the meantime by overriding the

element ourselves during scan results post-processing. Also, because the node host name is not that useful outside baremetal on-prem deployments, let's also put the node name into the with "id=urn:xccdf:fact:identifier". This is done by setting the OSCAP_EVALUATION_TARGET environment variable. But in order to do so, we need to stop using oscap-chroot and start using oscap directly with the OSCAP_PROBE_ROOT variable set to "chroot://" ourselves because the oscap-chroot wrapper script also sets OSCAP_EVALUATION_TARGET to a value on its own. Platform check results are not changed in any way, because it is not possible to find the cluster name internally, without having the client context (see e.g. https://github.com/kubernetes/kubernetes/issues/44954) Related: [OCPBUGSM-30237](https://issues.redhat.com/browse/OCPBUGSM-30237)
jhrozek commented 3 years ago

@mrogers950 do you know of a way to get the cluster name from inside the cluster? It looks like pure k8s doesn't do that (https://github.com/kubernetes/kubernetes/issues/44954) but maybe there's some openshift API?

jhrozek commented 3 years ago

uhm.... we could pass in the client context, the name should be there as far as I remember.

sorry, which client context do you mean? I tried looking into config e.g.:

cfg, err := config.GetConfig()
cfg.Name

but that only holds the IP address of the API server.

JAORMX commented 3 years ago

uhm.... we could pass in the client context, the name should be there as far as I remember.

sorry, which client context do you mean? I tried looking into config e.g.:

cfg, err := config.GetConfig()
cfg.Name

but that only holds the IP address of the API server.

oh! I actually thought that exact key would have had the URL of the API Server. Nevermind then.

jhrozek commented 3 years ago

There was a stupid bug in the patch that I've fixed. I still haven't added any new tests.

JAORMX commented 3 years ago

@jhrozek so... do we have a plan to address the target for platform checks? or are we planning to leave those as they are?

jhrozek commented 3 years ago

On Fri, Aug 27, 2021 at 12:30:53AM -0700, Juan Osorio Robles wrote:

@jhrozek so... do we have a plan to address the target for platform checks? or are we planning to leave those as they are?

Maybe, but honestly I think it's not worth it. I took another look and "oc cluster-info" looks at restConfig's Host, but that appears to be giving a different result in cluster (an IP address, e.g. https://172.30.0.1:443 for me, same as the KUBERNETES_PORT variable).

There is also the openshiftapiservers/cluster object which has a .spec.observedConfig.routingConfig.subdomain, but 1) that one is not readable by the compliance-operator SA, so we'd have to extend the compliance-operator SA permissions and 2) the routingConfig.subdomain URL is slightly different, e.g. for me the API server runs at https://api.jhrozek-cluster-08-27-13-42.devcluster.openshift.com:6443 but the routing subdomain is apps.jhrozek-cluster-08-27-13-42.devcluster.openshift.com so I'm not even sure how useful that would be.

In the end, it would provide /some/ identification in the report, but do you think it's worth opening the permissions for?

The api-resource-collector SA does have the permissions to read that object, so I guess we could dump the info somewhere and read it during post-processing of the results..

JAORMX commented 3 years ago

@jhrozek thanks for checking into this... I think it's not worth the effort as you put it. We might be introducing too much tech debt for something that might not bring as much value. Is this good to go as-is?

jhrozek commented 3 years ago

@jhrozek thanks for checking into this... I think it's not worth the effort as you put it. We might be introducing too much tech debt for something that might not bring as much value. Is this good to go as-is?

I think yes, the only question is about tests, but I could add them in a separate PR as well. The intent being that the scan produces the expected number of CMs and a PV with some files.

JAORMX commented 3 years ago

/approve /lgtm

openshift-ci[bot] commented 3 years ago

[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
openshift-ci[bot] commented 3 years ago

@jhrozek: This pull request references Bugzilla bug 1969620, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/compliance-operator/pull/688): >Bug 1969620: compliancescan: Fill the element and the urn:xccdf:fact:identifier for node checks Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-ci[bot] commented 3 years ago

@jhrozek: This pull request references Bugzilla bug 1969620, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/compliance-operator/pull/688): >Bug 1969620: compliancescan: Fill the element and the urn:xccdf:fact:identifier for node checks Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
JAORMX commented 3 years ago

/bugzilla refresh

JAORMX commented 3 years ago

/bugzilla refresh

openshift-ci[bot] commented 3 years ago

@JAORMX: This pull request references Bugzilla bug 1969620, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target release (4.9.0) matches configured target release for branch (4.9.0) * bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (pdhamdhe@redhat.com), skipping review request.

In response to [this](https://github.com/openshift/compliance-operator/pull/688#issuecomment-908142745): >/bugzilla refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-ci[bot] commented 3 years ago

@JAORMX: This pull request references Bugzilla bug 1969620, which is valid.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target release (4.9.0) matches configured target release for branch (4.9.0) * bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (pdhamdhe@redhat.com), skipping review request.

In response to [this](https://github.com/openshift/compliance-operator/pull/688#issuecomment-908142299): >/bugzilla refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-ci[bot] commented 3 years ago

@jhrozek: All pull requests linked via external trackers have merged:

Bugzilla bug 1969620 has been moved to the MODIFIED state.

In response to [this](https://github.com/openshift/compliance-operator/pull/688): >Bug 1969620: compliancescan: Fill the element and the urn:xccdf:fact:identifier for node checks Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.