Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPBUGS-24653: Ensure FIPS compliance for operator image #133

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

alebedev87
Copy link
Contributor

@alebedev87 alebedev87 commented Mar 26, 2024

This commit addresses all the errors detected by the check-payload tool by implementing the following changes:

  • Replaced the base image with a non-UBI variant.
  • Updated the builder image to be FIPS-capable.
  • Enabled dynamic linkage for the operator binary (CGO_ENABLED=1).
  • Added the strictfipsruntime tag to the operator binary.

Additionally, implemented image-fips-scan command for local FIPS compliance checks.

Result:

$ IMG=quay.io/alebedev/albo make image-fips-scan
sudo podman run --privileged registry.ci.openshift.org/ci/check-payload:latest scan operator --spec quay.io/alebedev/albo
I0326 08:50:55.098182       1 main.go:302] using embedded config
...
I0326 08:50:55.098589       1 main.go:102] "scan" version="0.3.1-91-ga0d1e0d3-dirty"
---- Successful run

@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 26, 2024
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-24653, which is invalid:

  • expected the bug to target the "4.16.0" version, but no target version was set

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

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This commit addresses all the errors detected by the check-payload tool by implementing the following changes:

  • Replaced the base image with a non-UBI variant.
  • Updated the builder image to be FIPS-capable.
  • Enabled dynamic linkage for the operator binary (CGO_ENABLED=1).
  • Added the strictfipsruntime tag to the operator binary.

Additionally, implemented image-fips-scan command for local FIPS compliance checks.

Result:

$ make image-fips-scan
sudo podman run --privileged registry.ci.openshift.org/ci/check-payload:latest scan operator --spec quay.io/alebedev/albo
I0326 08:50:55.098182       1 main.go:302] using embedded config
...
I0326 08:50:55.098589       1 main.go:102] "scan" version="0.3.1-91-ga0d1e0d3-dirty"
---- Successful run

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 openshift-eng/jira-lifecycle-plugin repository.

@alebedev87
Copy link
Contributor Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 26, 2024
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-24653, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.16.0) matches configured target version for branch (4.16.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-24653, which is valid.

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

Requesting review from QA contact:
/cc @lihongan

In response to this:

This commit addresses all the errors detected by the check-payload tool by implementing the following changes:

  • Replaced the base image with a non-UBI variant.
  • Updated the builder image to be FIPS-capable.
  • Enabled dynamic linkage for the operator binary (CGO_ENABLED=1).
  • Added the strictfipsruntime tag to the operator binary.

Additionally, implemented image-fips-scan command for local FIPS compliance checks.

Result:

$ IMG=quay.io/alebedev/albo make image-fips-scan
sudo podman run --privileged registry.ci.openshift.org/ci/check-payload:latest scan operator --spec quay.io/alebedev/albo
I0326 08:50:55.098182       1 main.go:302] using embedded config
...
I0326 08:50:55.098589       1 main.go:102] "scan" version="0.3.1-91-ga0d1e0d3-dirty"
---- Successful run

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 openshift-eng/jira-lifecycle-plugin repository.

@alebedev87
Copy link
Contributor Author

/test e2e-aws-rosa-operator

2 similar comments
@alebedev87
Copy link
Contributor Author

/test e2e-aws-rosa-operator

@alebedev87
Copy link
Contributor Author

/test e2e-aws-rosa-operator

@Miciah
Copy link
Contributor

Miciah commented Mar 27, 2024

/assign
/approve
/lgtm
/hold
for @gcs278 to review.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024
@candita
Copy link

candita commented Mar 27, 2024

/assign @gcs278
/assign @Miciah

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024
Copy link
Contributor

openshift-ci bot commented Mar 27, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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:

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 Mar 27, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024
@alebedev87
Copy link
Contributor Author

Dockerfile: use 1.19 go-toolset version instead of latest.

Copy link

@gcs278 gcs278 left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple nit picks.

Makefile Outdated
@@ -189,10 +191,14 @@ image-build: build test ## Build container image with the manager.
image-push: ## Push container image with the manager.
${CONTAINER_ENGINE} push ${IMG}

.PHONY: image-fips-scan
image-fips-scan: image-build image-push
sudo $(CONTAINER_ENGINE) run --privileged $(CHECK_PAYLOAD_IMG) scan operator --spec $(IMG)
Copy link

Choose a reason for hiding this comment

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

Do we need the sudo? I noticed the other $(CONTAINER_ENGINE) commands don't have that, that seems a bit odd. Did you need that for your local machine?

Is this new command just for troubleshooting? Or will a downstream CI job be using this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need the sudo?

Yes, otherwise it gives the following error:

podman error (args=[pull quay.io/alebedev/albo]) (stderr=Error: filesystem type 0x65735546 reported for /var/lib/containers/storage is not supported with 'overlay': backing file system is unsupported for this graph driver | quay.io/alebedev/albo

In their examples they use sudo too. I didn't want to dig too deep to find out why.

Is this new command just for troubleshooting?

Yes.

Or will a downstream CI job be using this?

No, in the CI we will need to do something similar to this.

Copy link

Choose a reason for hiding this comment

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

Yes, otherwise it gives the following error:

Hmm. If you can run the image-build and the image-push command with rootless podman, there's no reason you shouldn't be able to run this command with podman. My point being these should be consistent.

Maybe the $() is causing issues, the other commands use ${}? Try that:

Suggested change
sudo $(CONTAINER_ENGINE) run --privileged $(CHECK_PAYLOAD_IMG) scan operator --spec $(IMG)
${CONTAINER_ENGINE} run --privileged $(CHECK_PAYLOAD_IMG) scan operator --spec $(IMG)

In their examples they use sudo too. I didn't want to dig too deep to find out why.

They use sudo on the ./check-payload command, not on the podman command.

It seems like in our other repos, we don't use sudo, like https://github.com/openshift/cluster-ingress-operator/blob/1217332a5a4e64b1ca333f817fa66bb6aae310ab/hack/release-local.sh#L26 or https://github.com/openshift/cluster-dns-operator/blob/afb52de2b4e18c2ed35f1bdb01f24a41a5efafc3/Makefile#L75.

No, in the CI we will need to do something similar to this.

Ack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. If you can run the image-build and the image-push command with rootless podman, there's no reason you shouldn't be able to run this command with podman. My point being these should be consistent.

Agree about the consistency and I would prefer to have it but check-payload is not a regular binary, it runs podman commands on its own (podman pull and others). So this turns into a "podman inside podman" kind of problem. May be there is a right combination of flags which would allow us to get rid of sudo but I didn't manage to find it quickly. I saw that microshift uses sudo in their tests too when they run check-payload as an image.

Maybe the $() is causing issues, the other commands use ${}?

$() is the standard Makefile syntax, equivalent to ${}. I often use $() as it looks to me more "Makefile native".

In their examples they use sudo too. I didn't want to dig too deep to find out why.

They use sudo on the ./check-payload command, not on the podman command.

Yes, true. I was running check-payload built locally from the source code. Then I started looking for how to use their image to avoid referencing a third party tool in the make rule.

Copy link

Choose a reason for hiding this comment

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

$() is the standard Makefile syntax, equivalent to ${}. I often use $() as it looks to me more "Makefile native".

Ah yea sorry this isn't bash...Ack.

I think your laptop is having issues with the podman run command as rootless. Try out podman run docker.io/hello-world as an example. It looks like you can successfully run podman build and podman push with the other make commands. Might want to try a podman system reset to reset the storage to clear everything.

So you don't need sudo to run this container (or any container) with podman, works fine for me.

But it's a nit pick and I won't hold up the PR over it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you don't need sudo to run this container (or any container) with podman, works fine for me.

Does this command works fine on your machine?

podman run --privileged registry.ci.openshift.org/ci/check-payload:latest scan operator --spec quay.io/alebedev/albo:latest

Copy link

Choose a reason for hiding this comment

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

Yes sir, works fine for me.

$ podman run --privileged registry.ci.openshift.org/ci/check-payload:latest scan operator --spec quay.io/alebedev/albo:latest
I0328 22:18:36.752612       1 main.go:302] using embedded config
I0328 22:18:36.753011       1 types_config.go:12] using config &{Components:[] FailOnWarnings:false FilterFile: FromFile: FromURL: InsecurePull:false Limit:-1 ContainerImageComponent: ContainerImage: OutputFile: OutputFormat:table Parallelism:5 Java:false PrintExceptions:false PullSecret: TimeLimit:1h0m0s Verbose:false UseRPMScan:false ConfigFile:{FilterFiles:[] FilterDirs:[/lib/firmware /lib/modules /usr/lib/.build-id /usr/lib/firmware /usr/lib/grub /usr/lib/modules /usr/share/app-info /usr/share/doc /usr/share/fonts /usr/share/icons /usr/share/openshift /usr/src/plugins /rootfs /sysroot] FilterImages:[] JavaDisabledAlgorithms:[DH keySize < 2048 TLSv1.1 TLSv1 SSLv3 SSLv2 TLS_RSA_WITH_AES_256_CBC_SHA256 TLS_RSA_WITH_AES_256_CBC_SHA TLS_RSA_WITH_AES_128_CBC_SHA256 TLS_RSA_WITH_AES_128_CBC_SHA TLS_RSA_WITH_AES_256_GCM_SHA384 TLS_RSA_WITH_AES_128_GCM_SHA256 DHE_DSS RSA_EXPORT DHE_DSS_EXPORT DHE_RSA_EXPORT DH_DSS_EXPORT DH_RSA_EXPORT DH_anon ECDH_anon DH_RSA DH_DSS ECDH 3DES_EDE_CBC DES_CBC RC4_40 RC4_128 DES40_CBC RC2 HmacMD5] PayloadIgnores:map[openshift-enterprise-pod-container:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrNotDynLinked Files:[/usr/bin/pod] Dirs:[]}]} operator-lifecycle-manager-container:{FilterFiles:[/usr/bin/cpb /usr/bin/copy-content] FilterDirs:[] ErrIgnores:[]} ose-olm-rukpak-container:{FilterFiles:[/unpack] FilterDirs:[] ErrIgnores:[]}] TagIgnores:map[] RPMIgnores:map[containernetworking-plugins:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrGoMissingTag Files:[] Dirs:[/usr/libexec/cni]}]} cri-o:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrGoMissingTag Files:[/usr/bin/crio /usr/bin/crio-status] Dirs:[]} {Error:ErrNotDynLinked Files:[/usr/bin/pinns] Dirs:[]}]} cri-tools:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrGoMissingTag Files:[/usr/bin/crictl] Dirs:[]}]} glibc:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrNotDynLinked Files:[/usr/sbin/ldconfig /sbin/ldconfig] Dirs:[]}]} glibc-common:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrNotDynLinked Files:[/usr/sbin/build-locale-archive] Dirs:[]}]} ignition:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrGoMissingTag Files:[/usr/lib/dracut/modules.d/30ignition/ignition] Dirs:[]}]} podman:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrGoMissingTag Files:[/usr/bin/podman /usr/libexec/podman/quadlet /usr/libexec/podman/rootlessport] Dirs:[]} {Error:ErrNotDynLinked Files:[/usr/libexec/podman/catatonit] Dirs:[]}]} podman-catatonit:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrNotDynLinked Files:[/usr/libexec/catatonit/catatonit] Dirs:[]}]} runc:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrGoMissingTag Files:[/usr/bin/runc] Dirs:[]}]} skopeo:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrGoMissingTag Files:[/usr/bin/skopeo] Dirs:[]}]} tini:{FilterFiles:[] FilterDirs:[] ErrIgnores:[{Error:ErrNotDynLinked Files:[/usr/bin/tini-static] Dirs:[]}]}] ErrIgnores:[]}}
I0328 22:18:36.753076       1 main.go:102] "scan" version="0.3.1-91-ga0d1e0d3-dirty"
---- Successful run

Does podman run docker.io/hello-world work for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does podman run docker.io/hello-world work for you?

Yes.

I start to blame this shortcoming of rootless podman:

Support for using native overlayfs as an unprivileged user is only available for Podman version >= 3.1 on a Linux kernel version >= 5.12

As I still use the RHEL CBS (kernel 4.18.0). Mostly probably I'm the only one with the kernel that old. I have to migrate to the Fedora CBD anyway. So, I think that removing sudo makes sense for the future use indeed.

Copy link

Choose a reason for hiding this comment

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

That's odd you can run docker.io/hello-world fine. Maybe it's the --privileged flag causing issues.

@@ -189,10 +191,14 @@ image-build: build test ## Build container image with the manager.
image-push: ## Push container image with the manager.
${CONTAINER_ENGINE} push ${IMG}

.PHONY: image-fips-scan
image-fips-scan: image-build image-push
Copy link

Choose a reason for hiding this comment

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

nit Do you need to do build and push in this step?

I had a bit of pain with this, because to build and push I use different registry auth files (one that can pull the images from registry.ci.openshift.org, another that can push to quay.io under my user).

So I had to work around this by doing the build and push ahead of time.

Maybe it's just my particular case was painful, so it's not that big of a deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you need to do build and push in this step?

Yes, check-payload needs the image to be pullable.

I had a bit of pain with this, because to build and push I use different registry auth files (one that can pull the images from registry.ci.openshift.org, another that can push to quay.io under my user).

Interesting. I'm logged to my quay.io using podman login. The CI image registry auth is saved to /run/user/${id}/containers/auth.json thanks to oc registry login command. And they don't interfere on my machine.

Copy link

Choose a reason for hiding this comment

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

Yea this sounds like a personal problem I have with managing podman auth files. I should combine them.

@alebedev87
Copy link
Contributor Author

e2e-aws-rosa-operator job is failing because the ROSA is experiencing a problem with 4.16 release candidate.

@gcs278
Copy link

gcs278 commented Mar 28, 2024

Thanks!
/unhold
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 28, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD da08aa2 and 2 for PR HEAD 3fa0112 in total

This commit addresses the bug by implementing the following changes:
- Replaced the base image with a non-UBI variant.
- Updated the builder image to be FIPS-capable.
- Enabled dynamic linkage for the operator binary (CGO_ENABLED=1).
- Added the 'strictfipsruntime' tag to the operator binary.

Additionally, implemented 'image-fips-scan' command for local FIPS compliance checks.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2024
@alebedev87
Copy link
Contributor Author

/test e2e-aws-proxy-operator

@alebedev87
Copy link
Contributor Author

/retest

@gcs278
Copy link

gcs278 commented Mar 29, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD da08aa2 and 2 for PR HEAD 8747bf6 in total

@alebedev87
Copy link
Contributor Author

alebedev87 commented Mar 29, 2024

/override ci/prow/e2e-aws-rosa-operator

ROSA is having an issue integrating the candidate OCP 4.16

Copy link
Contributor

openshift-ci bot commented Mar 29, 2024

@alebedev87: Overrode contexts on behalf of alebedev87: ci/prow/e2e-aws-rosa-operator

In response to this:

/override ci/prow/e2e-aws-rosa-operator

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 2b9bd9d into openshift:main Mar 29, 2024
8 checks passed
@openshift-ci-robot
Copy link

@alebedev87: Jira Issue OCPBUGS-24653: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-24653 has been moved to the MODIFIED state.

In response to this:

This commit addresses all the errors detected by the check-payload tool by implementing the following changes:

  • Replaced the base image with a non-UBI variant.
  • Updated the builder image to be FIPS-capable.
  • Enabled dynamic linkage for the operator binary (CGO_ENABLED=1).
  • Added the strictfipsruntime tag to the operator binary.

Additionally, implemented image-fips-scan command for local FIPS compliance checks.

Result:

$ IMG=quay.io/alebedev/albo make image-fips-scan
sudo podman run --privileged registry.ci.openshift.org/ci/check-payload:latest scan operator --spec quay.io/alebedev/albo
I0326 08:50:55.098182       1 main.go:302] using embedded config
...
I0326 08:50:55.098589       1 main.go:102] "scan" version="0.3.1-91-ga0d1e0d3-dirty"
---- Successful run

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Mar 29, 2024

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

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.

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. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants