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

parse XML configuration files #159

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jokeyrhyme
Copy link

@jokeyrhyme jokeyrhyme commented Oct 27, 2024

based on reading https://dbus.freedesktop.org/doc/dbus-daemon.1.html#configuration_file

Fixes #78 and #79 and #148

Related to #23 and #146

TODOs for follow-up PRs:

  • <selinux>
  • <associate>
  • <apparmor>
  • protect against circular <include> / <includedirs>
  • <include if_selinux_enabled=...>
  • <include selinux_root_relative=...>
  • <listen> better than String values (e.g. parse and validate into more useful type)
  • <auth> better than String, is there an enum of specific values we support
  • validate/warn when using < standard_session_servicedirs /> in a system config
  • validate/warn when using <standard_system_servicedirs /> in a session config
  • validate DOCTYPE
  • validate root element is <busconfig>
  • implement Ord / PartialOrd trait for Policy to facilitate use during evaluation
  • provide default XML configuration files

@jokeyrhyme jokeyrhyme marked this pull request as draft October 27, 2024 03:29
@jokeyrhyme jokeyrhyme changed the title 78 parse xml configuration files parse XML configuration files Oct 27, 2024
@jokeyrhyme jokeyrhyme force-pushed the 78-parse-xml-configuration-files branch from bcea200 to 07e1119 Compare October 29, 2024 06:35
@jokeyrhyme jokeyrhyme force-pushed the 78-parse-xml-configuration-files branch 2 times, most recently from 9320376 to 6402177 Compare November 10, 2024 01:22
Cargo.toml Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly have nitpicks but overall it looks pretty good already. Let's try and get it merged soon.

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 15, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 15, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 15, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 15, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 15, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 16, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 16, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 16, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 16, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 16, 2024
jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 16, 2024
@jokeyrhyme jokeyrhyme marked this pull request as ready for review November 17, 2024 10:46
@jokeyrhyme
Copy link
Author

As mentioned over on Matrix, I propose that we tackle AppArmor/SELinux support in a follow-up PR

Aside from that, I believe we've got a mostly-complete implementation of the XML configuration format, although we don't handle all the same error cases as in #23 or #146

@zeenix
Copy link
Collaborator

zeenix commented Nov 17, 2024

As mentioned over on Matrix, I propose that we tackle AppArmor/SELinux support in a follow-up PR

Sure thing.

Aside from that, I believe we've got a mostly-complete implementation of the XML configuration format

Awesome! I'll have a look soon.

although we don't handle all the same error cases as in #23 or #146

That's ok, we can handle them later. If you could list them here, that would be great though, just to ensure that we don't end up leaving something very important/critical.

Also do we add tests to parse system and session configuration from /usr/share/dbus-1/system.conf and /usr/share/dbus-1/session.conf? I think we should and make it linux-specific.

@jokeyrhyme
Copy link
Author

Also do we add tests to parse system and session configuration from /usr/share/dbus-1/system.conf and /usr/share/dbus-1/session.conf? I think we should and make it linux-specific.

Do you mean I should copy the default configuration files from dbus-broker in as tests? Or should busd try to read these files by default if run with --session or --system ?

@zeenix
Copy link
Collaborator

zeenix commented Nov 17, 2024

Also do we add tests to parse system and session configuration from /usr/share/dbus-1/system.conf and /usr/share/dbus-1/session.conf? I think we should and make it linux-specific.

Do you mean I should copy the default configuration files from dbus-broker in as tests? Or should busd try to read these files by default if run with --session or --system ?

So busd should most definitely read them from those paths and later when we've some build configuration (#77), we can make the paths configurable at build time too. I was saying we should have tests that also try to load from those paths on Linux but we can also copy them into the repo to keep the tests self-contained (then the tests don't need to be linux-only).

@jokeyrhyme
Copy link
Author

I've updated the TODO checklist in the PR description

jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 18, 2024
@jokeyrhyme jokeyrhyme marked this pull request as draft November 18, 2024 09:18
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
zeenix pushed a commit to jokeyrhyme/busd that referenced this pull request Nov 27, 2024
src/config.rs Outdated Show resolved Hide resolved
@jokeyrhyme
Copy link
Author

test config_read_file_with_includes_ok ... FAILED

Weird, this passed on my machine, I'll have to take a look

jokeyrhyme added a commit to jokeyrhyme/busd that referenced this pull request Nov 29, 2024
Copy link
Collaborator

@zeenix zeenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote on the matrix channel:

  • This PR now breaks busd on launch. We need to add handling of tmpdir already (could be done in a separate PR first that this PR can be rebased on after that's merged).
  • It probably makes sense at this point to just merge most commits in this branch.

tests/config.rs Show resolved Hide resolved
src/config/xml.rs Outdated Show resolved Hide resolved
@jokeyrhyme jokeyrhyme force-pushed the 78-parse-xml-configuration-files branch 2 times, most recently from 82a5f7b to 57ae57a Compare November 30, 2024 00:30
This crate offers a serde-compatible Deserializer for XML data
This is inspired by @elmarco 's work over in dbus2#23

Importantly, by starting with this intermediate representation,
we can preserve the intended order of elements
@jokeyrhyme jokeyrhyme force-pushed the 78-parse-xml-configuration-files branch from 57ae57a to 102cf3a Compare November 30, 2024 01:10
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.

Ability to parse D-Bus configuration file(s)
2 participants