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.

Sign up for GitHub

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

ROX-24283: enable strictfipsruntime in the Konflux build #1709

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+4 −1
Open

ROX-24283: enable strictfipsruntime in the Konflux build #1709

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c51901
ROX-24283: enable strictfipsruntime in Konflux build
BradLugo Nov 15, 2024
3f58edc
PR feedback
BradLugo Nov 15, 2024
22873cf
PR feedback
BradLugo Nov 19, 2024
5b3e249
Update Makefile
BradLugo Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ GOPATH_WD_OVERRIDES := -w /src -e GOPATH=/go
IMAGE_BUILD_FLAGS := -e CGO_ENABLED=$(CGO_ENABLED) -e GOOS=linux -e GOARCH=$(GOARCH)
IMAGE_BUILD_ARGS = --build-arg LABEL_VERSION=$(TAG) --build-arg LABEL_RELEASE=$(TAG) --build-arg QUAY_TAG_EXPIRATION=$(QUAY_TAG_EXPIRATION)
BUILD_FLAGS := CGO_ENABLED=$(CGO_ENABLED) GOOS=linux GOARCH=$(GOARCH)
BUILD_CMD := go build -trimpath -ldflags="-X github.com/stackrox/scanner/pkg/version.Version=$(TAG)" -o image/scanner/bin/scanner ./cmd/clair
BUILD_CMD := go build -trimpath -ldflags="-X github.com/stackrox/scanner/pkg/version.Version=$(TAG)" -tags=$(GOTAGS) -o image/scanner/bin/scanner ./cmd/clair
BradLugo marked this conversation as resolved.
Show resolved Hide resolved
NODESCAN_BUILD_CMD := go build -trimpath -o tools/bin/local-nodescanner ./tools/local-nodescanner
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not seem to me that local-nodescanner is included in the final Scanner image. Is this correct?

Is it a development tool? Does anyone still use it?
@Maddosaurus

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a development tool. From what I can tell, the (old) RHCOS node scanner runs as a container in the collector pod called node-inventory which runs the usual scanner binary /scanner from within the usual scanner image ._rox.image.scanner.fullRef https://github.com/stackrox/stackrox/blob/master/image/templates/helm/stackrox-secured-cluster/templates/collector.yaml.htpl#L196

Looks like the typical scanner main function was modified to run the scanner in "node inventory mode" - https://github.com/stackrox/scanner/blob/master/cmd/clair/main.go#L279

So I'd say it's accurate to keep this command out

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is correct to not be included in the image.
local-nodescanner is a tool that enables local testing and debugging of Scanner v2 Node Scanning code without the need to deploy ACS.
It is similar to the tools for collector and sensor we have over in the main stackrox repo 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you Ross and Matthias, while you answered about FIPS part for the local-nodescanner, it will be good to know whether we need to keep and maintain this tool. Therefore:

Does anyone still use it?

Is there any value to keep it?

Copy link
Member

Choose a reason for hiding this comment

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

As long as we support Scanner V2 Node Scanning, this tool should stay in place to enable effective debugging and troubleshooting of Node Scanning code.


Copy link
Contributor

Choose a reason for hiding this comment

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

(not about this line) Similarly there's updater which does not seem to be included in the image and so doesn't need to be FIPS-built. Is it correct?

scanner/Makefile

Lines 117 to 120 in ff7e0f3

.PHONY: build-updater
build-updater: deps
@echo "+ $@"
go build -trimpath -o ./bin/updater ./cmd/updater

Copy link
Collaborator

Choose a reason for hiding this comment

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

This updater just runs in CI and isn't shipped to customers, so no need to be FIPS-built

#####################################################################
Expand Down
3 changes: 3 additions & 0 deletions image/scanner/rhel/konflux.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ ARG SCANNER_TAG
RUN if [[ "$SCANNER_TAG" == "" ]]; then >&2 echo "error: required SCANNER_TAG arg is unset"; exit 6; fi
ENV RELEASE_TAG="${SCANNER_TAG}"

# TODO(ROX-27054): Remove the redundant strictfipsruntime option if one is found to be so
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love the way this comment reads, but it's the best I could come up with.

Copy link

Choose a reason for hiding this comment

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

lgtm. I'd prefer "remove -tags=strictfipsruntime when GOEXPERIMENT is checked by check-payload" but that may be too much information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly I think I prefer the more info in David's suggestion (perhaps even add a link to check-payload so readers can find it)

ENV GOEXPERIMENT=strictfipsruntime
ENV GOTAGS=strictfipsruntime
Comment on lines +14 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot: do these both GOEXPERIMENT=strictfipsruntime env variable and -tags=strictfipsruntime argument need to be provided at the same time?

Copy link

Choose a reason for hiding this comment

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

I think we do not want the tag set because "The -tag method can technically work on 1.17+, but the environment variable method is preferred." (https://docs.google.com/presentation/d/10O9MIItuPW1fCN4dPaGTxSHAE4KhqISMKAxNGdmPyV4/edit#slide=id.g227165bb50a_0_0)

check-payload accepts the tag for golang <=1.16 and goexperiment var for golang >=1.17 openshift/check-payload#143 (comment)
🤔 The code appears to check for tags on golang < 1.18. So 1.17 may be incorrectly checked, but we're not concerned with 1.17 since we're on 1.20+ (https://github.com/openshift/check-payload/pull/143/files#diff-e8b02dd31a1900ab2922d2408ab4727fdc85e7cafe302133d125ec0a8062efd6R114)

discussion in https://issues.redhat.com/browse/OCPBUGS-24612
and that matches the slides (https://docs.google.com/presentation/d/10O9MIItuPW1fCN4dPaGTxSHAE4KhqISMKAxNGdmPyV4/edit#slide=id.g227165bb50a_0_0):

For Go 1.17+:
As an environment variable: "GOEXPERIMENT=strictfipsruntime" 
Setting this environment variable is preferred since it can be done trivially at build time without being concerned with each go build invocation.
For Go 1.16 or prior:
As a -tags parameter "-tags=strictfipsruntime" .
The environment variable method is not supported. The -tag method can technically work on 1.17+, but the environment variable method is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my reading of the info you shared, for us it should be sufficient to only keep GOEXPERIMENT=strictfipsruntime environment variable. Since you achieved the state when check-payload is happy about our images, could you please try removing -tags=strictfipsruntime/GOTAGS=strictfipsruntime and running the tool against the results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing this in stackrox/stackrox#12909, but only setting GOEXPERIMENT didn't move the needle (hence this commit: stackrox/stackrox@0e094ea)

Copy link

Choose a reason for hiding this comment

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

Perhaps the GOEXPERIMENT variable is not getting carried through and we're relying on the tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird. If -tags=strictfipsruntime/GOTAGS=strictfipsruntime has the effect, maybe we don't need GOEXPERIMENT=strictfipsruntime?

Copy link

Choose a reason for hiding this comment

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

It looks like GOEXPERIMENT is not found on the built binaries. So I think that is why the tags are needed right now.

Copy link

Choose a reason for hiding this comment

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

I'll try checking in a way separate from check-payload. Maybe check-payload is wrong.

Copy link

Choose a reason for hiding this comment

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

I'll try checking in a way separate from check-payload. Maybe check-payload is wrong.

This. I'll report the issue to check-payload.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the conclusion of this? Just keeping both?

Comment on lines +14 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @davdhacs suggestion to add a # TODO(ROX-1234): reminder for us to remove one or the other thing.

ENV GOFLAGS=""
ENV CI=1

Expand Down
Loading