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.

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

Fix Dockerfile to have the proper permissions in directories #4967

Merged
merged 28 commits into from
Jun 7, 2024

Conversation

amartinezfayo
Copy link
Member

In the Dockerfile, proper permissions in directories were intended to be set in the install commands that are run when setting up the directories. But those directories are then copied using the COPY instruction.
With the COPY instruction, all files and directories copied from the build context are created with a UID and GID of 0 unless the --chown flag specifies a given username, groupname, or UID/GID combination to request specific ownership of the copied content.

This PR fixes the Dockerfile to use --chown and --chmod flags in the COPY instructions to set the permissions.

Fixes #4903.
Supersedes #4871.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Copy link
Member

@maxlambrecht maxlambrecht left a comment

Choose a reason for hiding this comment

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

I've seen that several test suites currently create workload entries using the selector "unix:uid:0":

  • ghostunnel-federation
  • join-token
  • envoy-sds-v3-spiffe-auth

Should we also update these to use a "unix:uid:1000"?

@amartinezfayo
Copy link
Member Author

Should we also update these to use a "unix:uid:1000"?

Thank you for the observation, @maxlambrecht. Yes, I've already added a commit to have that fixed.
Unfortunately, there are still errors caused by "permission denied" issues that I'm looking into.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
…on test

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Dockerfile Outdated
CMD []
COPY --link --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
COPY --link --from=builder --chown=${spireuid}:${spiregid} --chmod=777 /empty-dir /opt/spire
Copy link
Member

Choose a reason for hiding this comment

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

Could we use separate stages for creating the directories and then apply the permissions when they are copied to the final images? Something like this:

# Setting up SPIRE Server directories
FROM alpine as prep-server
RUN mkdir -p /opt/spire/bin \
    /etc/spire/server \
    /run/spire/server/private \
    /tmp/spire-server/private \
    /var/lib/spire/server

# Setting up SPIRE Agent directories
FROM alpine as prep-agent
RUN mkdir -p /opt/spire/bin \
    /etc/spire/agent \
    /run/spire/agent/public \
    /tmp/spire-agent/public \
    /var/lib/spire/agent

# SPIRE Server
FROM spire-base AS spire-server
ARG spireuid=1000
ARG spiregid=1000
USER ${spireuid}:${spiregid}
COPY --link --from=prep-server --chown=${spireuid}:${spiregid} --chmod=755 / /
COPY --link --from=builder --chown=${spireuid}:${spiregid} --chmod=755 /spire/bin/static/spire-server /opt/spire/bin/
ENTRYPOINT ["/opt/spire/bin/spire-server", "run"]

# Similar configuration for SPIRE Agent

Copy link
Member

Choose a reason for hiding this comment

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

Also spire-base could be just:

FROM --platform=${BUILDPLATFORM} scratch AS spire-base
COPY --link --from=builder --chown=root:root --chmod=755 /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
WORKDIR /opt/spire

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @maxlambrecht for the suggestions, I think that they make sense.
I've updated the Dockerfile accordingly.

…ration test to be able to access the Docker daemon socket

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Dockerfile Outdated
Comment on lines 40 to 45
FROM alpine as prep-server
RUN mkdir -p /opt/spire/bin \
/etc/spire/server \
/run/spire/server/private \
/tmp/spire-server/private \
/var/lib/spire/server
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that my earlier suggestion would include the entire alpine filesystem in our final image, which isn't ideal. Here's an optimized approach:

FROM alpine as prep-server
RUN mkdir -p /spire/opt/spire/bin \
             /spire/etc/spire/server \
             /spire/run/spire/server/private \
             /spire/tmp/spire-server/private \
             /spire/var/lib/spire/server

We should only copy the /spire directory:

COPY --link --from=prep-server --chown=${spireuid}:${spiregid} --chmod=755 /spire /

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right. Yeah, I thought about that at some point and then didn't check it properly.
Thanks again for the suggestions!

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Copy link
Member

@maxlambrecht maxlambrecht left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
@@ -111,6 +111,10 @@ spec:
hostNetwork: true
dnsPolicy: ClusterFirstWithHostNet
serviceAccountName: spire-agent
# Make sure that we can create the directory for the socket in the host
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does only this one test need to run as root to create the socket directory? Could this be avoided with a config change in the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed because we use a hostPath volume to share the socket for the Workload API. I've moved the securityContext field to affect the spire-agent container only.

@azdagron
Copy link
Member

Closing this for now. We can re-open and merge after 1.9.3 ships.

@azdagron azdagron closed this Mar 28, 2024
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
@amartinezfayo amartinezfayo reopened this Jun 4, 2024
@azdagron azdagron assigned azdagron and unassigned rturner3 Jun 4, 2024
@azdagron azdagron merged commit ecabb6c into spiffe:main Jun 7, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerfile does not honor the specified ${spireuid}:${spiregid} in the USER instructions
4 participants