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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

chore: Dockerfile remove redundant directives #3914

Show file tree
Hide file tree
Changes from all 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
33 changes: 20 additions & 13 deletions .docker/Dockerfile-alpine
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,29 @@
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:
# 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
Comment on lines +7 to +15
Copy link
Author

Choose a reason for hiding this comment

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

Proposal to consider removing this in future follow-up PR.

  • Only seems relevant as a common "best practice" that some users request. It has it's fair share of caveats though, especially when not clearly communicated/documented for the image.
  • The only known dependency on this is optional SQLite for a fixed DSN location at /var/lib/sqlite, but this becomes redundant if the container has a bind mount volume, which again is not documented for clarity, but a common practice for persistence vs named data volumes.
  • Without any other support needed in the container, a user that wants this could better run the container with the user option to set the user to switch to and gain the same benefits.
  • If Hydra ever needs to use some linux capabilities, you'd need to add a step with setcap to grant them for non-root users, and ideally at runtime in Go handle checking this to raise the capability rather than enforce it via setcap (especially if the feature using it is optional). With root (including rootless Docker/Podman), the need for setcap is avoided. Keep this simple to maintain, since historically in this repo the Docker support is already showing inconsistencies in maintenance, mostly due to redundant noise for running as a non-root user by default.


# 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.
# - 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.
# - Bind mount volumes will replace the ownership with that of the host directory, requiring correction.
install --owner ory --group ory --directory /var/lib/sqlite
aeneasr marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +17 to +22
Copy link
Author

Choose a reason for hiding this comment

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

I've added commentary here, but from what I understand this path is only relevant when it's part of the DSN configured like in quickstart.yml? While ownership is only adjusted to support the non-root ory user to have RW access.

Some platforms like OpenShift have a feature that uses the root group for access while randomizing the UID the container runs as. I'm not entirely sure how viable that approach is and raised some concerns at another repo (caddy-docker) where someone proposed supporting that.

However, group/other permissions need to be adjusted for the DSN path regardless so that the hydra process has write access to create the DB (the execute bit on the directory is also required for that operation), thus 770 (rwxrwx---) is necessary.

services:
  hydra:
    image: localhost/hydra:v2.2.0
    # No delay in shutdown (uses tini as PID 1), proper signal forwarding + reaping:
    init: true
    # Optional: Run as non-root user, but use the root group for /var/lib/sqlite to avoid needing a chown
    user: 500:0
    environment:
      # DSN to SQLite:
      # https://www.ory.sh/docs/self-hosted/deployment#sql-persistent
      DSN: sqlite:///var/lib/sqlite/db.sqlite?_fk=true
    # Unlike the current quickstart.yml example which relies upon the Docker Compose `depends_on`
    # feature to apply DB migrations through another container instance, just run both commands here:
    # NOTE: While the quickstart doesn't mention it, `migrate sql` should technically
    # have you manually create a backup copy prior to running it (take caution with restart policy if automating):
    # https://www.ory.sh/docs/hydra/self-hosted/dependencies-environment
    entrypoint: ["ash", "-c"]
    command:
      - |
        hydra --config /etc/hydra/config.yml migrate sql --read-from-env --yes
        hydra --config /etc/hydra/config.yml serve all --dev --sqa-opt-out
    # Build image locally (no need to try pull from a remote registry):
    pull_policy: build
    build:
      dockerfile_inline: |
        FROM alpine
        RUN install --mode 770 --directory /var/lib/sqlite
        COPY --from=oryd/hydra:v2.2.0 /usr/bin/hydra /usr/bin/hydra
        # `--chmod` only required when container user is not run as root,
        # Volume mounting the config instead is fine as `git clone` permissions use umask which should result in 644.
        ADD --chmod=640 https://raw.githubusercontent.com/ory/hydra/refs/heads/master/contrib/quickstart/5-min/hydra.yml /etc/hydra/config.yml

So that works well, and if the user bind mounts a host directory instead, they can just set the user field to match UID of their host user. This avoids needing to set permissions of /var/lib/sqlite to 770, or know the UID/GID for the containers ory user/group is so that they can chmod their host directory ownership to that for compatibility 👍 (both inconvenient)

Copy link
Member

Choose a reason for hiding this comment

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

Nice! The dockerfile_inline would probably need to build the binary inline as well, otherwise the binary type might mismatch (osx versus linux for example)

HEREDOC

USER ory
COPY hydra /usr/bin/hydra
Copy link
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
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

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
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

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
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
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