Skip to content
This repository was archived by the owner on Apr 24, 2024. It is now read-only.

SCMOD-12755: Allow startup scripts to run as non-root user.#23

Open
dgibson-microfocus wants to merge 7 commits intodevelopfrom
SCMOD-12755
Open

SCMOD-12755: Allow startup scripts to run as non-root user.#23
dgibson-microfocus wants to merge 7 commits intodevelopfrom
SCMOD-12755

Conversation

@dgibson-microfocus
Copy link
Copy Markdown

@dgibson-microfocus dgibson-microfocus commented Mar 9, 2021

@buildmachine-sou-jenkins2
Copy link
Copy Markdown
Contributor

RUN chmod +x /startup/* /startup/startup.d/*
COPY cert-scripts/install_ca_cert.sh /startup/startup.d/install_ca_cert-base.sh
RUN chmod -R +rx /startup/* /startup/startup.d/*
COPY permissions/caf /etc/sudoers.d/caf
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm don't really like the file being called caf. It doesn't project that the permissions are required for the install-ca-cert-base.sh script. Can we change the filename to install-ca-cert-base? And can we change the folder name from permissions to sudoers.d?

ADD https://raw.githubusercontent.com/CAFapi/caf-common/v1.19.0/container-cert-script/install-ca-cert.sh \
/startup/startup.d/install-ca-cert-base.sh
RUN chmod +x /startup/* /startup/startup.d/*
COPY cert-scripts/install_ca_cert.sh /startup/startup.d/install_ca_cert-base.sh
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The output filename is now install_ca_cert-base.sh. I don't like the mix of underscores and dash. Just keep it as install-ca-cert-base.sh like it was before. Also change the filename in the source code to match that and change the folder name from container-cert-script to startup.d, and put it inside the startup folder. Come to think of it, if it's in the startup folder like that you may not need the line at all since the COPY startup /startup line above will probably cover it.

log "The RUNAS_USER environment variable is not set, subsequent commands will be run as the default user."
exec "$@"
fi
exec "$@"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's put the comment back so that it's back the way it was before the gosu changes.

Suggested change
exec "$@"
# Execute the specified command
exec "$@"

@@ -0,0 +1 @@
ALL ALL=(ALL) NOPASSWD: /bin/cp * /etc/pki/trust/anchors*, /usr/sbin/update-ca-certificates
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the space at the start of the line necessary? It just looks strange to me (but I don't know the syntax so keep it if it's normal).

else
echo "Not installing CA Certificate."
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Drop this blank line.

Copy link
Copy Markdown
Contributor

@dermot-hardy dermot-hardy left a comment

Choose a reason for hiding this comment

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

Does this actually work? I tried it on Moe and I'm not seeing it working:

image

@dgibson-microfocus
Copy link
Copy Markdown
Author

dgibson-microfocus commented Mar 15, 2021

Does this actually work? I tried it on Moe and I'm not seeing it working:

image

@dermot-hardy sudo checks the passwd file to resolve the uid to a username for permissions checking which is why you're getting that error. From what I can tell, either the user needs to be created on the container or mount your passwd file into the container

@dermot-hardy
Copy link
Copy Markdown
Contributor

That is a domain user so it won't exist in the passwd file. Is there a way to make this work with anonymous users?

@dgibson-microfocus
Copy link
Copy Markdown
Author

dgibson-microfocus commented Mar 16, 2021

That is a domain user so it won't exist in the passwd file. Is there a way to make this work with anonymous users?

@dermot-hardy sudo will always use your real UID for permissions checks so it must map to a user who exists on the container. Setting the SetUID flag only works on binaries but some of the setup steps we need root permissions for are scripts (e.g. update-ca-certificates) We could add our own binary to execute arbitrary files similar to sudo but that seems like a big can of worms.

@dermot-hardy
Copy link
Copy Markdown
Contributor

Let's try to keep it simple but if we have to create a binary that wraps update-ca-certificates then so be it.

@dgibson-microfocus
Copy link
Copy Markdown
Author

Let's try to keep it simple but if we have to create a binary that wraps update-ca-certificates then so be it.

@dermot-hardy A possible alternative is we create a default non-root user in the base image and then use docker namespace remapping though I don't know how practical that would be in prod.

@dermot-hardy
Copy link
Copy Markdown
Contributor

That still leaves all containers running as the same user. It's an improvement because it's not the root user but it's still not the arbitrary user being requested here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants