-
Notifications
You must be signed in to change notification settings - Fork 53
Add whippet #661
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
base: develop
Are you sure you want to change the base?
Add whippet #661
Conversation
26c1001
to
d0d9df6
Compare
Forced push includes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review covering build-related concerns.
packages/os/os.spec
Outdated
Provides: %{_cross_os}dbus-launcher = %{package_priority_epoch}: | ||
Conflicts: %{_cross_os}dbus-launcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using parentheses for the capabilities that aren't expected to match a package name:
Provides: %{_cross_os}dbus-launcher = %{package_priority_epoch}: | |
Conflicts: %{_cross_os}dbus-launcher | |
Provides: %{_cross_os}dbus-broker(launcher) = %{package_priority_epoch}: | |
Conflicts: %{_cross_os}dbus-broker(launcher) |
[[package]] | ||
name = "nix" | ||
version = "0.30.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update our pinned workspace version of nix so we don't need an extra version?
|
||
[[package]] | ||
name = "async-broadcast" | ||
version = "0.7.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we disable the async feature of zbus to avoid bringing in these additional dependencies? Not sure what async is really buying us in this context.
sources/Cargo.toml
Outdated
zbus = { version = "5.11", features = ["p2p"] } | ||
zvariant = "5.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do another pass over these dependencies and make sure we disable any features we don't actually need.
# zbus introduces newer versions of crates that are either | ||
# incompatible or way ahead from the versions used in other crates | ||
{ name = "zbus", version = "=5.11" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to try to resolve this now if we can or else document what's keeping us on older crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an issue to track these:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback before I dive deeper into the code
|
||
"whippet", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - it looks like this list is meant to be in alphabetical order (with constants
as an outlier...)
@@ -0,0 +1,25 @@ | |||
[package] | |||
name = "whippet" | |||
version = "0.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can give yourself some credit here 😄
version = "0.1.0" | |
version = "0.1.0" | |
authors = [ "..." ] |
sources/whippet/README.md
Outdated
*whippet* prevents the definition of invalid rules by using actual types, to prevent users from mixing | ||
fields for different rules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really follow the distinction you're drawing here. For the upstream launcher, the canonical policy format is XML, so it must be parsed and invalid constructs must be rejected at runtime. For whippet, the canonical policy format is TOML, so it must be deserialized and invalid constructs will be rejected at runtime.
Those seem essentially the same to me - we can't stop people from writing garbage policies, so we have to identify garbage and throw it out.
I'd prefer to just provide examples of translating valid policies from XML to TOML so it doesn't come off as a language flex.
{ allow = true, user = "*" }, # This is a connect user rule | ||
{ allow = true, own = "*" }, # This is an own rule | ||
{ allow = true, send_destination = "*" }, # This is a send rule | ||
{ allow = true , receive_sender = "*"}, # This is a receive rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ allow = true , receive_sender = "*"}, # This is a receive rule | |
{ allow = true, receive_sender = "*"}, # This is a receive rule |
sources/whippet/README.md
Outdated
The original dbus-daemon allowed setting the user the daemon should run as. In whippet, the user for | ||
the bus is configured in the systemd unit file that runs the bus. This allows whippet to only care about | ||
starting the broker, and keeps its implementation simpler as it doesn't have to implement any sort of | ||
privilege-dropping mechanism. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you switch from talking about "the original dbus-daemon" to "the broker". I'd just stick to summarizing this implementation's key points, like "doesn't change users, runs with whatever user is configured in the systemd unit", since those are more important to integrators than a comparison with other implementations.
This also buries a potential security concern that should be made clear: unlike other implementations, if you start this launcher as root with full capabilities, that's what the broker will end up running with too.
sources/whippet/README.md
Outdated
|
||
#### Policy drop-in files | ||
|
||
The original dbus-daemon implmentation allows system services to provide their own custom policy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: probably worth a spell-checker pass
The original dbus-daemon implmentation allows system services to provide their own custom policy | |
The original dbus-daemon implementation allows system services to provide their own custom policy |
sources/whippet/README.md
Outdated
*whippet* implements the priority assignment as close as possible to what the dbus-launcher does. The dbus-launcher | ||
numbers the rules in a policy as a whole, considering all contexts in the order they appear In these policies, the | ||
same rules end up with a different index and therefore a different priority: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is your audience for this note? Policy authors? Dbus implementation experts?
It's confusing since I'm not sure it's actionable. You say "whippet implements the priority assignment as close as possible to what the dbus-launcher does" here and then go on to explain how it's different. But it's not clear how that difference matters in practice. Also, "as close as possible" implies "identical" unless you explain why it's not possible to mimic the behavior exactly.
Here again I think the documentation would be better written from the standpoint of what policy authors need to do (or not do) or need to be aware of, or what isn't supported that they might expect.
let users: Vec<String> = users_contexts.keys().cloned().collect(); | ||
for user in users { | ||
let uid = resolve_username_to_uid(&user)?.to_string(); | ||
let policy = users_contexts.shift_remove(&user).unwrap_or_default(); | ||
users_contexts.insert(uid, policy); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't very efficient. Instead I'd suggest draining the original users_contexts
and creating a new IndexMap, replacing the username with the resolved UID and the original rules.
sources/whippet/src/policy.rs
Outdated
/// ] | ||
/// ``` | ||
/// | ||
/// After this funciton is called, the result is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// After this funciton is called, the result is: | |
/// After this function is called, the result is: |
sources/whippet/src/policy.rs
Outdated
// Unlikely to get the error as the line above guarantees the user policy should be set | ||
let user_policy = self.user.as_mut().expect("Unexpected empty user context"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have this comment - is it "unlikely" or impossible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the comment just to explain that the expect
here doesn't do any harm, and it should be impossible for it to cause a panic. I looked at other places in Bottlerocket to see what was done before with this type of calls, and no comment was added to explain them so I'll remove the comment.
connect_rules.sort_by(|a, b| b.get_priority().cmp(a.get_priority())); | ||
let default_priority = connect_rules.first().map(|r| *r.get_priority()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
priority_floor
or priority_threshold
might be a better name - "default" is somewhat overloaded and confusing.
sources/whippet/src/policy.rs
Outdated
} else if *r.get_priority() > priority { | ||
priority = *r.get_priority(); | ||
true | ||
} else { | ||
false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we end up using a user's connect rule as the new priority threshold if it's greater than our initial threshold.
However, if we don't have an initial threshold, we don't enter this loop at all. That seems inconsistent, maybe incorrect. Why should we discard subsequent lower priority user rules in the first case but keep them unconditionally in the second case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! There was indeed a bug.
Forced push includes:
|
(Forced push includes rebase) |
whippet is a dbus-broker launcher that implements the minimal set of featured required by Bottlerocket to get a functional Bus, which includes: - D-Bus Policy parsing - Systemd socket activation Signed-off-by: Arnaldo Garcia Rincon <[email protected]>
whippet requires the bus_exec treatment for it to iteract with the dbus-broker. Signed-off-by: Arnaldo Garcia Rincon <[email protected]>
(Forced push fixes clippy failures) |
// easier for debugging | ||
dbus_policy | ||
.uid_entries | ||
.insert(0, (u32::MAX, default_batch.clone())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.insert(0, (u32::MAX, default_batch.clone())); | |
.insert(0, (DEFAULT_USER_ID, default_batch.clone())); |
{{ allow = true, send_interface = "org.example.Dummy" }} | ||
] | ||
"#, | ||
u32::MAX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u32::MAX | |
DEFAULT_USER_ID |
{{ allow = true, send_interface = "org.example.Dummy" }} # Rule 4 | ||
] | ||
"#, | ||
u32::MAX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u32::MAX | |
DEFAULT_USER_ID |
policy.optimize(); | ||
|
||
let dbus_policy: DbusPolicy = policy.try_into().unwrap(); | ||
assert_eq!(dbus_policy.uid_entries[0].0, u32::MAX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_eq!(dbus_policy.uid_entries[0].0, u32::MAX); | |
assert_eq!(dbus_policy.uid_entries[0].0, DEFAULT_USER_ID); |
Issue number:
Part of #660
Description of changes:
This adds
whippet
, a minimal implementation of a dbus-launcher. The set of features implemented by whippet include:Differences between the dbus launcher and whippet
No default connect rule for whippet’s user
One of the key differences between the launcher and whippet is that whippet doesn’t insert a default connect rule, the reason for this is twofold:
Drop support for deprecated or unused rule configurations
To keep whippet’s implementation as minimal as possible whippet only supports the rule fields that are actually used in Bottlerocket. Additionally, whippet removes fields marked as deprecated in the configuration file and policy rules, this includes
Rule priority assignment
In the dbus-launcher, rules are given a priority that is used to determine which rule wins when sending or receiving a message through the bus. The rules with the highest priorities win.
Whippet implements the priority assignment as close as possible to what the dbus-launcher does. However, there is a limitation on whippet’s implementation. The dbus-launcher assigns priorities to the rules in the order they appear, e.g. these rules:
Will get different priorities than the following rules, even though the actual permissions are identical:
This difference in order doesn’t affect the purpose of the policy, as the formula to calculate the rules guarantees that user rules will always get a higher priority to the default rules:
For a default rule to overlap with the next context, there would have to be at least
2635249153387078802
rules. Bottlerocket has 204 rules in total.Generated priorities
After re-arranging the configuration files for both whippet and the dbus-launcher to have the same order, and removing unused rules from the dbus-launcher configuration file (so that they are more deterministic), I confirmed:
Sample rules to demonstrate they got the same priority
Launcher
Whippet
NOTE FOR REVIEWERS
Reviewers are advised to review in this order:
main.rs
policy.rs
: contains information of how rules are assigned a priority, and provides examples and unit tests cases for better understandingdbus_policy.rs
: contains information of how the default rules are passed to all the users in a policybroker.rs
child.rs
Testing done:
In combination with #662:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.