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

cptbox: move logic from rule adding to constructor #999

Closed
wants to merge 1 commit into from

Conversation

Riolku
Copy link
Contributor

@Riolku Riolku commented Feb 28, 2022

Basically, instead of checking that rules are correctly written when they are compiled into the jail, we should just check at construction time.

@dmoj-build
Copy link
Collaborator

Can one of the admins verify this patch?

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2022

Codecov Report

Merging #999 (b9137ef) into master (749ab2f) will decrease coverage by 27.96%.
The diff coverage is 97.77%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #999       +/-   ##
===========================================
- Coverage   81.12%   53.15%   -27.97%     
===========================================
  Files         140      140               
  Lines        4753     4763       +10     
===========================================
- Hits         3856     2532     -1324     
- Misses        897     2231     +1334     
Impacted Files Coverage Δ
dmoj/cptbox/filesystem_policies.py 97.95% <97.36%> (-0.84%) ⬇️
dmoj/tests/test_filesystem_policy.py 100.00% <100.00%> (ø)
dmoj/executors/C.py 0.00% <0.00%> (-100.00%) ⬇️
dmoj/executors/D.py 0.00% <0.00%> (-100.00%) ⬇️
dmoj/executors/GO.py 0.00% <0.00%> (-100.00%) ⬇️
dmoj/executors/ADA.py 0.00% <0.00%> (-100.00%) ⬇️
dmoj/executors/AWK.py 0.00% <0.00%> (-100.00%) ⬇️
dmoj/executors/C11.py 0.00% <0.00%> (-100.00%) ⬇️
dmoj/executors/F95.py 0.00% <0.00%> (-100.00%) ⬇️
dmoj/executors/ICK.py 0.00% <0.00%> (-100.00%) ⬇️
... and 86 more

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 749ab2f...b9137ef. Read the comment docs.

@kiritofeng
Copy link
Member

ok to test

@Riolku Riolku force-pushed the change-filesystem-policies branch 3 times, most recently from a37cbc6 to 6fbb084 Compare February 28, 2022 15:35
@Riolku
Copy link
Contributor Author

Riolku commented Feb 28, 2022

This is marginally beneficial to landlock, since it checks the paths before they are even added to the jail. It doesn't end up mattering as of now because we construct the jail anyway, but it's probably better for the long term.

Basically, instead of checking that rules are correctly written when 
they are compiled into the jail, we should just check at construction 
time.
@Riolku
Copy link
Contributor Author

Riolku commented Mar 11, 2022

I'm confused: did you mean to merge the PR that depended on this, #1010? If so, I'll just close this (or you can).

@Riolku
Copy link
Contributor Author

Riolku commented Mar 11, 2022

I'm just going to close this. The commit has been merged anyways.

@Riolku Riolku closed this Mar 11, 2022
@Riolku Riolku deleted the change-filesystem-policies branch March 11, 2022 05:31
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.

None yet

5 participants