Improve ownership and permissions of files in OpenSearch deb and rpm packages #3898
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The files installed in /usr/share/opensearch do not need to be owned by the opensearch user. The default ownership from Debian packages, root:root is fine and should be preferred. Remove the post-installation line that customized these permissions. Signed-off-by: Romain Tartière <[email protected]>
The configuartion files installed by the opensearch package can contain sensitive information such as clear-text passwords (e.g. to reach an LDAP directory to authenticate users). When installing from a tarball, the configuration file are only accessible to the owner and members of the group of the file. Adjust the post-installation script to setup permissions for these files similar to the ones of the tarball. Currently, OpenSearch create files in the configuartion directory on first start (opensearch.keystore), so we cannot change the configuration directory ownership to prevent the service from tinkering with these files. Signed-off-by: Romain Tartière <[email protected]>
The OpenSearch logs may contain sensitive data. In order to prevent random users from accessing log files, Debian packages often setup the log directory so that only the service can write to it, members of the adm group can read these files, and other users are not granted access. Adjust the log directory owner and permissions to match that behavior. Signed-off-by: Romain Tartière <[email protected]>
Other users on the system have no reason to access the OpenSearch raw data files. Make sure these files are only available to the user running the service. Signed-off-by: Romain Tartière <[email protected]>
Now that the deb permissions and files ownerships are better, adjust the rpm receipe is a similar fashion for a consistent experience accross different operating systems. Signed-off-by: Romain Tartière <[email protected]>
smortex
requested review from
dblock,
peterzhuamazon,
bbarani,
gaiksaya,
rishabh6788,
zelinh,
jordarlu,
prudhvigodithi,
Divyaasm and
tianleh
as code owners
Codecov Report
@@ Coverage Diff @@
## main #3898 +/- ##
==========================================
+ Coverage 92.03% 92.05% +0.01%
==========================================
Files 186 187 +1
Lines 5639 5662 +23
==========================================
+ Hits 5190 5212 +22
- Misses 449 450 +1 |
|
In deb we also set the initial permissions here: |
There was a problem hiding this comment.
Hi @smortex thanks for the contribution here.
I have some comments and once we clear these, I would need to run a full-ledge build on ci images with this code, to ensure it is running without issues.
Thanks.
Instead of adding and removing permission bits, use explicit values. Signed-off-by: Romain Tartière <[email protected]>
|
Thanks @smortex, once you address my last comment / open conversation, we can queue this up for merging. Bare in mind we might have a patch release before 2.10.0, so we might keep this PR open a bit until we enter the 2.10.0 release cycle, since there are a lot of essential changes on both deb/rpm. I will start the test on my side once all the existing conversation addressed. Thanks. |
scripts/pkg/build_templates/opensearch/deb/debmake_opensearch_install.sh
Show resolved
Hide resolved
They where changed unexpectedly in eb0e032 but while the change seems noop, better limit the number of changes to avoid unexpected regressions. Signed-off-by: Romain Tartière <[email protected]>
1 task
|
Hi @smortex , Add a comment and want to know if opensearch-project/OpenSearch#9447 needs to go in before this? Thanks. |
The presence of this bit on the files seems to have no consequence on the final package as the installed files do not have the SGID bit set. But for consistency with the rpm packaging, unset the bit before building the package. Signed-off-by: Romain Tartière <[email protected]>
smortex
force-pushed
the
deb-rpm-owner-permissions
branch
from
0b460e8 to
71c7c34
Compare
|
@peterzhuamazon I added a commit to have the .deb and .rpm packaging doing the same stripping of the SGID bit. I am good with this PR as it is now, no need to wait for opensearch-project/OpenSearch#9447 to be merged IMHO as we have all required workarounds here. When opensearch-project/OpenSearch#9447 is merged, we can do the cleanup in this repo in a new PR. |
peterzhuamazon
approved these changes
Aug 21, 2023
smortex
added a commit
to smortex/opensearch-build
that referenced
this pull request
Sep 1, 2023
Similar to the issue fixed in opensearch-project#3898, OpenSearch-Dashboards package has unexpected files owner and permissions. This ensure the installed files are not owner by the opensearch-dashboards user (preventing the program to overwrite itself with malicious code if the service has some kind of vulnerability), and make sure logs and data cannot be accessed by random users. Signed-off-by: Romain Tartière <[email protected]>
smortex
added a commit
to smortex/opensearch-build
that referenced
this pull request
Sep 11, 2023
Similar to the issue fixed in opensearch-project#3898, OpenSearch-Dashboards package has unexpected files owner and permissions. This ensure the installed files are not owner by the opensearch-dashboards user (preventing the program to overwrite itself with malicious code if the service has some kind of vulnerability), and make sure logs and data cannot be accessed by random users. Signed-off-by: Romain Tartière <[email protected]>
71 tasks
smortex
changed the title
peterzhuamazon
added a commit
to peterzhuamazon/opensearch-build
that referenced
this pull request
Sep 19, 2023
…kages (opensearch-project#3898)" This reverts commit 35db59d.
peterzhuamazon
pushed a commit
to peterzhuamazon/opensearch-build
that referenced
this pull request
Sep 19, 2023
…pensearch-project#3898) Signed-off-by: Romain Tartière <[email protected]>
peterzhuamazon
pushed a commit
to peterzhuamazon/opensearch-build
that referenced
this pull request
Sep 19, 2023
…pensearch-project#3898) Signed-off-by: Romain Tartière <[email protected]> Signed-off-by: Peter Zhu <[email protected]>
24 tasks
peterzhuamazon
pushed a commit
to peterzhuamazon/opensearch-build
that referenced
this pull request
Mar 14, 2024
…pensearch-project#3898) Signed-off-by: Romain Tartière <[email protected]> Signed-off-by: Peter Zhu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When installing OpenSearch from .deb or .rpm packages, the files permissions are unexpectedly relaxed allowing all local users to access all files from the OpenSearch, and allowing OpenSearch itself to update any file installed in the package.
This PR adjust the permissions so that the configuration, logs and data cannot be accessed by random users, and the application code cannot be updated by the opensearch user.
Issues Resolved
Fixes #3815