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

Always use the same permissions for youki dir #705

Merged
merged 6 commits into from
Feb 15, 2022

Conversation

Szymongib
Copy link
Contributor

Summary

This PR aligns permissions of /run/youki directory to 700.

Relevant line in runc https://github.com/opencontainers/runc/blob/main/libcontainer/factory_linux.go#L49

Fixes #481

Signed-off-by: Szymon Gibała <szymongib@gmail.com>
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2022

Codecov Report

Merging #705 (347fb67) into main (b77cb8a) will decrease coverage by 0.18%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##             main     #705      +/-   ##
==========================================
- Coverage   70.77%   70.58%   -0.19%     
==========================================
  Files          84      100      +16     
  Lines       11144    11349     +205     
==========================================
+ Hits         7887     8011     +124     
- Misses       3257     3338      +81     

@utam0k
Copy link
Member

utam0k commented Feb 13, 2022

@Szymongib Thanks for your PR. First of all, this PR is enough valuable to merge. I think it would be better to add a unit test. WDYT?
I know there is a problem around the hardcoding path. However, If you do the same with rootless, you should be fine.
https://github.com/containers/youki/blob/9e8354394b97d63d3428485111031f5ecd547ed2/crates/libcontainer/src/rootless.rs#L100-L108

Signed-off-by: Szymon Gibała <szymongib@gmail.com>
@Szymongib Szymongib force-pushed the fix/always-use-same-permissions branch from 46753a8 to e085f42 Compare February 14, 2022 08:42
@Szymongib
Copy link
Contributor Author

Thanks, @utam0k. It is an interesting approach to mocking values for tests.

I have added tests for the whole determine_root_path function. Let me know what do you think.

@utam0k
Copy link
Member

utam0k commented Feb 14, 2022

Thanks, @utam0k. It is an interesting approach to mocking values for tests.

I have added tests for the whole determine_root_path function. Let me know what do you think.

It seems the ci got failed 😭 Can you check it?

@@ -64,7 +64,7 @@ jobs:
- run: sudo apt-get -y update
- run: sudo apt-get install -y pkg-config libsystemd-dev libdbus-glib-1-dev libelf-dev libseccomp-dev
- name: Run tests
run: cargo test --all --all-features --no-fail-fast
run: sudo cargo test --all --all-features --no-fail-fast
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to use sudo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry that was my attempt to fix it. The issue is that CI tests do not run as a root, therefore rootless_required always returns true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This, unfortunately, does not work, so I will try to find a way through it, otherwise, I will delete the test_determine_root_path_non_rootless test.

@Szymongib Szymongib force-pushed the fix/always-use-same-permissions branch from f57b1ef to 52e8c0b Compare February 14, 2022 17:39
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
@Szymongib Szymongib force-pushed the fix/always-use-same-permissions branch from 52e8c0b to 9af52a3 Compare February 14, 2022 17:39
Signed-off-by: Szymon Gibała <szymongib@gmail.com>
@Szymongib
Copy link
Contributor Author

Hey @utam0k, sorry for the inconvenience 😕 . I did not find a way to run tests as a root user so I added a check to skip the test if the user is not root https://github.com/containers/youki/pull/705/files#diff-6628fb893f86fbbacb0317d2125dc7777897c6eefac88ba293fb7e49bee75726R216-R219. Not sure if this is the best approach, let me know if you would prefer me to delete this test altogether.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@utam0k utam0k merged commit a108e8e into youki-dev:main Feb 15, 2022
@Szymongib Szymongib deleted the fix/always-use-same-permissions branch February 15, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Folder /run/youki mode is not 0700
3 participants