Add non-root user support by lbussell · Pull Request #4397 · dotnet/dotnet-docker

lbussell · 2023-02-07T02:47:40Z

Fixes #2249. Also see dotnet/designs#271.

Adds non-root runtime-deps files so that any 8.0 image may be run with -u app or new images can be created with USER app at the end of the Dockerfile to run as non-root.

Other changes:

switch the default port from 80 to 8080
use the new environment variable ASPNETCORE_HTTP_PORTS instead of ASPNETCORE_URLS
don't unset ASPNETCORE_URLS in sdk images. Aspnetcore sees the (empty) URLS variable and overrides the new HTTP_PORTS variable

…-user

richlander · 2023-02-07T02:55:05Z

Other related issues:

richlander · 2023-02-07T03:03:37Z

Soooo nice to see this PR (start to) land.

Configure web servers to bind to port 8080/8443 when present

It seems like it should only mention ports that are configured.

Aspnetcore sees the (empty) URLS variable and overrides the new HTTP_PORTS variable

That doesn't seems right. The diff suggests that the old variable is gone.

Home directory

It looks like we set the home directory for regular images and not for distroless/chiseled. We should document our policy.

Scope

Would be good to document the scope ... I think it is "all image types: runtime and SDK, Linux and Windows, regular and chiseled/distroless".

mthalman · 2023-02-07T18:19:00Z

FYI: Looks like tests are failing for all versions other than 8.0.

eng/dockerfile-templates/Dockerfile.common-dotnet-envs

eng/dockerfile-templates/runtime-deps/Dockerfile

tests/Microsoft.DotNet.Docker.Tests/CommonRuntimeImageTests.cs

tests/Microsoft.DotNet.Docker.Tests/ImageScenarioVerifier.cs

richlander · 2023-02-13T04:52:38Z

FYI ... This is what Ubuntu 23.04 has.

$ docker run --rm ubuntu:23.04 cat /etc/passwd | grep ubuntu
ubuntu:x:1000:1000:Ubuntu:/home/ubuntu:/bin/bash

tests/Microsoft.DotNet.Docker.Tests/ImageScenarioVerifier.cs

mthalman · 2023-02-13T14:09:26Z

eng/dockerfile-templates/monitor/Dockerfile.envs

-    # Unset ASPNETCORE_URLS from aspnet base image
-    ASPNETCORE_URLS= \
+    {{if dotnetMajor != "6" && dotnetMajor != "7":# Unset ASPNETCORE_HTTP_PORTS from aspnet base image
+    ASPNETCORE_HTTP_PORTS= \^else:# Unset ASPNETCORE_URLS from aspnet base image


@jander-msft - I know you requested this. Can you explain the need for this?

Also, should the monitor Dockerfile be configured to run as non-root by default?

@jander-msft - I know you requested this. Can you explain the need for this?

.NET Monitor already runs its HTTP server at ports 52323 and 52325 by default. Either setting ASPNETCORE_HTTP_PORTS would override that behavior (which we don't want by default) or it is not observed (which would be bad to insinuate that it has some effect when it does not); I think the former would be the case if the environment variable is specified. I will very later today that this is the case.

Also, should the monitor Dockerfile be configured to run as non-root by default?

That would be great if that could be added too. Although, if this change is only scoped to .NET 8+, then this work shouldn't be necessary because .NET Monitor is only offering distroless and chiseled images for .NET 8+, which should already be using the non-root user.

MichaelSimons · 2023-02-13T14:38:33Z

src/runtime-deps/8.0/alpine3.17/arm32v7/Dockerfile

+
+# Create a non-root user and group
+RUN addgroup \
+        --system \


Should the options be ordered consistently between addgroup and adduser? The system and id options are ordered in opposite order. A strict alphabetical order would be consistent with the rest of the Dockerfile.

I agree with you but I think this would mean either 1.) changing all of the old distroless Dockerfiles in this PR, or 2.) conditioning the alphabetical order in the dockerfile templates for 8.0 (until the next servicing release, when we can change all the older Dockerfiles?)

@mthalman thoughts?

A 3rd option would be to use the existing pattern and log a follow-up issue to correct the ordering across the board.

I agree that this can be done as a follow-up issue to apply consistently.

MichaelSimons · 2023-02-13T14:41:06Z

src/runtime-deps/8.0/cbl-mariner2.0-distroless/amd64/Dockerfile

+    && mkdir -p /staging/var/lib/rpmmanifest \
+    # Remove duplicates that match on the first field (package name)
+    && tac $tmpManifestPath | gawk '!x[$1]++' | sort > /staging/var/lib/rpmmanifest/container-manifest-2
+


Are the two blank lines intentional here? One feels more consistent. Typically two are only used between stages.

Suggested change

The extra line is present in old mariner 2.0 distroless dockerfiles as well, which I'm trying not to overwrite:

dotnet-docker/src/runtime-deps/6.0/cbl-mariner2.0-distroless/amd64/Dockerfile

Lines 30 to 31 in b806387

Should we file an issue for this as well?

Should we file an issue for this as well?

Yes, please.

Filed #4418.

MichaelSimons · 2023-02-13T14:46:35Z

src/runtime-deps/8.0/cbl-mariner2.0/amd64/Dockerfile

+        openssl-libs \
+        zlib \
+    && tdnf clean all \
+    # Create a non-root user and group


What's the reason to do this all within one stage? It feels a bit inconsistent with the rest of the images (I am intentionally overlooking the shadow-utils issue). For consistency and readability, it feels to me that this should be a separate instruction. If you decide to keep with this pattern, I would suggest only calling tdnf clean all once.

The shadow-utils issue is the reason it's all in one stage. Separating it into multiple stages anywhere in-between will increase the image size. I fixed the duplicate cleaning logic, that was a bug. Do you think that we should install shadow-utils only for the new user creation logic, and uninstall immediately after? i.e.

RUN tdnf install -y \ shadow-utils \ && groupadd \ --system \ ... && tdnf remove -y \ shadow-utils \ && tdnf clean all \

Do you think that we should install shadow-utils only for the new user creation logic, and uninstall immediately after?

Yes, the instruction you included is the pattern I was expecting to see.

Got it, misunderstood the original discussion, I'll work on that change now.

That seems rather inefficient to me. We'd then need to "update" and clean up packages twice. What benefit does that approach have?

Consistency of the patterns and layers across our Dockerfiles.

@mthalman there is the --cacheonly / -C option if we want to avoid updating the cache from the previous layer.

MichaelSimons · 2023-02-13T14:56:38Z

src/runtime-deps/8.0/alpine3.17/amd64/Dockerfile

+        libstdc++ \
+        zlib
+
+# Create a non-root user and group


From an instruction ordering perspective, it feels like this layer is more stable than the package installs and should therefore be placed before the packages instruction. This doesn't provide any value to us but is a best practice and would be beneficial to some who copy our Dockerfiles. FYI, there is a degree of inconsistency across the Dockerfiles because the chiseled images create the user before installing the packages.

Related, it feels like we should do the same for the ENV instruction. We place the ENV first in the aspnet and sdk Dockerfiles. This should be fixed in a separate PR if everyone agrees.

Filed #4419 to address this.

MichaelSimons

LGTM

eng/dockerfile-templates/Dockerfile.linux.install-deps

jander-msft

.NET Monitor LGTM

mthalman · 2023-02-14T22:13:03Z

eng/dockerfile-templates/Dockerfile.linux.install-deps

+        set certsPkgPrefix to when(isMariner,
+            [
+                when(isDistrolessMariner, "prebuilt-ca-certificates", "ca-certificates"),
+                "",
+                dotnetDepsComment
+            ],
+            [
+                "ca-certificates",
+                "",
+                dotnetDepsComment
+            ]) ^


This looks to be indented more than everything else.

I reverted to the original version of this file that exists on nightly - but since it does look weird, I can address it here.

(cherry picked from commit 4fced56)

mthalman · 2023-06-26T16:25:29Z

Documented breaking changes at dotnet/docs#35958, dotnet/docs#35959

lbussell added 11 commits January 26, 2023 10:13

Add 8.0 images with new non-root user

737fd53

Regenerate Dockerfiles

e71bc0b

All new dockerfiles build now

02e6241

Add jammy-chiseled 8.0 runtime-deps files for new aspnet ports

26c26ca

Move aspnet sample back to net7.0

fc176d8

Update environment variables for 8.0 dockerfiles

aee134e

WIP tests

7773de5

Try to clear tmp directory when running dotnet help

670ec48

Clean up Dockerfiles

74311e6

Merge remote-tracking branch 'upstream/nightly' into feature/non-root…

452945b

…-user

Remove commented out tests that don't run

a60cc4f

lbussell requested a review from richlander February 7, 2023 02:47

lbussell requested review from MichaelSimons and mthalman as code owners February 7, 2023 02:47

ghost added the area-dockerfiles Concerns the official .NET Dockerfiles or Dockerfile templates label Feb 7, 2023

Remove https port variables

dc8141a

lbussell force-pushed the feature/non-root-user branch from bd831bf to dc8141a Compare February 7, 2023 18:40

.NET versions < 8.0 want the --urls argument

3fced5d

lbussell force-pushed the feature/non-root-user branch from f4d27c0 to 3fced5d Compare February 7, 2023 19:31

mthalman reviewed Feb 7, 2023

View reviewed changes

lbussell added 7 commits February 7, 2023 11:57

Address some review comments

9130588

Clean up ports, run fx dependent test as non-root

0d68933

Fix debian home creation behavior

cfff6d4

Fix aspnet sample base images

0ae0262

Add equals sign back in group add command

991f949

I don't know why I swapped these arguments, swap them back

c0b37b0

Correctly pass through create-home variable to non-root-user template

ebb8aed

Change aspnet port env var in 8.0+ monitor dockerfiles

1a0fa36

lbussell requested review from jander-msft and mthalman February 10, 2023 23:20

Make version checks in tests more serviceable

8d775f5

mthalman reviewed Feb 13, 2023

View reviewed changes

MichaelSimons reviewed Feb 13, 2023

View reviewed changes

Clean up version checks in tests

c5aae01

lbussell force-pushed the feature/non-root-user branch from 9b4836c to c5aae01 Compare February 13, 2023 16:43

lbussell added 5 commits February 13, 2023 08:47

Look for new environment variable to be unset in monitor tests

40d8c42

Add args back

a880a6a

Fix no-clean logic to only clean once in mariner 8.0

cc858da

7.0 doesn't have non-root support

452e753

Install shadow-utils in line with non-root user in mariner

c0442d4

MichaelSimons approved these changes Feb 14, 2023

View reviewed changes

mthalman reviewed Feb 14, 2023

View reviewed changes

eng/dockerfile-templates/Dockerfile.linux.install-deps Outdated Show resolved Hide resolved

Revert to old install-deps template

daaa7e3

jander-msft approved these changes Feb 14, 2023

View reviewed changes

mthalman reviewed Feb 14, 2023

View reviewed changes

Fix indentation in install-deps template

724e253

mthalman approved these changes Feb 15, 2023

View reviewed changes

lbussell merged commit 4fced56 into dotnet:nightly Feb 15, 2023

mthalman pushed a commit to mthalman/dotnet-docker that referenced this pull request Feb 16, 2023

Add non-root user support (dotnet#4397)

b9bf553

(cherry picked from commit 4fced56)

mthalman mentioned this pull request Feb 16, 2023

Updates for .NET 8 Preview 1 release #4432

Merged

mthalman pushed a commit to mthalman/dotnet-docker that referenced this pull request Feb 16, 2023

Add non-root user support (dotnet#4397)

17c97de

(cherry picked from commit 4fced56)

rwkarg mentioned this pull request Feb 23, 2023

New app user for .NET 8 images isn't documented as a potentially breaking change #4451

Closed

richlander mentioned this pull request Mar 15, 2023

Theme: Cloud Native dotnet/aspnetcore#47240

Closed

11 tasks

mthalman added the needs-announcement An announcement is needed to discuss customer impact label Jun 26, 2023

richlander mentioned this pull request Aug 4, 2023

Update samples to .NET 8 #4742

Merged

Conversation

lbussell commented Feb 7, 2023

Uh oh!

richlander commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richlander commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mthalman commented Feb 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richlander commented Feb 13, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbussell Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelSimons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jander-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mthalman commented Jun 26, 2023

Uh oh!

Reviewers

Assignees

richlander commented Feb 7, 2023 •

edited

Loading

richlander commented Feb 7, 2023 •

edited

Loading

lbussell Feb 13, 2023 •

edited

Loading