Fix multiple ACL handling bugs by joergsteffens · Pull Request #1875 · bareos/bareos

joergsteffens · 2024-07-01T18:45:40Z

ACL handling for commands have been wrong.
The UA detect a command, if all typed chars matches the start of a
command string. E.g. "w" => "whoami", ".cons" => ".consoles", ...
The ACL check also only used the typed part of the command,
so if your ACL denies the command ".consoles",
you could execute it with ".cons" (or ".co").
Now we check with the full command string against the ACL.

Fix handling ACL profiles

When using negative ACLs (ACLs starting with '!')
profiles could have overwritten this entries,
as the code did not distringuish between negative ACL and not found.

Now the ACL checking can have three states:
positive match found,
negative match found,
no match found.

Also unifies the ACL checking
and adding the ACL directive !*all*
to deny all (before a regex must be used (!.*).

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)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General

Is the PR title usable as CHANGELOG entry?
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Required backport PRs have been created
Correct milestone is set

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

Tests

Decision taken that a test is required (if not, then remove this paragraph)
The choice of the type of test (unit test or systemtest) is reasonable
Testname matches exactly what is being tested
On a fail, output of the test leads quickly to the origin of the fault

sebsura

Nice changes! I found some issues that i would like to see fixed.

core/src/dird/ua_acl.cc

core/src/dird/ua_cmds.cc

systemtests/tests/python-bareos/test_acl.py

core/src/dird/ua_acl.cc

joergsteffens · 2024-07-10T21:36:40Z

The test config-syntax-crash will now fail, as the Director will now produce a proper config error on invalid ACLs, instead of just ignoring them as before. The test have been introduced because of https://bugs.bareos.org/view.php?id=1175, leading to #410. @arogge do you remember, if there is anything more to this test? Otherwise I replaced the test with a small extra python-bareos test.

sebsura

I have not checked everything yet but these are things i noticed

systemtests/python-modules/bareos_unittest/json.py

core/src/dird/ua_cmds.cc

core/src/dird/ua_run.cc

sebsura

Thanks for your work!

ACL handling for commands have been wrong. The UA detect a command, if all typed chars matches the start of a command string. E.g. "w" => "whoami", ".cons" => ".consoles", ... The ACL check also only used the typed part of the command, so if your ACL denies the command ".consoles", you could execute it with ".cons" (or ".co"). Now we check with the full command string against the ACL.

When using negative ACLs (ACLs starting with '!') profiles could have overwritten this entries, as the code did not distringuish between negative ACL and not found. Now the ACL checking can have three states: positive match found, negative match found, no match found. Also unifies the ACL checking and adding the ACL directive "!*all*" to deny all (before a regex must be used ("!.*").

Make it work again with current python versions

Co-authored-by: Sebastian Sura <124262655+sebsura@users.noreply.github.com>

The usage strings are rewarped anyway, so manual formating chars ("\n", "\t") are only misleading.

Before an invalid ACL emits and error but has than be igorned. Now parsing stops at an error and mark the whole configuration as invalid.

This is now implemented like it has been documented.

The documenation and code comments have described that an empty WhereACL would accept all input, like "*all*". In fact, that has never (?) been implemented in that way, therefore we removed this special case and handle the WhereACL like all other ACLs.

In some automated systemtests, the path did contain the "~", which is considerd invalid in a WhereACL. Changed the WhereACL to circumvent that.

Invalid chars in WhereAcls are now properly marked as configuration errors. Therefore in the original test config-syntax-crash, the Bareos Director refuses to start (with a proper error message), instead of just ignoring the invalid entry. Instead of adapting the config-syntax-crash, the test for invalid chars is now added to the python-bareos tests and the config-syntax-crash removed.

Co-authored-by: Sebastian Sura <124262655+sebsura@users.noreply.github.com>

exit is an alias to quit, so handle exit identical as quit.

On ModifyJobParameters, check PluginOptionsACL and the WhereACL also for regexwhere.

As bareos#1875 and its backports (down to bareos-21) have been merged, we could simply the ReaR configuration by using a standard Profile.

As bareos#1875 and its backports (down to bareos-21) have been merged, we could simplify the ReaR configuration by using a standard Profile.

arogge assigned joergsteffens Jul 2, 2024

arogge added the bugfix label Jul 2, 2024

arogge requested a review from sebsura July 2, 2024 10:30

arogge added requires backport to 21 requires backport to 22 requires backport to 23 labels Jul 2, 2024

joergsteffens force-pushed the dev/joergs/master/acl-fix branch from 8500660 to 587c4c0 Compare July 2, 2024 13:26

sebsura requested changes Jul 8, 2024

View reviewed changes

joergsteffens force-pushed the dev/joergs/master/acl-fix branch from b37a33d to 07b2935 Compare July 9, 2024 15:52

joergsteffens requested a review from sebsura July 9, 2024 15:53

joergsteffens mentioned this pull request Jul 10, 2024

Update ReaR documentation #1871

Merged

11 tasks

joergsteffens force-pushed the dev/joergs/master/acl-fix branch 4 times, most recently from 78f3e2f to ba910ad Compare July 11, 2024 07:38

sebsura requested changes Jul 11, 2024

View reviewed changes

systemtests/python-modules/bareos_unittest/json.py Outdated Show resolved Hide resolved

core/src/dird/ua_cmds.cc Outdated Show resolved Hide resolved

core/src/dird/ua_run.cc Show resolved Hide resolved

joergsteffens force-pushed the dev/joergs/master/acl-fix branch from 71c9a0c to 5631ca1 Compare July 11, 2024 15:27

joergsteffens requested a review from sebsura July 11, 2024 15:29

sebsura approved these changes Jul 12, 2024

View reviewed changes

sebsura added this to the 24.0.0 milestone Jul 12, 2024

This was referenced Jul 12, 2024

Fix multiple ACL handling bugs #1890

Merged

Fix multiple ACL handling bugs #1891

Merged

Fix multiple ACL handling bugs #1892

Merged

joergsteffens and others added 6 commits July 12, 2024 14:41

gitignore: ignore __pycache__ everywhere

26b86b7

systemtests python-bareos: adapt list_unittests.py

f96a56c

Make it work again with current python versions

systemtests: adding more ACL tests

2af8a4e

Apply suggestions from code review

982c4cf

Co-authored-by: Sebastian Sura <124262655+sebsura@users.noreply.github.com>

joergsteffens and others added 17 commits July 12, 2024 14:41

systemtests python-bareos: extend ACL test

1aa8fe2

fix output format

942311f

core: remove manual format chars in cmd usage

0d983a9

The usage strings are rewarped anyway, so manual formating chars ("\n", "\t") are only misleading.

core: fix ACL parsing error handling

a14322c

Before an invalid ACL emits and error but has than be igorned. Now parsing stops at an error and mark the whole configuration as invalid.

core: WhereACL: adding missing check when modify job

65a5502

core: cleanup

ea24119

core: empty Where_ACL allows restoring everywhere

57ab8b2

This is now implemented like it has been documented.

docs: regenerate autogenerated files

3e59d0e

systemtests python-bareos: reuse operator profile

1dcc262

systemtests python-bareos: check WhereACL

d3eaeb3

systemtests: prevent problems with WhereACL path

faa34db

In some automated systemtests, the path did contain the "~", which is considerd invalid in a WhereACL. Changed the WhereACL to circumvent that.

Update systemtests/python-modules/bareos_unittest/json.py

fc4a8a2

Co-authored-by: Sebastian Sura <124262655+sebsura@users.noreply.github.com>

core: always allow 'exit' cmd

84dbca4

exit is an alias to quit, so handle exit identical as quit.

core: handle PluginOptionsACL and regexwhere

c38a9ac

On ModifyJobParameters, check PluginOptionsACL and the WhereACL also for regexwhere.

Update CHANGELOG.md

af86d44

BareosBot force-pushed the dev/joergs/master/acl-fix branch from 4fd5ecc to af86d44 Compare July 12, 2024 14:41

BareosBot merged commit 2a02669 into bareos:master Jul 12, 2024

joergsteffens added a commit to joergsteffens/bareos that referenced this pull request Jul 12, 2024

docs rear: use rear profile

6971823

As bareos#1875 and its backports (down to bareos-21) have been merged, we could simply the ReaR configuration by using a standard Profile.

joergsteffens added a commit to joergsteffens/bareos that referenced this pull request Jul 12, 2024

docs rear: use rear profile

7dcd940

As bareos#1875 and its backports (down to bareos-21) have been merged, we could simplify the ReaR configuration by using a standard Profile.

joergsteffens added a commit to joergsteffens/bareos that referenced this pull request Jul 12, 2024

docs rear: use rear profile

a90071a

As bareos#1875 and its backports (down to bareos-21) have been merged, we could simplify the ReaR configuration by using a standard Profile.

BareosBot pushed a commit to joergsteffens/bareos that referenced this pull request Jul 12, 2024

docs rear: use rear profile

986be40

As bareos#1875 and its backports (down to bareos-21) have been merged, we could simplify the ReaR configuration by using a standard Profile.

joergsteffens deleted the dev/joergs/master/acl-fix branch September 25, 2024 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiple ACL handling bugs#1875

Fix multiple ACL handling bugs#1875
BareosBot merged 23 commits intobareos:masterfrom
joergsteffens:dev/joergs/master/acl-fix

joergsteffens commented Jul 1, 2024 •

edited

Loading

Uh oh!

sebsura left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joergsteffens commented Jul 10, 2024

Uh oh!

sebsura left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebsura left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

joergsteffens commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please check

Helpful documentation and best practices

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

General

Source code quality

Tests

Uh oh!

sebsura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joergsteffens commented Jul 10, 2024

Uh oh!

sebsura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sebsura left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joergsteffens commented Jul 1, 2024 •

edited

Loading