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

[Bug]: RPM packaging enforces insecure permissions and default config values #3262

Closed
dmz-uk opened this issue Mar 2, 2023 · 8 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@dmz-uk
Copy link

dmz-uk commented Mar 2, 2023

Describe the bug

This bug concerns the RPM packaging defined by scripts/pkg/build_templates/opensearch/rpm/opensearch.rpm.spec and consists of two related problems: default permissions (lack of %attr) and lack of %config(noreplace) on config files/dirs expected to be modified according to documentation.

First problem:

The spec doesn't define default file/directory permissions in %defattr or in %attr for any of the config files/dirs. This means that the RPM packaging is enforcing the permissions present on the build system, leading to the following enforced permissions in the package:

02755 /etc/opensearch
02755 /etc/opensearch/jvm.options.d
 0755 (all packaged directories under /etc/opensearch)
 0644 (all packaged files under /etc/opensearch)

This leads to a situation where the security plugin emits insecure permissions warnings on startup:

Directory /etc/opensearch has insecure file permissions (should be 0700)
File /etc/opensearch/opensearch.yml has insecure file permissions (should be 0600)
...
(repeat for all config dir/files)

The configs will be reset to these permissions on package install/upgrade - any manual fix to secure defaults after install in production will be reset (potentially until a configuration manager corrects them again).

Second problem:

Subdirectories under /etc/opensearch have been defined in the spec with config files included under them. The configs are not marked as %config or %config(noreplace), meaning that the default values are enforced on package upgrade. As of 2.6.0, these consist of:

/etc/opensearch/opensearch-notifications-core/notifications-core.yml
/etc/opensearch/opensearch-notifications/notifications.yml
/etc/opensearch/opensearch-observability/observability.yml
/etc/opensearch/opensearch-performance-analyzer/agent-stats-metadata
/etc/opensearch/opensearch-performance-analyzer/log4j2.xml
/etc/opensearch/opensearch-performance-analyzer/opensearch_security.policy
/etc/opensearch/opensearch-performance-analyzer/performance-analyzer.properties
/etc/opensearch/opensearch-performance-analyzer/plugin-stats-metadata
/etc/opensearch/opensearch-performance-analyzer/rca.conf
/etc/opensearch/opensearch-performance-analyzer/rca_cluster_manager.conf
/etc/opensearch/opensearch-performance-analyzer/rca_idle_cluster_manager.conf
/etc/opensearch/opensearch-performance-analyzer/supervisord.conf
/etc/opensearch/opensearch-reports-scheduler/reports-scheduler.yml
/etc/opensearch/opensearch-security/action_groups.yml
/etc/opensearch/opensearch-security/allowlist.yml
/etc/opensearch/opensearch-security/audit.yml
/etc/opensearch/opensearch-security/config.yml
/etc/opensearch/opensearch-security/internal_users.yml
/etc/opensearch/opensearch-security/nodes_dn.yml
/etc/opensearch/opensearch-security/opensearch.yml.example
/etc/opensearch/opensearch-security/roles.yml
/etc/opensearch/opensearch-security/roles_mapping.yml
/etc/opensearch/opensearch-security/tenants.yml
/etc/opensearch/opensearch-security/whitelist.yml

If any of these files are modified, those changes will be lost on package upgrade. Although it's understood that many are just used to import initial settings into the indexes, it might reasonably be expected that changes to at least some of these files persist after upgrade.

To reproduce

Install opensearch from RPM, either manually or via YUM repository.

Expected behavior

  1. Secure permissions on config files/dirs under and including /etc/opensearch in the RPM packaging.
  2. Config files expected to be modified not reset on RPM package upgrade.

Sorry, I can't confirm corrections to the specfile as I don't have a build environment for this project, and I'm unsure of the intended behaviour of the plugin config settings on upgrade. Obviously setting %defattr has implications for all packaged files (especially Main dirs), so unfortunately it may require plugin subdirectories and files be specified individually in the spec. This leads to something like this (again, sorry, this is unconfirmed), which requires naming/splitting out plugin directories, which may make the packaging more brittle to change going forward:

# Config dirs/files
%attr(0700,%{name},%{name}) %dir %{config_dir}
%attr(0700,%{name},%{name}) %dir %{config_dir}/jvm.options.d

%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/%{name}.yml
%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/jvm.options
%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/log4j2.properties
%attr(0600,%{name},%{name}) %config(noreplace) %{data_dir}/rca_enabled.conf
%attr(0600,%{name},%{name}) %config(noreplace) %{data_dir}/performance_analyzer_enabled.conf

%attr(0700,%{name},%{name}) %dir %{config_dir}/opensearch-notifications
%attr(0700,%{name},%{name}) %dir %{config_dir}/opensearch-notifications-core
%attr(0700,%{name},%{name}) %dir %{config_dir}/opensearch-observability
%attr(0700,%{name},%{name}) %dir %{config_dir}/opensearch-performance-analyzer
%attr(0700,%{name},%{name}) %dir %{config_dir}/opensearch-reports-scheduler
%attr(0700,%{name},%{name}) %dir %{config_dir}/opensearch-security

%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/opensearch-notifications/*.yml
%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/opensearch-notifications-core/*.yml
%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/opensearch-observability/*.yml
%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/opensearch-performance-analyzer/*
%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/opensearch-reports-scheduler/*.yml
%attr(0600,%{name},%{name}) %config(noreplace) %{config_dir}/opensearch-security/*.yml
%attr(0600,%{name},%{name}) %{config_dir}/opensearch-security/*.example

Screenshots

If applicable, add screenshots to help explain your problem.

Host / Environment

No response

Additional context

No response

Relevant log output

No response

@dmz-uk dmz-uk added bug Something isn't working untriaged Issues that have not yet been triaged labels Mar 2, 2023
@gaiksaya gaiksaya removed the untriaged Issues that have not yet been triaged label Mar 2, 2023
@gaiksaya
Copy link
Member

gaiksaya commented Mar 2, 2023

[Triage] @peterzhuamazon Can you take a look? Thanks!

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Mar 2, 2023

Hi @dmz-uk thanks for the detailed post here.

As for the 1st problem here, I used - for the files as I want to keep them the same permission as when they are packaged. Not sure if my understanding is wrong so please correct me.
Sourcing here: http://ftp.rpm.org/max-rpm/s1-rpm-anywhere-specifying-file-attributes.html

 There are a couple other wrinkles to using the %attr macro. If a particular file attribute doesn't need to be specified, that attribute can be replaced with a dash "-" and %attr will not change it. Say, for instance, that a package's files are installed with the permissions correctly set, as they almost always are. Instead of having to go to the trouble of entering the permissions for each and every file, each file can have the same %attr: 

And for files being 644 and the folders being 755, that is standard.
The caused file permission warning you see in log is a long reported issue: opensearch-project/security#1465

I dont remember the exact reason now, but it might due to that this script in the security plugin still needs to modify things in /etc/opensearch. I do remember a PR was sent sometime before but could not find it.


The 2nd problem is more complicated as we are using this spec file to also build rpms when not all plugins are presenting. This means we cannot hardcode each config file for each plugin, or if any plugin not present, the build will surely showing error because it cannot find the config files.


Let me know what you think.

Thanks.

@dmz-uk
Copy link
Author

dmz-uk commented Mar 3, 2023

Thanks @peterzhuamazon your understanding is correct. Apologies for duplicating an existing issue, I'm used to working with packaging as its own primary truth, but I can see the intent is to wrapper the existing install.

As for the 2nd problem, after all my nonsense it should be as simple as:

%config(noreplace) %{config_dir}/opensearch-*

which might be sufficient, although it marks the entire of the plugin dir contents as config. Otherwise using the %install section to scan for plugin directories and populating a files list for finer grained control over what is and isn't config would be possible, if awkward.

@peterzhuamazon
Copy link
Member

Thanks @peterzhuamazon your understanding is correct. Apologies for duplicating an existing issue, I'm used to working with packaging as its own primary truth, but I can see the intent is to wrapper the existing install.

As for the 2nd problem, after all my nonsense it should be as simple as:

%config(noreplace) %{config_dir}/opensearch-*

which might be sufficient, although it marks the entire of the plugin dir contents as config. Otherwise using the %install section to scan for plugin directories and populating a files list for finer grained control over what is and isn't config would be possible, if awkward.

Yeah, your proposal is doable but the reason I didnt do that before is due to these reasons 😄

  1. At the time not all plugins are having opensearch- prefix
  2. As you said it will mark the folders, alongside all the content that is not config files, as config. I am not sure what will be the side effect, other than if changes are made, the files will preserved on the server.

Let me know what you think. I am not against having this change as a result.
We will be appreciate if you can make a PR/contribution as well 😄 .

Thanks.

@peterzhuamazon
Copy link
Member

Hi @opensearch-project/security could you take a look again on opensearch-project/security#1465 once you have time?
This issue has been going on for a long time.
It also related to when to retire / change the demo configuration script later on.

Thanks.

@bbarani
Copy link
Member

bbarani commented Mar 17, 2023

@peterzhuamazon looks like security team has changed the permissions now. Does that help fix this issue?

@peterzhuamazon
Copy link
Member

Hi @bbarani,

Seems like it would resolve some warnings at least for now.
We need to monitor the changes in new snapshots.
As of now, I think we can close this issue until more information surface.

Let me know what you think @dmz-uk.

Thanks.

@jordarlu
Copy link
Contributor

jordarlu commented Apr 4, 2023

Hi, @dmz-uk , I am gonna close this issue for now as it has been a while no dialogues ... please do not hesitate to reopen it if still needed.. thanks again!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

No branches or pull requests

5 participants