[SYCL][Devops] Fix `DockerFile` linting issues discovered by `trivy` #16361

AlexeySachkov · 2024-12-13T16:34:49Z

This is a re-submit of #16290 with fixes from #16324 and some more extra changes.

Issues addressed:

AVD-DS-0017 (HIGH): The instruction 'RUN update' should always be followed by ' install' in the same RUN statement.
See https://avd.aquasec.com/misconfig/ds017
AVD-DS-0002 (HIGH): Specify at least 1 USER command in Dockerfile with non-root user as argument
See https://avd.aquasec.com/misconfig/ds002
AVD-DS-0002 (HIGH): Last USER command in Dockerfile should not be 'root'

Issues remaining:

AVD-DS-0026 (LOW): Add HEALTHCHECK instruction in your Dockerfile
See https://avd.aquasec.com/misconfig/ds026

I didn't add HEALTHCHECK command to our containers, because I don't know if that makes sense and which command to launch. I.e. our containers they only provide some pre-installed tools, but they don't launch any services which we could check.

Other notable changes:

sycl user creation was outlined into a separate helper script and that user requires password to use sudo.

See https://avd.aquasec.com/misconfig/ds017 Docker best practices says that running `update` and `install` commands separately may lead to situations where Docker skips `update` step and re-uses cache leading to outdated versions of packages being installed.

See https://avd.aquasec.com/misconfig/ds002 Made it so that the last `USER` command in `base` and `build` is not `root`.

See https://avd.aquasec.com/misconfig/ds002 Made it so our docker files have at least one `USER` command which is not `root`.

…ssword for sudo

sarnex

sorry for late review, looks really good, just some minor comments

devops/actions/build_container/action.yml

devops/scripts/create-sycl-user.sh

devops/containers/ubuntu2204_preinstalled.Dockerfile

sarnex

lgtm with root thing fixed

Extended build containers action with an extra argument which specifies a password that will be assigned to the `sycl_ci` user created within containers. For now this new secret is unused, so this changes is expected to have no impact on our CI. This is outlined from intel#16361 to improve testing for that PR: for security reasons actions are only invoked from the default branch and never from a PR branch. Therefore, to actually test that this secret is properly used without errors we need to update the action first.

sarnex

lgtm, minor comments again, thx

sarnex · 2024-12-16T20:01:40Z

.github/workflows/sycl-linux-run-tests.yml

@@ -196,9 +196,9 @@ jobs:
      run: |
        if [ "${{ inputs.install_dev_igc_driver }}" = "true" ]; then
            # If libllvm14 is already installed (dev igc docker), still return true.
-            sudo apt-get install -yqq libllvm14 || true;
+            cat /run/secrets/sycl_passwd | sudo -S apt-get install -yqq libllvm14 || true;


do we need parens here?

From my local experiments:

$ cat /run/secrets/sycl_ci_passwd | sudo -S echo "I'm root!" || echo "second" I'm root! $ cat /run/secrets/sycl_ci_passwd | sudo -S false || echo "second" second

So, it seems to work correctly, but I can add parens for clarity if that's a preference

sarnex · 2024-12-16T20:03:18Z

devops/scripts/create-sycl-user.sh

+
+if [[ $# -eq 0 ]]; then
+  # When launched without arguments, we assume that it was launched as part of
+  # CI workflow and therefore a different kind of user is created


i wonder if we can check GITHUB_ACTIONS here

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables

Considering that all our containers are only for the purposes of CI and local development, I decided that they will all use sycl_ci by default.

Those, who need sycl user are able to create it using that script manually as documented (8d931df). Considering that sycl user won't be created by us anymore, I think that it is actually better to have this script react to an arguments instead of the environment (i.e. I want everything to be explicit)

sarnex · 2024-12-16T20:04:05Z

devops/scripts/create-sycl-user.sh

+
+if [[ $SET_PASSWD == true ]]; then
+  if [[ ! -f /run/secrets/sycl_ci_passwd ]]; then
+    echo "Password is requested, but /run/secrtes/sycl_ci_passwd doesn't exists!"


Suggested change

echo "Password is requested, but /run/secrtes/sycl_ci_passwd doesn't exists!"

echo "Password is requested, but /run/secrtes/sycl_ci_passwd doesn't exist!"

sarnex · 2024-12-16T20:53:54Z

devops/scripts/create-sycl-user.sh

+  SET_PASSWD=false
+
+  # Some user id which is different from the one assigned to sycl_ci user
+  USER_ID=1234


also sorry, you might have to rebase because i added a 24.04 oneapi docker image today

No problem, see be4e0c6 for merge conflicts resolution and ff333a1 for changes to that new dockerfile

bader · 2024-12-16T22:08:35Z

Other notable changes:

sycl user creation was outlined into a separate helper script and that user requires password to use sudo.

Do we need to update anything in https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/DockerBKMs.md#changing-docker-user?

Extended build containers action with an extra argument which specifies a password that will be assigned to the `sycl_ci` user created within containers. For now this new secret is unused, so this changes is expected to have no impact on our CI. This is outlined from #16361 to improve testing for that PR: for security reasons actions are only invoked from the default branch and never from a PR branch. Therefore, to actually test that this secret is properly used without errors we need to update the action first.

AlexeySachkov · 2024-12-17T09:45:25Z

Other notable changes:

sycl user creation was outlined into a separate helper script and that user requires password to use sudo.

Do we need to update anything in https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/DockerBKMs.md#changing-docker-user?

Good point, I wasn't aware of that documentation, I will update it as well. In general, this change warrants a discussion, I believe, because short summary of what I do is making those images less useable for anyone else. I will initiate some discussion about that.

…y-docker-best-practices

AlexeySachkov · 2024-12-18T16:35:53Z

Other notable changes:

sycl user creation was outlined into a separate helper script and that user requires password to use sudo.

Do we need to update anything in https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/DockerBKMs.md#changing-docker-user?

Good point, I wasn't aware of that documentation, I will update it as well. In general, this change warrants a discussion, I believe, because short summary of what I do is making those images less useable for anyone else. I will initiate some discussion about that.

Documentation was updated in 8d931df. The concern I had is resolved, so no extra discussions are necessary besides this code review.

AlexeySachkov · 2024-12-18T17:49:48Z

So, due to the way GH operates which has to do with security, there is no way to test this PR (more or less properly) when its submitted from a fork. #16411 is a re-submit which is made from within the repo to properly test this.

AlexeySachkov added 9 commits December 13, 2024 05:11

[SYCL][Devops] Fix AVD-DS-0002

15a7c6a

See https://avd.aquasec.com/misconfig/ds002 Made it so that the last `USER` command in `base` and `build` is not `root`.

[SYCL][Devops] Fix remaining AVD-DS-0002 issues

10cc0e3

See https://avd.aquasec.com/misconfig/ds002 Made it so our docker files have at least one `USER` command which is not `root`.

An attempt to fix docker images build

1ea04f5

Outline sycl user creation into a separate script; Make it require pa…

49ab319

…ssword for sudo

Drop seemingly dead legacy code

c0439ff

Propagate new secret to images and use sudo with password in workflows

35a07f0

Run various driver installs as root when building containers

ed7171c

Properly propagate secrets down to user creation script

1ff5dd4

AlexeySachkov had a problem deploying to WindowsCILock December 13, 2024 16:35 — with GitHub Actions Error

AlexeySachkov had a problem deploying to WindowsCILock December 13, 2024 16:36 — with GitHub Actions Error

AlexeySachkov added 2 commits December 13, 2024 08:41

Properly use new secret in build-container action

4d28c77

Use the right secret

8530141

AlexeySachkov had a problem deploying to WindowsCILock December 13, 2024 16:46 — with GitHub Actions Error

Access action inputs properly

3d7c76f

AlexeySachkov had a problem deploying to WindowsCILock December 13, 2024 16:47 — with GitHub Actions Error

Permissions & action fixes

3cb941a

AlexeySachkov had a problem deploying to WindowsCILock December 13, 2024 16:53 — with GitHub Actions Error

sarnex reviewed Dec 16, 2024

View reviewed changes

devops/actions/build_container/action.yml Outdated Show resolved Hide resolved

devops/scripts/create-sycl-user.sh Outdated Show resolved Hide resolved

devops/containers/ubuntu2204_preinstalled.Dockerfile Outdated Show resolved Hide resolved

Lowercase root

6d88ec7

sarnex approved these changes Dec 16, 2024

View reviewed changes

Differentiate password-less sycl vs sycl-ci user

0964ce8

AlexeySachkov mentioned this pull request Dec 16, 2024

[SYCL][Devops] Propagate a new secret to containers build #16384

Merged

Align secret name with intel#16384

0625177

AlexeySachkov temporarily deployed to WindowsCILock December 16, 2024 17:52 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to WindowsCILock December 16, 2024 19:12 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to WindowsCILock December 16, 2024 19:28 — with GitHub Actions Inactive

sarnex reviewed Dec 16, 2024

View reviewed changes

AlexeySachkov added 3 commits December 18, 2024 07:24

Merge remote-tracking branch 'origin/sycl' into private/asachkov/appl…

be4e0c6

…y-docker-best-practices

Fix trivy reported issues in a new docker file

ff333a1

Switch all containers to sycl_ci user and update the documentation

8d931df

AlexeySachkov had a problem deploying to WindowsCILock December 18, 2024 16:35 — with GitHub Actions Error

AlexeySachkov added 2 commits December 18, 2024 08:36

Fix a typo

5d82571

One more typo fix

4ef8161

AlexeySachkov had a problem deploying to WindowsCILock December 18, 2024 17:13 — with GitHub Actions Error

AlexeySachkov closed this Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][Devops] Fix `DockerFile` linting issues discovered by `trivy` #16361

[SYCL][Devops] Fix `DockerFile` linting issues discovered by `trivy` #16361

AlexeySachkov commented Dec 13, 2024

sarnex left a comment

sarnex left a comment

sarnex left a comment

sarnex Dec 16, 2024

AlexeySachkov Dec 18, 2024

sarnex Dec 16, 2024

AlexeySachkov Dec 18, 2024

sarnex Dec 16, 2024

sarnex Dec 16, 2024

AlexeySachkov Dec 18, 2024

bader commented Dec 16, 2024

AlexeySachkov commented Dec 17, 2024

AlexeySachkov commented Dec 18, 2024

AlexeySachkov commented Dec 18, 2024

	echo "Password is requested, but /run/secrtes/sycl_ci_passwd doesn't exists!"
	echo "Password is requested, but /run/secrtes/sycl_ci_passwd doesn't exist!"

[SYCL][Devops] Fix DockerFile linting issues discovered by trivy #16361

[SYCL][Devops] Fix DockerFile linting issues discovered by trivy #16361

Conversation

AlexeySachkov commented Dec 13, 2024

sarnex left a comment

Choose a reason for hiding this comment

sarnex left a comment

Choose a reason for hiding this comment

sarnex left a comment

Choose a reason for hiding this comment

sarnex Dec 16, 2024

Choose a reason for hiding this comment

AlexeySachkov Dec 18, 2024

Choose a reason for hiding this comment

sarnex Dec 16, 2024

Choose a reason for hiding this comment

AlexeySachkov Dec 18, 2024

Choose a reason for hiding this comment

sarnex Dec 16, 2024

Choose a reason for hiding this comment

sarnex Dec 16, 2024

Choose a reason for hiding this comment

AlexeySachkov Dec 18, 2024

Choose a reason for hiding this comment

bader commented Dec 16, 2024

AlexeySachkov commented Dec 17, 2024

AlexeySachkov commented Dec 18, 2024

AlexeySachkov commented Dec 18, 2024

[SYCL][Devops] Fix `DockerFile` linting issues discovered by `trivy` #16361

[SYCL][Devops] Fix `DockerFile` linting issues discovered by `trivy` #16361