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

Run tomcat as non-root user #612

Merged

Conversation

jeanpommier
Copy link
Member

update based on PR #442

@jeanpommier
Copy link
Member Author

@pierrejego I believe we can merge this PR.

We will need to document in the migration notes that the mapstore writable datadir needs to be writable by UID/GID 999 (for the docker version)

Copy link
Member

@edevosc2c edevosc2c left a comment

Choose a reason for hiding this comment

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

Could you allow changing the UID and GID at build time?

You can do it with something like this:

ARG USER_GID=999
ARG USER_UID=999
RUN addgroup --gid ${USER_GID} tomcat && \
    adduser --system  -u ${USER_UID} --gid ${USER_GID} --no-create-home tomcat && \
    chown -R ${USER_UID}:${USER_GID} /usr/local/tomcat

@jeanpommier
Copy link
Member Author

Hi @edevosc2c . Can you explain why for ?

The idea is to run the georchestra official docker image, right ? Since this applies at build time, it means that to get different settings one would still have a build a custom image. I don't see the added value

@pmauduit
Copy link
Member

Can you explain why for ?

it looks like because of georchestra/georchestra#4071

@edevosc2c
Copy link
Member

edevosc2c commented Nov 13, 2023

Hi @edevosc2c . Can you explain why for ?

The idea is to run the georchestra official docker image, right ? Since this applies at build time, it means that to get different settings one would still have a build a custom image. I don't see the added value

It's a standard thing in docker images to allow anyone to set UID and GID at build time.
It doesn't hurt to add it.

On top of that, you have just one line/one place to change the UID and GID in case it's needed.

@edevosc2c
Copy link
Member

edevosc2c commented Nov 29, 2023

Note: Once #671 will be merged, there will be a need here to add after RUN mkdir -p /docker-entrypoint.d this line:

RUN chown tomcat:tomcat /docker-entrypoint.d

Otherwise, the copy in this script won't work: https://github.com/georchestra/mapstore2-georchestra/pull/671/files#diff-1b0bf6d59703af25ba177c67baa3686a4aa1846098b91e7ff32375e0f4eb43eaR10

@jeanpommier jeanpommier force-pushed the non-root-tomcat-user-master branch from bb9cf90 to c65b297 Compare September 24, 2024 13:52
@jeanpommier
Copy link
Member Author

Hi,
Sorry I had left this PR untidy. I've updated the PR according to your suggestions @edevosc2c.
Does it look OK to you ?

Dockerfile Show resolved Hide resolved
Copy link
Member

@edevosc2c edevosc2c left a comment

Choose a reason for hiding this comment

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

good for merging

@edevosc2c
Copy link
Member

We need to fix the CI before merge if possible.

@edevosc2c
Copy link
Member

hey @jeanpommier can you rebase against master? this should fix the CI because geosolutions fixed it: fe581a2#diff-7c5b1aec3fd401c4634f1d2e303fa0b61fef81dd181322616a7058e833328a50R74

jeanpommier added a commit to pi-geosolutions/mapstore2-georchestra that referenced this pull request Oct 30, 2024
Based on PR georchestra#442
Updated according to suggestions from @edevos on PR georchestra#612
@jeanpommier jeanpommier force-pushed the non-root-tomcat-user-master branch from c65b297 to b17539d Compare October 30, 2024 09:53
Based on PR georchestra#442
Updated according to suggestions from @edevosc2c on PR georchestra#612
@jeanpommier jeanpommier force-pushed the non-root-tomcat-user-master branch from b17539d to 4dd778e Compare October 30, 2024 09:56
@jeanpommier
Copy link
Member Author

hey @jeanpommier can you rebase against master? this should fix the CI because geosolutions fixed it: fe581a2#diff-7c5b1aec3fd401c4634f1d2e303fa0b61fef81dd181322616a7058e833328a50R74

Hi @edevosc2c
Done, thanks

@jeanpommier jeanpommier merged commit 43cc019 into georchestra:master Oct 31, 2024
1 check passed
@jeanpommier jeanpommier deleted the non-root-tomcat-user-master branch October 31, 2024 14:24
@edevosc2c
Copy link
Member

will probably be great to create a new mapstore release

@landryb
Copy link
Member

landryb commented Nov 12, 2024

there will be an rc for 2024.02 soon, see #714. unless you want to cut a release from the previous 'stable' branch.

@landryb
Copy link
Member

landryb commented Nov 12, 2024

@edevosc2c
Copy link
Member

Yes we want to backport that to previous versions. Some of our clients want this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants