Check the peer credentials when opening a remote socket#1580
Conversation
bjorn3
left a comment
bjorn3
left a comment
There was a problem hiding this comment.
I didn't review the parsing code and tests too closely.
| When managing enterprise-wide sudoers rules, it is sometimes preferable to store them in a centralized repository. The @socket directive can be used to include the contents provided by a server application over a Unix domain socket. For example, providing: | ||
|
|
||
| @socket /var/run/providers/sudoers.socket | ||
| @socket /var/run/providers/sudoers.socket (sssd:%sssd) |
There was a problem hiding this comment.
Requiring a % is inconsistent with Runas_Spec.
There was a problem hiding this comment.
Oh! Sorry. I followed a limited version of Runas_Member for each component.
So far I was accepting a single user and group. Do you want to accept a Runas_List for each of them?
Also, the user was mandatory, but I see that for Runas_spec the user is optional. Should I also accept an empty user?
There was a problem hiding this comment.
I'd say requiring exactly a single user and single group is fine for now.
It's just that the %grp is strange since elsewhere it means "a user belonging to the group grp"; e.g. I could see how:
@socket (%sssd:sssd) /socketpath
could make sense (it's owned by a user in the sssd group, and group-owned by sssd).
But allowing multiple users would, I think, complicate the ownership checks in the audit.rs side of things and we don't need to do that unless there's a good reason.
| peer_spec: Option<&'a ast::PeerSpec>, | ||
| } | ||
|
|
||
| fn include(cfg: &mut Sudoers, ctx: &mut IncludeContext) { |
There was a problem hiding this comment.
IncludeContext can be passed by-value, right?
There was a problem hiding this comment.
I think it is not possible because ctx.diagnostics needs to be mutable to call push(). Do you agree?
aplopez
changed the title
|
Hi, Is it possible to relaunch the failing test? It is not failing in my environment and, at first sight, it is not related to my changes. I' d like to check whether it fails again. |
|
Flaky tests sometimes occur. |
| skip_trailing_whitespace(stream)?; | ||
| // After skipping whitespaces, next char should be '\n', or '(' for a @socket peer spec | ||
| let next = stream.peek(); | ||
| if next != Some('\n') && next != Some('(') { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Scratch the above suggestion: the /etc/sudoers format has plenty of weird edge cases, but in this case we can make our own syntax.
Let's change it to @socket (user:group) path (with the same optionality as before). This also has a bit of similarity with the regular sudoers rule where the "RunAs" specifier goes before the command.
Much easier to parse.
This feature was introduced not long ago and needs to be applied also for the e2e socket tests.
The directive @socket now takes the expected user and possibly group that the peer at the other end of the socket must be running as. This information must preced the socket path and be enclosed in parentheses: @socket (user:group) /path/to/socket Numeric values are accepted preceded by #. Existing tests where updated and a new unit test was added for the parsing. Man pages were also updated.
3 participants
The PR fixes the TOCTOU introduced with the socket reading by checking the credentials of the peer process at the other side of the socket.
The directive is now:
These changes are introduced in the second commit. Tests and man pages are updated and a new unit test was added. Please see the commit message for more details.
The first commit makes remote sudoers 2e2 tests use the
unstable-remote-sudoersfeature that was previously introduced.The third commit is an add-on to the second one. Because the second commit added an 8th parameter to the
include()function,cargo clippycomplains about having too many parameters. This commit is my proposition to fix this, but I'm not sure you agree with it. Basically it introduces a newIncludeContextstructure where the parameters are stored and this context is passed as the second parameter (the first one being the configuration). If you agree with this solution, I will merge both commits into a single one as it is useless to have them separate.[Edit: The third commit was merged into the second one]
Resolves: #1549