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

chore: Dockerfile remove redundant directives #3914

Closed
wants to merge 8 commits into from
Closed

chore: Dockerfile remove redundant directives #3914

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
031787c
chore/remove-nsswitch-hosts
polarathene Dec 28, 2024
04fa48b
chore: Revise scratch image
polarathene Dec 28, 2024
8ed5090
chore: Revise alpine images
polarathene Dec 28, 2024
f612685
chore: Use the same busybox commands with distroless
polarathene Dec 28, 2024
51fc717
chore: Remove redundant `VOLUME` directives
polarathene Dec 28, 2024
ed85d9a
chore: `Dockerfile-scratch` - Add commentary
polarathene Jan 5, 2025
4ada680
chore: `Dockerfile-sqlite` - Add commentary
polarathene Jan 5, 2025
90f2518
chore: `Dockerfile-alpine` - Add commentary
polarathene Jan 5, 2025
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
29 changes: 16 additions & 13 deletions .docker/Dockerfile-alpine
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
FROM alpine:3.20

RUN addgroup -S ory; \
adduser -S ory -G ory -D -H -s /bin/nologin && \
apk upgrade --no-cache && \
RUN <<HEREDOC
apk upgrade --no-cache
apk add --no-cache --upgrade ca-certificates

COPY hydra /usr/bin/hydra

# set up nsswitch.conf for Go's "netgo" implementation
# - https://github.com/golang/go/blob/go1.9.1/src/net/conf.go#L194-L275
RUN echo 'hosts: files dns' > /etc/nsswitch.conf
# Add a user/group for Ory with a stable UID + GID:
addgroup --system --gid 500 ory
adduser --system --uid 500 \
--gecos "Ory User" \
--home /home/ory \
--ingroup ory \
--shell /sbin/nologin \
ory

# By creating the sqlite folder as the ory user, the mounted volume will be owned by ory:ory, which
# is required for read/write of SQLite.
RUN mkdir -p /var/lib/sqlite && \
chown ory:ory /var/lib/sqlite
# Create the sqlite directory with ownership to that user and group:
# NOTE: This is required for read/write by SQLite.
install --owner ory --group ory --directory /var/lib/sqlite
aeneasr marked this conversation as resolved.
Show resolved Hide resolved
HEREDOC

USER ory
COPY hydra /usr/bin/hydra
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context is lacking a bit for these COPY instructions.

Since the image itself doesn't have a build stage, it's rather vague what the hydra binary was built with. I doubt it matters for most, but since this is used for the official image published to DockerHub, some context might be worthwhile.

I assume going forward both Dockerfile-alpine + Dockerfile-distroless will take the binary built via Dockerfile-build, but perhaps you typically build Hydra outside of a container? 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Outside of docker compose and make docker, the binaries are built by goreleaser and then injected into the containers. So this is correct here.


ENTRYPOINT ["hydra"]
CMD ["serve", "all"]
USER ory
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As covered by earlier review comments, this could be dropped in a follow-up PR.

Technically a breaking change (but so is the 500:500 UID/GID proposed in this PR), but all a user should need to do is adjust the ownership of the /var/lib/sqlite or run with --user 101:101 (might differ by image depending on implicit UID/GID assigned to ory).

Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with this change, the hydra command should run as non-root in my view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no suggestion snippet to remove the line. It was simply shifted to the end of the file. A change to drop it would be:

Suggested change
USER ory

My comment only proposed dropping non-root, and noted that this PR configures the non-root user with a stable UID/GID value, rather than relying on it implicitly (unstable as any package installed prior may affect user/groups causing the UID/GID for ory to change unexpectedly).

I then noted that the stable UID/GID change itself, would be a breaking change to existing users of the image going forward, similar to if changing from non-root to root.

Since there's no real dependency on the non-root user in the image, it's fine and someone can choose to explicitly run as root at runtime (which should be secure, especially via rootless container).

2 changes: 0 additions & 2 deletions .docker/Dockerfile-build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ FROM gcr.io/distroless/static-debian12:nonroot AS runner
COPY --from=builder --chown=nonroot:nonroot /var/lib/sqlite /var/lib/sqlite
COPY --from=builder /usr/bin/hydra /usr/bin/hydra

VOLUME /var/lib/sqlite

# Declare the standard ports used by hydra (4444 for public service endpoint, 4445 for admin service endpoint)
EXPOSE 4444 4445

Expand Down
36 changes: 21 additions & 15 deletions .docker/Dockerfile-hsm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,30 +53,36 @@ ENV HSM_LIBRARY=/usr/lib/softhsm/libsofthsm2.so
ENV HSM_TOKEN_LABEL=hydra
ENV HSM_PIN=1234

# NOTE: This is broken already. Even though this image provides a shell, you'd need to configure it with
# `SHELL ["/busybox/sh", "-c"]`, however `apt-get` does not exist either in a distroless image.
# This was original an Alpine image, the refactoring was not verified properly in this commit:
# https://github.com/ory/hydra/commit/c1e1a569621d88365dceee7372ca49ecd119f939#diff-ae54bef08e3587b28ad8e93eb253a9a5cd9ea6f4251977e35b88dc6b42329e25L31
Comment on lines +56 to +59
Copy link
Member

Choose a reason for hiding this comment

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

The HSM image is really just to run some e2e hsm tests. It's not being distributed and should not be used.

Copy link
Member

@aeneasr aeneasr Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, e2e tests are now failing. Probably just revert the changes here

https://github.com/ory/hydra/actions/runs/12523819546/job/34933781514?pr=3914

RUN apt-get -y install softhsm opensc &&\
pkcs11-tool --module "$HSM_LIBRARY" --slot 0 --init-token --so-pin 0000 --init-pin --pin "$HSM_PIN" --label "$HSM_TOKEN_LABEL"

RUN addgroup -S ory; \
adduser -S ory -G ory -D -h /home/ory -s /bin/nologin; \
chown -R ory:ory /home/ory; \
RUN <<HEREDOC
# Add a user/group for Ory with a stable UID + GID:
addgroup --system --gid 500 ory
adduser --system --uid 500 \
--gecos "Ory User" \
--home /home/ory \
--ingroup ory \
--shell /sbin/nologin \
ory

# Create the sqlite directory with ownership to that user and group:
# NOTE: This is required for read/write by SQLite.
install --owner ory --group ory --directory /var/lib/sqlite

# NOTE: Presumably this was already created by the prior RUN directive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
# NOTE: Presumably this was already created by the prior RUN directive
# NOTE: This location was created by `softhsm2` package install with ownership `root:softhsm`,
# while the `pkcs11-tool` command generated a token dir + files with permissions of 600.
# To support running hydra in the container as the non-root ory user, grant ownership for RW:

/var/lib/softhsm/tokens is created when installing the softhsm / softhsm2 package. Ownership is root:softhsm. As is the token dir/files generated by the earlier pkcs11-tool command.

Rather than chown -R this directory and contents, it'd be wiser to just add ory to the softhsm group. However, the token generated only allows access to the file owner (root):

$ ls -l /var/lib/softhsm
drwxrws--- 2 root softhsm 4096 Apr  1  2024 tokens

$ ls -l /var/lib/softhsm/tokens
drwx--S--- 2 root softhsm 4096 Jan  5 22:34 c0e04acd-3e15-0266-6003-f9394edce34b

$ ls -l /var/lib/softhsm/tokens/c0e04acd-3e15-0266-6003-f9394edce34b
-rw------- 1 root softhsm   8 Jan  5 22:34 generation
-rw------- 1 root softhsm   0 Jan  5 22:34 token.lock
-rw------- 1 root softhsm 320 Jan  5 22:34 token.object

So in this case chown is required for the owner specifically. Probably shouldn't mess with the group ownership? 🤷‍♂️

Personally unless there's a clear reason for building an image to run with a non-root user, I'd suggest keeping it simple to maintain and just using root which avoids all this.

Anyone bind mounting a volume for local storage to the container is not going to have the ory UID/GID match their host user since regardless of the previous implicit UID/GID (100:101) or the static 500:500 proposed here, the typical non-root host user is 1000:1000.

That leaves the remaining benefit as a "best practice", but AFAIK is mostly moot if the user instead runs the container with Podman/Docker in rootless mode which uses /etc/subuid + /etc/subgid tables on the host to map the containers root UID + GID to the hosts user and any other container UID/GID values to the subuid / subgid range. Alternatively, when run as rootful, the container can be run with all capabilities dropped, which is implicit when running a process as a non-root user in 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.

Personally unless there's a clear reason for building an image to run with a non-root user, I'd suggest keeping it simple to maintain and just using root which avoids all this.

That leaves the remaining benefit as a "best practice", but AFAIK is mostly moot if the user instead runs the container with Podman/Docker in rootless mode which uses /etc/subuid + /etc/subgid tables on the host to map the containers root UID + GID to the hosts user and any other container UID/GID values to the subuid / subgid range. Alternatively, when run as rootful, the container can be run with all capabilities dropped, which is implicit when running a process as a non-root user in the container.

[...] mostly moot if the user instead [...]

security is never moot if you assume that everyone is smart or experienced enough to do the right thing. Running as root will fail several CI checks, please revert the user changes made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhh, I generally don't see people understanding non-root practice in containers, just pushing for it as a security practice regardless of knowing why.

As I've said, you'll have default capabilities granted to the container root user, but these aren't the same as host root (full caps). Switching to a non-root user just drops these additional caps, which if you really need to you can do so yourself as a security practice explicitly (dropping all caps, or when compatible, switching the user to non-root if you really insist).

The other reason to use non-root is if container escape is accomplished, then escaping as the root user to be root on the host would allow more damage, but this doesn't make non-root in a container exempt from this, you can still escape as non-root and become root on the host depending on the vulnerability exploited. The bulk of vulnerabilities require granting capabilities or relaxing security intentionally for a container than would otherwise be permitted by default, hence why I'm not really sold on the non-root container user "best practice".

I've seen a variety of containers that adopted non-root as a security practice, but then did workarounds that made security worse. Some granted the non-root user access to the docker socket, others granted capabilities that wouldn't be allowed by default, or mandated capabilities to support non-root but in such a way that prevented someone who is not using a feature that requires that capability and knows how to properly lock down a container to be forced to grant the capability for the binary to run...

To assume that building a container that runs as non-root is always more secure would be a mistake. Inexperienced users that make a container insecure can happen regardless too, trying to address incompetence with a blanket "fix" which often introduces more caveats, especially when the image fails to document this practice is ill-advised IMO. If you're going to defend the practice, have a good reason beyond parroting / hand-waving the justification.

More security conscious (even those inexperienced) should be using rootless Docker/Podman, where it's perfectly ok to have the container user as root, since escaping the container does not result in retaining the same UID/GID of the container user thanks to user namespace mapping.

Root in a container is not equivalent to root on the host. I've explained and demonstrated this so many times in the past to those that don't know better but parrot paranoia insisting that they know what they're talking about.

Running as root will fail several CI checks, please revert the user changes made

I'm not sure what CI checks you were referring to, but your response is to a comment that proposed dropping the non-root approach. The PR respected the non-root decision, all it did here was ensure consistency with the other images for non-root user setup, thus nothing to revert.

chown -R ory:ory /var/lib/softhsm/tokens
HEREDOC

COPY --from=build-hydra /usr/bin/hydra /usr/bin/hydra

# By creating the sqlite folder as the ory user, the mounted volume will be owned by ory:ory, which
# is required for read/write of SQLite.
RUN mkdir -p /var/lib/sqlite && \
chown ory:ory /var/lib/sqlite

VOLUME /var/lib/sqlite

# Exposing the ory home directory
VOLUME /home/ory

# Declare the standard ports used by hydra (4444 for public service endpoint, 4445 for admin service endpoint)
EXPOSE 4444 4445

USER ory

ENTRYPOINT ["hydra"]
CMD ["serve"]
USER ory
49 changes: 31 additions & 18 deletions .docker/Dockerfile-scratch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,41 @@
FROM alpine:3.20
# TODO: Remove this file in favor of distroless-static variant:
# https://github.com/ory/hydra/blob/master/.docker/Dockerfile-distroless-static
# However if published to any registry, continue to publish the variant tag but as an alias to `-distroless` tags:
# https://github.com/ory/hydra/pull/3914#pullrequestreview-2527315326

Comment on lines +1 to +4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is redundant AFAIK, I don't know where it's being used/published, so proposed changes for consistency remain here and the file could be dropped in a follow-up PR, reverting the change if required.

Copy link
Member

Choose a reason for hiding this comment

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

I checked and we no longer distribute this scratch image but instead the distroless variant. It should be OK to be removed.

RUN apk upgrade --no-cache && \
apk add --no-cache --upgrade ca-certificates

# set up nsswitch.conf for Go's "netgo" implementation
# - https://github.com/golang/go/blob/go1.9.1/src/net/conf.go#L194-L275
RUN [ ! -e /etc/nsswitch.conf ] && echo 'hosts: files dns' > /etc/nsswitch.conf
FROM alpine:3.20 AS base-files

RUN addgroup -S ory; \
adduser -S ory -G ory -D -h /home/ory -s /bin/nologin;
RUN <<HEREDOC
apk upgrade --no-cache
apk add --no-cache --upgrade ca-certificates

RUN mkdir -p /var/lib/sqlite && \
chown -R ory:ory /var/lib/sqlite
# Add a user/group for Ory with a stable UID + GID:
# NOTE: This only appears relevant for supporting hydra as non-root, otherwise unnecessary.
addgroup --system --gid 500 ory
adduser --system --uid 500 \
--gecos "Ory User" \
--home /home/ory \
--ingroup ory \
--shell /sbin/nologin \
ory

# Create the sqlite directory with ownership to that user and group:
# NOTE: This is required for read/write by SQLite.
# - Path may be a default value somewhere, or only explicitly provided via DSN?
# - Owner/Group is only relevant to permissions allowing the hydra process to read/write to the location.
install --owner ory --group ory --directory /var/lib/sqlite
HEREDOC

FROM scratch

COPY --from=0 /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
COPY --from=0 /etc/nsswitch.conf /etc/nsswitch.conf
COPY --from=0 /etc/passwd /etc/passwd
COPY --from=0 /var/lib/sqlite /var/lib/sqlite
COPY --from=base-files /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
COPY --from=base-files /etc/nsswitch.conf /etc/nsswitch.conf
# NOTE: /etc/group and /etc/shadow were not copied over, only user lookup is valid for `USER`:
COPY --from=base-files /etc/passwd /etc/passwd
# NOTE: This COPY defaults to 0:0 for ownership, voiding the requirement conveyed above
COPY --from=base-files /var/lib/sqlite /var/lib/sqlite

COPY hydra /usr/bin/hydra

USER ory

ENTRYPOINT ["hydra"]
CMD ["serve", "all"]
USER ory
50 changes: 25 additions & 25 deletions .docker/Dockerfile-sqlite
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,36 @@
FROM alpine:3.20

# Because this image is built for SQLite, we create /home/ory and /home/ory/sqlite which is owned by the ory user
# and declare /home/ory/sqlite a volume.
#
# To get SQLite and Docker Volumes working with this image, mount the volume where SQLite should be written to at:
#
# /home/ory/sqlite/some-file.
# TODO: Remove this file in favor of the main/default Alpine image. The sqlite package is no longer required:
# https://github.com/ory/hydra/blob/master/.docker/Dockerfile-alpine
# However if published to any registry, continue to publish the variant tag but as an alias to standard Alpine image tags:
# https://github.com/ory/hydra/pull/3914#pullrequestreview-2527315326

Comment on lines +1 to +4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is redundant AFAIK, I don't know where it's being used/published, so proposed changes for consistency remain here and the file could be dropped in a follow-up PR, reverting the change if required.

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it, I also could not find any use for it.

RUN addgroup -S ory; \
adduser -S ory -G ory -D -h /home/ory -s /bin/nologin; \
chown -R ory:ory /home/ory && \
apk upgrade --no-cache && \
FROM alpine:3.20
RUN <<HEREDOC
# NOTE: The sqlite package is not required when the later copied hydra binary is built with statically linked sqlite?
apk upgrade --no-cache
apk add --no-cache --upgrade --latest ca-certificates sqlite

WORKDIR /home/ory
# Add a user/group for Ory with a stable UID + GID:
# NOTE: This only appears relevant for supporting hydra as non-root, otherwise unnecessary.
addgroup --system --gid 500 ory
adduser --system --uid 500 \
--gecos "Ory User" \
--home /home/ory \
--ingroup ory \
--shell /sbin/nologin \
ory

# Create the sqlite directory with ownership to that user and group:
# NOTE: This is required for read/write by SQLite.
# - Path may be a default value somewhere, or only explicitly provided via DSN?
# - Owner/Group is only relevant to permissions allowing the hydra process to read/write to the location.
install --owner ory --group ory --directory /var/lib/sqlite
HEREDOC

COPY hydra /usr/bin/hydra

# By creating the sqlite folder as the ory user, the mounted volume will be owned by ory:ory, which
# is required for read/write of SQLite.
RUN mkdir -p /var/lib/sqlite && \
chown ory:ory /var/lib/sqlite

VOLUME /var/lib/sqlite

# Exposing the ory home directory
VOLUME /home/ory

# Declare the standard ports used by Hydra (4444 for public service endpoint, 4445 for admin service endpoint)
EXPOSE 4444 4445

USER ory

ENTRYPOINT ["hydra"]
CMD ["serve"]
USER ory
Loading