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.

Sign up for GitHub

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

Jump to bottom

SD plugin scsicrypto check, set, unset cap_sys_rawio capabilities #1057

Merged
merged 17 commits into from
Feb 8, 2022
Merged

SD plugin scsicrypto check, set, unset cap_sys_rawio capabilities #1057

merged 17 commits into from
Feb 8, 2022

Conversation

bruno-at-bareos
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos commented Jan 26, 2022 •
edited by arogge
Loading

  • Platform: bareos-sd.service
    • Remove CapabilityBoundingSet=CAP_SYS_RAWIO as the daemon doesn't run with root.
    • Discourage the use of AmbientCapabilities due to the lack of flag support in systemd.
  • Packaging: add postinstall hooks for enabling cap_sys_rawio capabilities
  • Doc: review section about linux sg_io ioctl interface
    • Explain the usage of our bareos-config help and related scsicrypto functions
      • bareos-config check_scsicrypto_capabilities
      • bareos-config set_scsicrypto_capabilities
      • bareos-config unset_scsicrypto_capabilities

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
  • Commit descriptions are understandable and well formatted
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
  • bareos-check-sources --since-merge does not report any problems
  • git status should not report modifications in the source tree after building and testing

arogge
arogge requested changes Jan 26, 2022
View reviewed changes
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

I see some minor and easy-to-fix issues with the shell code.
However, this does not (yet) fix the upgrade-problem our users are facing.

core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
joergsteffens
joergsteffens reviewed Jan 26, 2022
View reviewed changes
Copy link
Member

@joergsteffens joergsteffens left a comment

Choose a reason for hiding this comment

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

As @arogge pointed out, you should a warning to the documentation, that the capabilities are removed by a package update.

So I read this section correctly, that I do not require the capability, if I run the b*-tools as root? Will it cause problems to run these commands as root?

docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlugins/ScsicryptoSd.rst.inc Outdated Show resolved Hide resolved
docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlugins/ScsicryptoSd.rst.inc Outdated Show resolved Hide resolved
docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlugins/ScsicryptoSd.rst.inc Outdated Show resolved Hide resolved
docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlugins/ScsicryptoSd.rst.inc Show resolved Hide resolved
@bruno-at-bareos
Copy link
Contributor Author

As @arogge pointed out, you should a warning to the documentation, that the capabilities are removed by a package update.

We will find a way to handle this the best as we can. Let discuss this today and choose a solution.

So I read this section correctly, that I do not require the capability, if I run the b*-tools as root? Will it cause problems to run these commands as root?
I didn't see any differences with capabilities set or not and the usage of tools with root user.

@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/docs-plugins-scsicrypto-sd branch 2 times, most recently from c88a980 to c0fc097 Compare January 27, 2022 15:12
@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/docs-plugins-scsicrypto-sd branch 3 times, most recently from 2146608 to 0ff303b Compare February 2, 2022 09:14
@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/docs-plugins-scsicrypto-sd branch 2 times, most recently from 63a1ace to 4073ff8 Compare February 3, 2022 12:48
@bruno-at-bareos
Copy link
Contributor Author

Final comment, after having tested in RHEL 8, openSUSE 15.3, Debian 11, I just fixed some typos errors in warn function and also mistyped the config file name in post macro. Now I'm confident everything looks ok.
Thanks for your final review.
If ok, I'm in favor of doing also a backport to 21.

@arogge
Copy link
Member

arogge commented Feb 7, 2022

The code looks good. However, as the implementation escalated quite a bit (from documentation-only to scripts and packaging changeS) the PR title and description don't reflect the changes anymore.

arogge
arogge requested changes Feb 7, 2022
View reviewed changes
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

There was some nitpicking left for me to do.
But other than a few text suggestions this looks great!
PR title and description need to be updated though.

docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlugins/ScsicryptoSd.rst.inc Outdated Show resolved Hide resolved
docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlugins/ScsicryptoSd.rst.inc Outdated Show resolved Hide resolved
docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlugins/ScsicryptoSd.rst.inc Outdated Show resolved Hide resolved
core/scripts/bareos-config-lib.sh.in Outdated Show resolved Hide resolved
@bruno-at-bareos bruno-at-bareos changed the title docs SD plugin scsicrypto SD plugin scsicrypto check, set, unset cap_sys_rawio capabilities Feb 8, 2022
@arogge arogge self-requested a review February 8, 2022 09:55
@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/docs-plugins-scsicrypto-sd branch 2 times, most recently from 29274fb to 0272788 Compare February 8, 2022 10:21
@bruno-at-bareos
Remove CapabilityBoundingSet=CAP_SYS_RAWIO from service
7faa87e 
- Remove CapabilityBoundingSet. SD daemon is not running as root.
  We don't use nor recommend the AmbientCapabilities due to
  the lack of systemd support for limited rights (+ep)

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Add three function to handle scsicrypto capabitilites
a6b58f1 
- check_scsicrypto_capabilities()
- set_scsicrypto_capabilities()
- unset_scsicrypto_capabilities()

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
docs: Scsicrypto
f305698 
- Add instruction for systemd AmbientCapabilities
- Add instructions for shell mode
- Add instructions for bareos-config.sh helper usage

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Use path variable
acebf2f 
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
script code improvements
ce1563b 
- Use lowercase variables, none used are going to env
- Use a safe list of binaries names, double quote any var used
- Return error earlier in function for non Linux operating system
- Improve test return of commands

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
bruno-at-bareos and others added 12 commits February 8, 2022 13:24
@bruno-at-bareos @joergsteffens
Update docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlug…
aea2a69 
…ins/ScsicryptoSd.rst.inc

Co-authored-by: Jörg Steffens <joergsteffens@users.noreply.github.com>
@bruno-at-bareos @joergsteffens
Update docs/manuals/source/TasksAndConcepts/Plugins/StorageDaemonPlug…
0f4412d 
…ins/ScsicryptoSd.rst.inc

Co-authored-by: Jörg Steffens <joergsteffens@users.noreply.github.com>
@bruno-at-bareos
Order alphabetically, add missing bextract and bcopy
edf1375 
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Add missing binaries, Add warning, Fix format
92d1fce 
- Order alphabetically binaries list.
- Add missing bcopy and bextract.
- Fix paragraph remove capabilities.
- Add warning about binary update loosing capabilities.
- Set a config file ${BAREOS_CONFIG_DIR}/.enable_capabilities
  to be used with pre/post packaging.
- Add chmod 0750 to binaries with capabilities to restrict their usage.

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
docs: update scsicrypto plugin chapter
6597ae8 
- Explain how to use helper function
- Add note about the support in packages updates

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Improve documentation and helper scripts
45e22d0 
- doc: better formulate capabilities introduction and systemd part
  Reorder helper set/unset/check presentation
- helper: reorder actions to chgrp, chmod, setcap

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Fix last bytes of documentation
55972fa 
    - Add cfg to code-block
    - Add :file: tag

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Rename config file for capabilities to .enable-cap_sys_rawio
6670748 
- Rename config file to explicitly show which capabilities is setup
- Add :file: tag in documentation for config file

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
scsicrypto postinst packaging integration
89556fd 
- Add function scsicrypto for postinstall in spec and debian.
- Add new debian/*.postinst files in .gitignore.
- Add instruction in documentation to have scscicrypto enabled at
  package install time by creating manually the file
  /etc/bareos/.enable-cap_sys_rawio file.
- Move manual setup before not recommended systemd paragraph in
  documentation.
- Dont fail function unset if setcap -r failed due to capabilities
  not set.

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Fix warning function call to warn in unset_
985f2c9 
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos @arogge
Apply suggestions from code review
8757119 
Co-authored-by: Andreas Rogge <andreas.rogge@bareos.com>
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos
Update CHANGELOG.md
550652f 
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/docs-plugins-scsicrypto-sd branch from 0272788 to 550652f Compare February 8, 2022 12:30
arogge
arogge approved these changes Feb 8, 2022
View reviewed changes
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

Great work! Thank you!

@arogge arogge merged commit 56a10bb into bareos:master Feb 8, 2022
@bruno-at-bareos bruno-at-bareos mentioned this pull request Feb 8, 2022
Merged
12 tasks
@bruno-at-bareos bruno-at-bareos deleted the dev/bruno/docs-plugins-scsicrypto-sd branch February 9, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joergsteffens joergsteffens joergsteffens left review comments

@arogge arogge arogge approved these changes

Assignees

@bruno-at-bareos bruno-at-bareos

Labels
requires backport to 21
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bruno-at-bareos @arogge @joergsteffens