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

Move umask from code to service files #29708

Merged
merged 14 commits into from
Jan 17, 2022

Conversation

rdner
Copy link
Member

@rdner rdner commented Jan 5, 2022

Enhancement

What does this PR do?

Before this the umask value was hard-coded in libbeat. It caused
some confusion among the users since file permission configuration was
practically ignored by a beat on the level of the binary. It's been
decided that we move umask to the service files, so the distribution
is secured by default but it still allows the users to set the value
if they choose to.

Why is it important?

The issue started as a security concern here #14005

Then was addressed by adding a hard-coded umask for all files created by libbeat #14119

Later documented here #28347

We've got some feedback from our customers about confusion and lack of documentation. Examples can be found here #20584 (comment)

Checklist

  • My code follows the style guidelines of this project
    - [ ] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
    - [ ] I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • systemd unit sets umask to 0027 for both beats and elastic agent
  • launchd manifest contains umask property set to 0027 for both beats and elstic agent
  • shell scripts that start both elastic agent and beats contain umask 0027 command

I made sure that all current permission defaults in beats are secure (set to 0600):

libbeat

filebeat

How to test this PR locally

I configured filebeat to have a file output with 666 permissions.

For that one can use the following config section in the filebeat config file:

output.file:
   path: "/path/to/the/output/directory"
   filename: "output.log"
   permissions: 0666

Before this change this would results in a file with 640 permissions, after this change it's 644 which lets users to make files available to the world as they request.

After setting umask 027 prior to running the filebeat binary the output file is created with 640 as expected.

Related issues

@rdner rdner self-assigned this Jan 5, 2022
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 5, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 5, 2022

This pull request does not have a backport label. Could you fix it @rdner? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 5, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 5, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-14T10:00:22.605+0000

  • Duration: 203 min 21 sec

  • Commit: 52bfcc1

Test stats 🧪

Test Results
Failed 0
Passed 47848
Skipped 4284
Total 52132

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@rdner
Copy link
Member Author

rdner commented Jan 6, 2022

/test

@mergify
Copy link
Contributor

mergify bot commented Jan 13, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feature-move-umask-to-service-files upstream/feature-move-umask-to-service-files
git merge upstream/master
git push upstream feature-move-umask-to-service-files

rdner added 12 commits January 14, 2022 09:35
Before this the `umask` value was hard-coded in `libbeat`. It caused
some confusion among the users since file permission configuration was
practically ignored by a beat on the level of the binary. It's been
decided that we move `umask` to the service files, so the distribution
is secured by default but it still allows the users to set the value
if they choose to.
umask is not hard-coded anymore and the test assumes that.
@rdner rdner force-pushed the feature-move-umask-to-service-files branch from 1fb7853 to 0cfe073 Compare January 14, 2022 09:56
@rdner rdner marked this pull request as ready for review January 14, 2022 13:52
@rdner rdner requested a review from a team as a code owner January 14, 2022 13:52
@rdner rdner added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jan 14, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 14, 2022
@rdner rdner added the v8.1.0 label Jan 14, 2022
@rdner rdner requested a review from kvch January 14, 2022 13:55
@rdner rdner added the review label Jan 14, 2022
@rdner rdner requested review from faec and belimawr January 14, 2022 15:11
@rdner
Copy link
Member Author

rdner commented Jan 17, 2022

The failing e2e test does not seem to be related to the change:

[2022-01-14T14:21:40.576Z] ERRO[2022-01-14T14:21:40Z] Error executing command                       args="[--kubeconfig /tmp/test-3728211276/kubeconfig --namespace test-9e7ba835-424b-4811-aad9-4ef5808d7342 cp --no-preserve test-9e7ba835-424b-4811-aad9-4ef5808d7342/elastic-agent-kdb7r:/tmp/beats-events /tmp/test-1292283775/events]" baseDir=. command=kubectl env="map[]" error="exit status 1" stderr="error: unable to upgrade connection: container not found (\"elastic-agent\")\n"

So, I think it's safe to merge the PR.

@rdner rdner merged commit 4974c9d into elastic:master Jan 17, 2022
@rdner rdner deleted the feature-move-umask-to-service-files branch January 17, 2022 08:34
@rdner
Copy link
Member Author

rdner commented Mar 7, 2022

Detailed testing steps:

  1. Access a Linux machine
  2. Go to the terminal and run umask 002
  3. Confirm the current umask value with the command umask it should print 002
  4. Create the following config file filebeat.yml for filebeat:
filebeat.inputs:
  - type: log
    paths:
      - "/home/user/input/input.log"

output.file:
  path: "/home/user/output_folder/"
  filename: "output_file.log"
  permissions: 0666
  1. Make sure there is no file /home/user/input/input.log and /home/user/output_folder/ is empty.
  2. Download the filebeat binary (artefact) to the current directory
  3. Start filebeat with ./filebeat -e -c filebeat.yml in the same terminal session used in steps 2 and 3
  4. Write a line to /home/user/input/input.log, for example using this command:
echo "some message" >> /home/user/input/input.log
  1. You should see a new file in /home/user/output_folder/ with permissions 664
  2. Stop filebeat
  3. Remove all files in /home/user/output_folder/
  4. Run umask 027
  5. Repeat steps 7 and 8
  6. You should see a new file in /home/user/output_folder/ with permissions 640

@dikshachauhan-qasource
Copy link

Hi @jlind23

This ticket has been validated and found working fine. Below are the detailed observation and steps followed:

Before setting umask 027

  • Access your Linux machine.
  • Go to Terminal and check current/default umask settings at machine, by hitting command 'umask' at terminal.
  • Now download the filebeat artifact and make below proposed changes in your_filebeat.yml. Note Here, we are providing 666 permissions to output file.
filebeat.inputs:
- type: Filestream
  enabled: true
  paths:
       - /home/user/input_folder/*

output.file:
  path: "/home/user/output_folder/"
  filename: "output_file.log"
  permissions: 0666

  • Now save the changes at filebeat.yml and hit terminal with below command:
    ./filebeat -e -c your_filebeat.yml
  • Observe, filebeat service will start generating some log events.
  • Now at another terminal instance, hit below command to generate log file.
    echo "message" >> output.log
  • Go to the output.log file and confirm permissions available on this file.

Observation: output.log file is generated with 664 permissions.

After setting umask 027

  • Stop filebeat.
  • Delete the output.log file from output folder.
  • Perform same steps mentioned above, however at step 2 , this time we will reset umask settings to 0027 using following command at terminal : 'umask 0027' .
    Note: we need to reset umask setting before starting the filebeat service.

Observation: output.log file is generated with 640 permissions.

Screenshot:
Before umask setting:
Annotation 2022-03-07 224701

After umask setting:
Annotation 2022-03-07 224654

@rdner Thanks for helping us out in validating same.

Thanks
QAS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify review Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[filebeat] default owner of output file
4 participants