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

[improve][sec] Add group pulsar and add user pulsar to it instead of root #21084

Merged
merged 3 commits into from
Dec 7, 2023
Merged

[improve][sec] Add group pulsar and add user pulsar to it instead of root #21084

merged 3 commits into from
Dec 7, 2023

Conversation

yaalsn
Copy link
Contributor

@yaalsn yaalsn commented Aug 29, 2023

Motivation

Currently, the user pulsar is in the root group, it would be better to use the non-root group to keep more safety.

Modifications

  • Add group pulsar (GID 10000)
  • Add user pulsar (UID 10000) to group pulsar

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 29, 2023
@Technoboy- Technoboy- added this to the 3.2.0 milestone Sep 5, 2023
@Technoboy- Technoboy- requested a review from tisonkun September 5, 2023 07:59
@Technoboy- Technoboy- closed this Sep 5, 2023
@Technoboy- Technoboy- reopened this Sep 5, 2023
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Oct 6, 2023
@zymap zymap closed this Dec 7, 2023
@zymap zymap reopened this Dec 7, 2023
@zymap
Copy link
Member

zymap commented Dec 7, 2023

Failed to build the image:

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.3:build (default) on project latest-version-image: Unable to build image [apachepulsar/pulsar-test-latest-version] : "The command '/bin/sh -c adduser -u 10000 --gid 0 --disabled-login --disabled-password --gecos '' pulsar' returned a non-zero code: 1" -> [Help 1]

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2023

Codecov Report

Merging #21084 (e89bd3e) into master (d099ac4) will increase coverage by 31.61%.
Report is 273 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21084       +/-   ##
=============================================
+ Coverage     36.79%   68.40%   +31.61%     
- Complexity    12223    32193    +19970     
=============================================
  Files          1698     1881      +183     
  Lines        130168   162542    +32374     
  Branches      14206    19987     +5781     
=============================================
+ Hits          47890   111193    +63303     
+ Misses        75951    42375    -33576     
- Partials       6327     8974     +2647     
Flag Coverage Δ
inttests 26.09% <ø> (+1.96%) ⬆️
systests ?
unittests 68.61% <ø> (+36.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1505 files with indirect coverage changes

@zymap zymap merged commit f25b082 into apache:master Dec 7, 2023
49 of 50 checks passed
@lhotari
Copy link
Member

lhotari commented Dec 7, 2023

Currently, the user pulsar is in the root group, it would be better to use the non-root group to keep more safety.

I think we need to revert and first do a PIP if we are doing this change. This change will break Openshift support. I'll provide more details as a follow-up.

@lhotari
Copy link
Member

lhotari commented Dec 7, 2023

This PR #13376 provides more context and the reference about why root group must be used for OpenShift support:

The Container user is always a member of the root group, so it can read or write files accessible by GID=0. Any command invoked by the Entrypoint will be executed with this unprivileged UID and GID pair. That means, it is an unprivileged user executing the commands and the UID that will be used during execution is not known in advance. From the technical design perspective, that means, directories and files that may be written to by processes in the Container should be owned by the root group and be read/writable by GID=0. Files to be executed should also have group execute permissions.

@michaeljmarshall
Copy link
Member

Why did we merge this without addressing my questions on the mailing list?

https://lists.apache.org/thread/34t3npt0368hppv1gk8wc808ofl786j3

I appreciate that you brought this to the mailing list @yaalsn, but we should not have merged this when there was active opposition and open questions.

michaeljmarshall pushed a commit that referenced this pull request Dec 8, 2023
…tead of root" (#21691)

Reverts #21084. Because the change breaks OpenShift support.
@yaalsn
Copy link
Contributor Author

yaalsn commented Dec 13, 2023

@michaeljmarshall Sorry, I should disscuss more.

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

Successfully merging this pull request may close these issues.

8 participants