-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Changes from all commits
031787c
04fa48b
8ed5090
f612685
51fc717
ed85d9a
4ada680
90f2518
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||||
|
||||
# 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Some platforms like OpenShift have a feature that uses the However, group/other permissions need to be adjusted for the DSN path regardless so that the 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't have macOS environment to verify, but the compose example above is just for a quick example. It copies the binary from the platform image it has, which AFAIK there is no macOS platform for registries/containers, they only publish windows and linux images? Unless you were referring to architecture like AMD64 vs ARM64? That shouldn't be an issue, the directive should be using the inferred platform for selecting the image to copy from. Equivalent to The example was focused on showing how much simpler the image is without catering to a non-root user, allowing the deployment to explicitly opt-in to non-root by choosing their own UID/GID, instead of needing to be mindful of what was chosen internally. That's possible since the image doesn't have any real dependency lock-in on a non-root user being configured. They could just as easily take the current non-root images and run them as root in the same manner. |
||||
HEREDOC | ||||
|
||||
USER ory | ||||
COPY hydra /usr/bin/hydra | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Context is lacking a bit for these Since the image itself doesn't have a build stage, it's rather vague what the I assume going forward both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Outside of docker compose and |
||||
|
||||
ENTRYPOINT ["hydra"] | ||||
CMD ["serve", "all"] | ||||
USER ory | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 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). |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Rather than $ 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 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 Anyone bind mounting a volume for local storage to the container is not going to have the 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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 |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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.
/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.user
option to set the user to switch to and gain the same benefits.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 viasetcap
(especially if the feature using it is optional). With root (including rootless Docker/Podman), the need forsetcap
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.