Skip to content

updating PodSecurityContext and SecurityContext for scorecard pod#6294

Merged
everettraven merged 1 commit into
operator-framework:masterfrom
acornett21:update_scorecard_for_psa
Feb 15, 2023
Merged

updating PodSecurityContext and SecurityContext for scorecard pod#6294
everettraven merged 1 commit into
operator-framework:masterfrom
acornett21:update_scorecard_for_psa

Conversation

@acornett21

@acornett21 acornett21 commented Feb 7, 2023

Copy link
Copy Markdown
Contributor

Description of the change:
Scorecard tests run in the default namespace, this change adds in all the necessary attributes in order to run in the default namespace on clusters that enforce PSA.

Motivation for the change:
To have scorecard work in new clusters that enforce PSA, and have any tooling that uses scorecard as a dependency also work in clusters that enforce PSA.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Signed-off-by: Adam D. Cornett adc@redhat.com

…tainers

Signed-off-by: Adam D. Cornett <adc@redhat.com>
@acornett21 acornett21 temporarily deployed to deploy February 7, 2023 19:36 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy February 7, 2023 19:36 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy February 7, 2023 19:36 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy February 7, 2023 19:36 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy February 7, 2023 19:36 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy February 7, 2023 19:36 — with GitHub Actions Inactive
@acornett21 acornett21 temporarily deployed to deploy February 7, 2023 19:36 — with GitHub Actions Inactive

@everettraven everettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acornett21 Thanks for the contribution! I have a general concern, but I am okay with accepting this as is with the idea that there will be a follow up issue for creating our own images that run as a non-root user by default.

/lgtm

Comment on lines +230 to +231
podSecCtx.RunAsUser = &[]int64{1000}[0]
podSecCtx.RunAsGroup = &[]int64{1000}[0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit concerned about adding the RunAsUser and the RunAsGroup fields long term. I think there are some known issues regarding PSA on OpenShift when setting RunAsUser. I also think in general it is preferred that the containers that we are running don't run as root in the first place.

I recall some previous discussion that we want to explore creating our own maintained images that don't run as root by default. This would make it so we don't even need to set the RunAsUser field. That being said, I am personally okay with accepting this as is until we get to that point so that Scorecard is usable for the time being.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is making sure that the container's don't run as root, and run as a specific user. User 0 is root, this is running as user 1000 which is perfectly acceptable. The issue is that PSA works differently in the default namespace then it does in other namespaces, and since scorecard runs in default all of this needs to be added in order to support that namespace.

Now if scorecard ran in some random namespace, these additions are less likely to be needed based on my testing. However, there are many tools that use operator-sdk as a dependency, and I think rolling back the default namespace would be a breaking change for many users, as I mentioned in the issues.

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2023
@everettraven

Copy link
Copy Markdown
Contributor

Looks like the sanity/docs checking is failing on every PR due to some actual dead links. I won't hold this PR on that and will create a separate one to fix these link failures. Going to go ahead and merge this in.

@everettraven everettraven merged commit 8f06eb8 into operator-framework:master Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scorecard container is not following PSA properly

2 participants