ast: change Tag.passwd into an enum #725
Conversation
| if skip_passwd { | ||
| *passwd = Some(false) | ||
| *authenticate = Authenticate::Nopasswd; |
There was a problem hiding this comment.
for the record, I've found this operation, and the associated tests, "strange" (for lack of a better word). first off, modifying the AST feels wrong; IME, after parsing, the AST should be considered an immutable data structure; rustc works like that.
I think the main issue is that there's no clear separation / delimination between the AST and "business" / "domain" level settings. whether 'pass auth is required' is a function of the sudoers policy, command line flags and who the invoking user is so that setting should not be stored in the AST but in a data structure that is created later in the pipeline.
in my mind, the pipeline should go like this: AST -?-> MIR -?-> Sudoers -> Context. AST is just a syntactically correct sudoers file. MIR is the sudoers AST after expanding aliases (+). Sudoers is a "sound" and processed set of settings and policies, e.g. it contains the final env_keep list rather than a list of operations to be performed on it. this Sudoers data structure then gets embedded in the Context that includes the invoking user, hostname, command line args, etc. and it's that Context what gets queried about auth requirements. sudo --list for example prints the MIR so it should have access to it but should not be able to modify it -- as that would invalidate the Sudoers data structure as well as the Context
the tests modified in the PR are doing Context-level queries so they shouldn't do assertions on AST-level types
(+) in rustc, this would be the HIR (or HAIR?) but we don't have that many immediate representations.
There was a problem hiding this comment.
This check does not modify the AST; the check_permission function takes the AST and a specific (context-sensitive request) and returns a "policy object" (which is not the ast but en encoding of a certain judgement).
Sadly there is an exception in sudo when someone is root and (since it is very important to get that right), it needed to be writen down very clearly, so this line modifies something in the resulting judgement (i.e. something that the pipeline that runs on top of this), not the AST (it also couldn't modify the AST since it doesn't have a mutable reference to it).
There was a problem hiding this comment.
I still think there's an organization / delimitation problem. Tag is in the ast module but it's appears to be directly treated as a MIR level object. a proper AST-level tag would be maintained as a list (vec) for each cmnd_spec so CWD=* PASSWD: NOPASSWD: /usr/bin/true would be parsed as having 3 tags: [Tag::Cwd(_), Tag::Nopasswd, Tag::Passwd]
a MIR level Tag would be a reduced and unfolded version, so the above cmnd_spec would only have the tags CWD and NOPASSWD (reduced). unfolding happens in NOPASSWD: /usr/bin/false, /usr/bin/true which unfolds into NOPASSWD: /usr/bin/false, NOPASSWD: /usr/bin/true. IMO, ast::Tag makes more sense as an enum; mir::Tag makes more sense as a struct (like the existing one)
lastly, I wouldn't modify Tag here but rather store the skip_passwd in the Judgment and have a Judgment::needs_passwd method that checks skip_passwd and then falls back to checking the Tag field. if the tests called Judgment::needs_passwd instead of reading a modified MIR-level Tag they'd feel less like they are punching through the abstraction.
There was a problem hiding this comment.
To me this does not feel like punching through an abstraction (some other things do) but is the thing being abstracted by the Policy trait.
The skip_passwd logic basically can happen in three spots: check_permission (but that is cleaner to read if it does only the general logic and not the exceptions), the higher-level function that constructs the (opaque to the outside world) Judgement object, or in the trait implementation (but then it is in a completely different file, and the skip_passwd needs to be kept around for longer, increasing the total state). Functionally it doesn't matter that much which one is used.
Right now the earlier parts (unfolding of aliases and tags) happens on-the-fly, which is quite efficient. I don't feel don't need a two- or three-pass compiler just to parse and check sudoers (feels like added complexity).
|
Number of dependencies and binary size impact report
Dependencies diff └─ sudo-rs [v0.2.0-dev.20230711]
├─ glob [v0.3.1]
├─ libc [v0.2.147]
└─ log [v0.4.19] |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #725 +/- ##
=======================================
Coverage 54.33% 54.33%
=======================================
Files 71 71
Lines 9635 9642 +7
=======================================
+ Hits 5235 5239 +4
- Misses 4400 4403 +3
☔ View full report in Codecov by Sentry. |
as suggested in https://github.com/memorysafety/sudo-rs/pull/721/files#r1278055634
depends on #721