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

tests: fix security-udev-input-subsystem test #11390

Merged

Conversation

sergiocazzolato
Copy link
Collaborator

Permission denied could also be returned instead of 'Operation not permitted'.

Permission denied could also be returned instead of 'Operation not
permitted'.
@sergiocazzolato sergiocazzolato added the Simple 😃 A small PR which can be reviewed quickly label Feb 14, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #11390 (80e03f8) into master (52b49e1) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11390   +/-   ##
=======================================
  Coverage   78.37%   78.37%           
=======================================
  Files         931      931           
  Lines      106681   106702   +21     
=======================================
+ Hits        83611    83633   +22     
+ Misses      17867    17866    -1     
  Partials     5203     5203           
Flag Coverage Δ
unittests 78.37% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
osutil/synctree.go 76.41% <0.00%> (-2.84%) ⬇️
asserts/asserts.go 91.84% <0.00%> (+0.26%) ⬆️
overlord/ifacestate/helpers.go 77.45% <0.00%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52b49e1...80e03f8. Read the comment docs.

Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

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

I'm afraid that this change is not what we want, because when the error is "Permission denied" it means that our cgroup configuration was not setup/enforced, and this is exactly what this test is for.

What we could do, is disabling this test altogether and create an issue for it.

@sergiocazzolato
Copy link
Collaborator Author

@mardy hi, thanks for taking a look, this is an old one, at some point @anonymouse64 confirmed that both messages could be expected, I don't remember the reason, I would add a comment in the test explaining the reason once Ian confirms that.

Comment on lines 83 to 85
# device cgroup is in effect (because of rtc) and the device isn't in the
# cgroup. DAC is evaluated before AppArmor so since the device wasn't added
# to the cgroup, we see a DAC denial.
Copy link
Member

Choose a reason for hiding this comment

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

the issue with this test is that the underlying assumption here was true for a long time in the kernel, but only on accident. As per talking with the security team (specifically jj), the order in which the kernel evaluates LSM's is not defined and it just so happened to be that the device cgroup would be evaluated first for along time with ubuntu kernels, but that has recently changed so it is no longer deterministic.

That being said, I think we need to change these tests at some point to instead manually craft apparmor and device cgroup policy such that the expected one fails and we get the right message. I think this specific change in this PR is okay as long as we file a JIRA issue or launchpad bug etc to have someone look into manually modifying the policies in these tests (note it's not just this one that fails like this, there are others which have this same assumption built into them)

Copy link
Contributor

Choose a reason for hiding this comment

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

@anonymouse64 thanks for extra insight into this!

Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

looks fine to me as a temporary change until someone has time to modify these tests to not have the wrong assumption that the evaluation order of LSMs in the kernel is non-deterministic

Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 83 to 85
# device cgroup is in effect (because of rtc) and the device isn't in the
# cgroup. DAC is evaluated before AppArmor so since the device wasn't added
# to the cgroup, we see a DAC denial.
Copy link
Contributor

Choose a reason for hiding this comment

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

@anonymouse64 thanks for extra insight into this!

Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

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

After Ian's explanation, I'm fine with the change :-) Thanks!

@mvo5 mvo5 merged commit bbb63c2 into snapcore:master Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
6 participants