| Summary: | Enable/disable the Stats interface without recompiling | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Alban Crequy <alban.crequy> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | enhancement | ||
| Priority: | medium | CC: | msniko14, smcv, thiago, walters |
| Version: | 1.5 | Keywords: | patch |
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | review? | ||
| i915 platform: | i915 features: | ||
| Attachments: |
[PATCH 1/4] Document <interface> in the dbus-daemon man page
[PATCH 2/4] Add interface element to the bus config dtd [PATCH 3/4] Add interface element support to the bus config parsing [PATCH 4/4] bus driver: check whether the interface is enabled [untested] Enable Stats interface by default; disallow non-root use on system bus [PATCH 1/2] Enable Stats interface by default; disallow non-root use on system bus [PATCH 2/2] config: add examples to show how to enable/disable the Stats interface [PATCH 2/2] config: add examples to show how to enable/disable the Stats interface |
||
|
Description
Alban Crequy
2014-07-01 16:01:28 UTC
Created attachment 102077 [details] [review] [PATCH 1/4] Document <interface> in the dbus-daemon man page Created attachment 102078 [details] [review] [PATCH 2/4] Add interface element to the bus config dtd Created attachment 102079 [details] [review] [PATCH 3/4] Add interface element support to the bus config parsing Created attachment 102080 [details] [review] [PATCH 4/4] bus driver: check whether the interface is enabled Comment on attachment 102077 [details] [review] [PATCH 1/4] Document <interface> in the dbus-daemon man page Review of attachment 102077 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 102078 [details] [review] [PATCH 2/4] Add interface element to the bus config dtd Review of attachment 102078 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 102079 [details] [review] [PATCH 3/4] Add interface element support to the bus config parsing Review of attachment 102079 [details] [review]: ----------------------------------------------------------------- Looks correct, but I'm not entirely sure about the XML parsing (not familiar anymore with the library). Comment on attachment 102080 [details] [review] [PATCH 4/4] bus driver: check whether the interface is enabled Review of attachment 102080 [details] [review]: ----------------------------------------------------------------- Looks good. I see now why you chose to implement it with an <interface> element. The code looks simpler with that. The implementation looks fine in principle, but I wonder whether we'd be better off using D-Bus' existing support for allowing/denying method calls instead:
<!-- Allow anyone to talk to the message bus -->
- <allow send_destination="org.freedesktop.DBus"/>
+ <allow send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.DBus"/>
<!-- But disallow some specific bus services -->
<deny send_destination="org.freedesktop.DBus"
send_interface="org.freedesktop.DBus.Debug.Stats"/>
<deny send_destination="org.freedesktop.DBus"
send_interface="org.freedesktop.DBus"
send_member="UpdateActivationEnvironment"/>
and further down the file:
+ <!-- uncomment, or add to a file in system.d, to let root
+ debug the system bus:
+ <policy user="root">
+ <allow send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.DBus.Debug.Stats"/>
+ </policy>
+ -->
I'm curious how much compiled size the Stats interface adds.
(In reply to comment #9) > The implementation looks fine in principle, but I wonder whether we'd be > better off using D-Bus' existing support for allowing/denying method calls > instead: As long as it can be enabled / disabled in the configuration file and SIGHUP, I am fine with it. > I'm curious how much compiled size the Stats interface adds. On x86_64: ./configure && make bus/dbus-daemon 1874194 ./configure --enable-stats && make bus/dbus-daemon 1891232 So the difference is ~17KB. And while running, it will use a bit more memory: - 6 * sizeof(int) in struct BusConnections, used only 1 time - 2 * sizeof(long) in struct DBusCounter, used in DBusConnection and DBusTransport It seems very reasonable to me. (In reply to comment #10) > So the difference is ~17KB. I think that's a sufficiently small difference that we should just mask it off with <deny/> rules in the default system bus configuration file, and consider compile-time-enabling it by default. (In reply to comment #10) > (In reply to comment #9) > > The implementation looks fine in principle, but I wonder whether we'd be > > better off using D-Bus' existing support for allowing/denying method calls > > instead: > > As long as it can be enabled / disabled in the configuration file and > SIGHUP, I am fine with it. I tested the changes in Comment #9 and changing the policy + SIGHUP worked fine. (In reply to comment #11) > (In reply to comment #10) > > So the difference is ~17KB. > > I think that's a sufficiently small difference that we should just mask it > off with <deny/> rules in the default system bus configuration file, and > consider compile-time-enabling it by default. I agree. Do you have a patch ready or do you want me to write one? I wonder about the user root: + <policy user="root"> Since I would like d-feet to use it: https://bugzilla.gnome.org/show_bug.cgi?id=735167 and d-feet should not run as root, does it worth adding a comment to say to replace "root" by another user? I wonder if there is an easy way to temporarily grant an user to access the interface, for a better integration in d-feet. gksudo does not help here. Created attachment 105819 [details] [review] [untested] Enable Stats interface by default; disallow non-root use on system bus --- I was planning to test this in a VM at some point, but if you get there first, so much the better! :-) > does it worth adding a comment to say to > replace "root" by another user? Already done, in fact. > I wonder if there is an easy way to temporarily grant an user to access the > interface, for a better integration in d-feet. gksudo does not help here. I think the way to do it is "developers should enable it for themselves". We could ship a sample file for system.d in /usr/share/doc/dbus/examples/enable-stats.conf? + <allow send_destination="org.freedesktop.DBus" + send_interface="org.freedesktop.DBus.Introspection"/> It should be "org.freedesktop.DBus.Introspectable" instead. Otherwise, the patch looks good. But I will test it a bit more... (In reply to comment #13) > We could ship a sample file for system.d in > /usr/share/doc/dbus/examples/enable-stats.conf? Yes. And I ponder about a sample file for session.d to disable stats: distributions that want to isolate apps with SELinux or AppArmor would reuse it. When the "Introspectable" typo is fixed, the patch works fine for me. (In reply to comment #14) > And I ponder about a sample file for session.d to disable stats: > distributions that want to isolate apps with SELinux or AppArmor would reuse > it. Good idea. Created attachment 106917 [details] [review] [PATCH 1/2] Enable Stats interface by default; disallow non-root use on system bus smcv's patch modified to fix typo in interface name Created attachment 106918 [details] [review] [PATCH 2/2] config: add examples to show how to enable/disable the Stats interface make distcheck pass, and @EXPANDED_SYSCONFDIR@ works, but not fully tested. Comment on attachment 106917 [details] [review] [PATCH 1/2] Enable Stats interface by default; disallow non-root use on system bus Review of attachment 106917 [details] [review]: ----------------------------------------------------------------- I still need to test this system-wide, but it looks good Comment on attachment 106918 [details] [review] [PATCH 2/2] config: add examples to show how to enable/disable the Stats interface Review of attachment 106918 [details] [review]: ----------------------------------------------------------------- Mostly looks good. ::: bus/Makefile.am @@ +42,4 @@ > config_DATA += system.conf > endif > > +doc_DATA = \ If you want this to be installed in $(docdir)/examples you need to use: exampledir = $(docdir)/examples example_DATA = \ ... @@ +215,4 @@ > install-data-hook: > $(mkinstalldirs) $(DESTDIR)$(configdir)/session.d > $(mkinstalldirs) $(DESTDIR)$(datadir)/dbus-1/services > + $(mkinstalldirs) $(DESTDIR)$(docdir)/examples This should be unnecessary if you do what I said above. Created attachment 107201 [details] [review] [PATCH 2/2] config: add examples to show how to enable/disable the Stats interface (In reply to comment #20) > If you want this to be installed in $(docdir)/examples you need to use: ... > This should be unnecessary if you do what I said above. Fixed both, tested with and without the configuration snippets, and pushed it. Fixed in git for 1.9.0, thanks! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.