Use final matching rule when determining if a command is permitted #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before, a command was permitted only if there were no other rules denying it *anywhere* in the file. This is inconsistent with the original doas. As a side effect of the changes, this commit fixes another inconsistency where the original doas did not allow using the `nopass` option with `deny` rules, but rsudoas did.
localspook
commented
Dec 25, 2023
| type Error = String; | ||
|
|
||
| fn try_from(config: &str) -> Result<Self, Self::Error> { | ||
| pub fn parse_into_rules(config: &str) -> Result<Vec<Rule>, String> { |
There was a problem hiding this comment.
parse_into_rules() is now incorrectly indented, but if I change the whitespace the diff becomes really hard to read >_<
|
Good catch, this is something I noticed as well but I had already finished the current matching logic by that time. I'm still not sure if I should switch to doas's simpler matching logic... perhaps I should offer this as a compile time option. I'd like to gather more feedback from actual doas users. Thanks for the PR in any case, mind if I ask how you stumbled upon this project? |
TheDcoder
reviewed
Dec 31, 2023
| @@ -57,8 +57,9 @@ fn execute(opts: Execute) { | |||
| config_file = file; | |||
| }, | |||
| } | |||
| let config = std::fs::read_to_string(config_file).expect("Failed to read config"); | |||
| let rules = match Rules::try_from(&*config) { | |||
| // Make sure the config file ends with a newline. | |||
TheDcoder
reviewed
Dec 31, 2023
| @@ -308,7 +273,10 @@ pub mod config { | |||
| }, | |||
| Expect::OptionOrIdentity => { | |||
| match (&*token, pure) { | |||
| ("nopass", true) => rule.options.nopass = true, | |||
| ("nopass", true) => match rule.action { | |||
| RuleAction::Deny => return Err("The nopass option cannot be used with the deny action".to_string()), | |||
There was a problem hiding this comment.
What does doas do in this case?
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, a command is permitted only if there are no other rules denying it anywhere in the config file. This is inconsistent with the original doas. From the
doas.confmanpage:So, given the following config file:
doas permits
l33thaxorto runcmatrix, but rsudoas doesn't.The key changes are:
Rules::try_from(&str)is replaced withconfig::parse_into_rules, which returns an ordered list of rules.Rules::r#matchis replaced withRule::matches, which checks if an individual rule matches.As a side effect of the changes, this commit fixes another inconsistency where the original doas did not allow using the
nopassoption withdenyrules, but rsudoas did.