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

Jump to bottom

NO-JIRA: Issue #362: feat(nbcs): build containers to be fips-ready #406

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

NO-JIRA: Issue #362: feat(nbcs): build containers to be fips-ready #406

wants to merge 1 commit into from

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Oct 2, 2024 •
edited
Loading

This is not intended to fully resolve https://issues.redhat.com/browse/RHOAISTRAT-214, but it's just a sensible first step done as a code improvement initiative, by following public advice in https://developers.redhat.com/articles/2022/05/31/your-go-application-fips-compliant

Description

  1. Dockerfiles are brought closer together, especially to the Red Hat build; previously, sourcing things in a stand-alone RUN command had no effect
  2. The openssl fips-compatible library is linked into the manager binaries, to proactively address fips concerns

As a next step, cpaas Dockerfiles will need to be updated in turn with the new things here.

How Has This Been Tested?

GHA
run this on fips cluster when I get PR images built

  • quay.io/opendatahub/kubeflow-notebook-controller:pr-406
  • quay.io/opendatahub/odh-notebook-controller:pr-406

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@jiridanek
Copy link
Member Author

/retest

Looks like a flake, let's retrigger without thinking.

@jiridanek
Copy link
Member Author

First check, see that the expected fips-related symbols are in the binary

sh-4.4$ cat /manager | grep --binary-files=text -oh FIPS_mode
FIPS_mode
FIPS_mode
FIPS_mode
FIPS_mode
FIPS_mode
FIPS_mode

check that the binary is dynamically linked with libc

sh-4.4$ ldd /manager 
 linux-vdso.so.1 (0x00007ffe7ccbd000)
 libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f08a5cb3000)
 libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f08a5a93000)
 libdl.so.2 => /lib64/libdl.so.2 (0x00007f08a588f000)
 libc.so.6 => /lib64/libc.so.6 (0x00007f08a54b9000)
 /lib64/ld-linux-x86-64.so.2 (0x00007f08a5ecb000)

https://developers.redhat.com/articles/2022/05/31/your-go-application-fips-compliant?source=sso#how_to_get_started

@jiridanek
Copy link
Member Author

There are no suspicious messages in the logs.

@jiridanek
Copy link
Member Author

For further improvements, we maybe should have /healthz endpoint that has some diagnostics, or just dump these into the log at startup

  • metadata about the binary, what it was built from, if it can be found
  • what are the dependencies, there is introspection api for go binaries so that they can write out their own deps
  • whether it's working in fips regimen or not

@jiridanek jiridanek mentioned this pull request Oct 2, 2024
Open
@jiridanek
Copy link
Member Author

And the notebook is spawned just fine, so, let's consider this tested.

I used fips-enabled https://console-openshift-console.apps.ods-qe-psi-06

I had to change oauth-proxy image digest to 4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695.

@harshad16
Copy link
Member

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Do not merge this PR label Oct 2, 2024
@jiridanek jiridanek changed the base branch from v1.9-branch to main October 2, 2024 12:41
@jiridanek
Issue opendatahub-io#362: feat(nbcs): build containers to be fips-ready
0400fc3 
This takes inspiration from:

* The Notebooks 2.0 Dockerfile, which comes from a default recent Kubebuilder template, at
https://github.com/kubeflow/notebooks/blob/notebooks-v2/workspaces/controller/Dockerfile

* The Red Hat build Dockerfile (that's the Cachito part) in an internal repository.

This change brings multiple improvements:

1. Dockerfiles are brought closer together, especially to the Red Hat build; previously, sourcing things in a stand-alone RUN command had no effect
2. The openssl fips-compatible library is linked into the manager binaries, to proactively address fips concerns
@openshift-cherrypick-robot
Copy link

@jiridanek: once the present PR merges, I will cherry-pick it on top of v1.9-branch in a new PR and assign it to you.

In response to this:

/cherrypick v1.9-branch

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-sigs/prow repository.

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Oct 2, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jiridanek. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@harshad16
Copy link
Member

/cherrypick v1.9-branch

@jiridanek , please dont use cherrypick anymore for this repo,
main to v1.9-branch would be synced.
cherry-pick would only be needed , if some other content is not being moved.

@jiridanek jiridanek changed the title Issue #362: feat(nbcs): build containers to be fips-ready NO-JIRA: Issue #362: feat(nbcs): build containers to be fips-ready Oct 4, 2024
jstourac
jstourac reviewed Oct 8, 2024
View reviewed changes
Copy link
Member

@jstourac jstourac left a comment

Choose a reason for hiding this comment

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

/lgtm in general

Two questions:

  1. Why TARGETARCH doesn't have a default value similarly as TARGETOS?
  2. Should we have the TARGETARCH defined in our Makefile?

jiridanek
jiridanek commented Oct 8, 2024
View reviewed changes
components/notebook-controller/Dockerfile
# the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore,
# by leaving it empty we can ensure that the container and binary shipped on it will have the same platform.
RUN if [ -z ${CACHITO_ENV_FILE} ]; then go mod download; else source ${CACHITO_ENV_FILE}; fi && \
CGO_ENABLED=1 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -tags strictfipsruntime -a -o ./bin/manager main.go
Copy link
Member Author

Choose a reason for hiding this comment

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

Why TARGETARCH doesn't have a default value similarly as TARGETOS?

That's explained in the comment above. No default means default to the arch of the container.

Copy link
Member

Choose a reason for hiding this comment

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

thanks, I didn't pay enough attention to the comment

jiridanek
jiridanek commented Oct 8, 2024
View reviewed changes
components/notebook-controller/Dockerfile
Comment on lines +14 to +15
ARG TARGETOS
ARG TARGETARCH
Copy link
Member Author

@jiridanek jiridanek Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we have the TARGETARCH defined in our Makefile?

In general, I think not, or not yet. These variables are there mostly when somebody wants to mess with it from the outside (do amd64 build on $m_x$ mac). When we want to do

we can do something with it.

The way kubebuilder does it is something like https://github.com/kubeflow/notebooks/blob/notebooks-v2/workspaces/controller/Makefile#L106-L115 We'd have to do this with podman instead of docker buildx, but the approach is not very different. And we'd have to take into account osbs/konflux, which (I guess) builds every arch natively and then does the manifest in a separate step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstourac jstourac jstourac left review comments

@caponetto caponetto Awaiting requested review from caponetto

@dibryant dibryant Awaiting requested review from dibryant

Assignees
No one assigned
Labels
do-not-merge/hold Do not merge this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build fips-ready
4 participants
@jiridanek @harshad16 @openshift-cherrypick-robot @jstourac