openbmc / sdbusplus

C++ bindings for systemd dbus APIs
Apache License 2.0
105 stars 82 forks source link

sdbusplus + polkit integration #71

Open MarkAEvans opened 2 years ago

MarkAEvans commented 2 years ago

We're attempting to recreate the following example in sdbusplus wrt the dbus call to polkit's "CheckAuthorization": Creating a D-Bus Service with dbus-python and Polkit Authentication

We need to make a “recursive” method call to another dbus method from within the handler of one dbus method. The key missing piece is that we need to pull out parameters from the original “sd_bus_message *” struct that is a parameter to the original call. The C++ wrapper stuff hides this parameter, and makes this difficult when in the context of the class-based handler.

This the sd_bus call to the polkit “CheckAuthorization” method is done from the handler of another dbus method handler. The sdbusplus class-based handler does not appear to have a way to get a reference to the “sd_bus_message *” that is needed to call sd_bus_message_get_sender(). The python example code has access to this parameter.

The sdbusplus/example/calculator-server.cpp example code. The “conceptual” thing we are trying to do is add a polkit check to the “multiply()” method (line 21) in the Calculator class. The multiply() method is called by generated code and part of the method call chain is the callback called by dbus, which is generated wrapper code that gets a sd_bus_message * parameter, but it does not appear to expose a method to get a reference to that parameter in the multiply() codepath.

In the other example in sdbusplus/example/asio-example.cpp, the “methodWithMessage(sdbusplus::message_t&)” is there. The “message_t” parameter appears to be EXACTLY what we need to be able to call sd_bus_message_get_sender(). Is there a way to get this parameter in a class-based call?

williamspatrick commented 2 years ago

We need to make a “recursive” method call to another dbus method from within the handler of one dbus method.

There isn't really any problem with doing this, in general except for a few considerations:

a. sd_bus doesn't allow you to call back into your own daemon. You will get an ELOOP. Doesn't sound applicable here. b. The secondary call has to complete within the client timeout of the original client call. The client side of dbus deals with timeouts. There isn't any way for the server to communicate back to the client "hey, I've got to make a bunch of secondary calls so you need to multiply your timeout by N". Hopefully the systemd/polkit stuff you're calling here is fast enough that this isn't a concern either. c. The generated bindings (ala calculator-server.cpp) is currently strictly synchronous. This means that your server will not be able to handle new requests while it is blocked waiting for the secondary request to polkit. For software-based dbus operations, like polkit, this still is probably not a serious concern. The ASIO support allows async (as it is in the name), but you have to write all your dbus interfaces "by hand". I'm also working on adding a C++20 co-routine, which will cover the generated bindings, so that any generated object can delegate an implementation function as async (by returning a co-routine instead of a response-value).

The key missing piece is that we need to pull out parameters from the original “sd_bus_message *” struct that is a parameter to the original call. The C++ wrapper stuff hides this parameter, and makes this difficult when in the context of the class-based handler.

I'm a little perplexed as to why you need the original message. The generated bindings already unpack all the parameters from the original message into function arguments for you. What is left in the original message that you need out of it? Assuming you've written the YAML with the right parameters directives, everything else should be done for you bind those bindings.

This the sd_bus call to the polkit “CheckAuthorization” method is done from the handler of another dbus method handler. The sdbusplus class-based handler does not appear to have a way to get a reference to the “sd_bus_message *” that is needed to call sd_bus_message_get_sender(). The python example code has access to this parameter.

Oh. You're not actually looking for message parameters. You're looking for meta-data from within the message object about who sent the request to you. I think I understand now.

Currently the bindings don't allow the message to be visible anywhere, but that could be changed. I had considered adding an interface like that as a hack to add async support to the bindings (if the message were erased by the handler then assume that the handler turned it into an async operation) but it seemed like a hack and co-routines have come along now.

These are the two locations where the message is saved prior to calling the handler (for properties and message handlers respectively): https://github.com/openbmc/sdbusplus/blob/7662fa6932c90c134ecc815be688957fa45e1c84/include/sdbusplus/sdbuspp_support/server.hpp#L33 https://github.com/openbmc/sdbusplus/blob/7662fa6932c90c134ecc815be688957fa45e1c84/include/sdbusplus/sdbuspp_support/server.hpp#L89

I don't see any reason we couldn't turn the auto m into a thread-local storage of some sort that the handlers could get at to pick apart additional values that aren't naturally passed to them. Hopefully this is something you'd be interested in working on and contributing?

The sdbusplus/example/calculator-server.cpp example code. The “conceptual” thing we are trying to do is add a polkit check to the “multiply()” method (line 21) in the Calculator class.

I'm not sure exactly what you're doing with polkit, but are you sure this is even the right thing to do? Are you trying to protect dbus interfaces or something else? If you're trying to protect dbus interfaces then dbus has ACLs already, which I think leverage polkit, and is handled by the broker without any additional support by you. There was work someone else in the community was driving to run daemons as non-root and they had to add some bbclass support (and examples) for supplying these ACLs in order to get that working.

The multiply() method is called by generated code and part of the method call chain is the callback called by dbus, which is generated wrapper code that gets a sd_bus_message * parameter, but it does not appear to expose a method to get a reference to that parameter in the multiply() codepath.

Unfortunately I can't really change the API of the pure-virtual functions without breaking all the existing implementations, so I can't just have the bindings feed it in as a new argument. The "expose it by a backdoor method that looks it up from a TLS" I mentioned above is probably the only way we can do it in the existing implementation. If this is valuable work to have, as I work on the co-routine support, I can put that on the table as well. I'm already planning some C++20 concept-based detection to figure out if the handler is sync/async by checking the return type for a co-routine or value. It wouldn't be too hard to also check the first argument for an optional sdbusplus::message_t. We're probably ~6 months away from having that all ready though.

In the other example in sdbusplus/example/asio-example.cpp, the “methodWithMessage(sdbusplus::message_t&)” is there. The “message_t” parameter appears to be EXACTLY what we need to be able to call sd_bus_message_get_sender(). Is there a way to get this parameter in a class-based call?

As I mentioned above, the ASIO code requires all your handlers to be written to be "by hand"; they don't perform any validation against YAML/XML dbus schema nor is there any capability to generate stubs. I was on hiatus from the project during the time that ASIO support was added but there was obviously some difference of opinion on how dbus interfaces should be written... but the net is that they're completely different in how to make a server than the generated bindings you're currently looking at.

MarkAEvans commented 2 years ago

Thanks for the tips @williamspatrick. Much appreciated. Just some quick background. We're looking to integrate polkit into SDBusPlus so that we can provide an Authorization layer to dbus services implemented in sdbusplus. As an example, Michael Brown provided the following: polkit-test. It demonstrates the connection. And, it appears to be synchronous. I think we've settled on using SDBusPlus because of the YAML support. So, I'm trying to duplicate this using the SDBusPlus framework.

I'll take a look at some of the ideas you provided. I'm still stuck on keeping this synchronous if I can. But, I also understand the pitfalls I'll likely face. Michael and I had already started looking at the message_t you're pointing to and how to expose it. Glad to hear we may be going the right direction. I'll let you know what I find out.

williamspatrick commented 2 years ago

We're looking to integrate polkit into SDBusPlus so that we can provide an Authorization layer to dbus services implemented in sdbusplus.

I'm still slightly curious what polkit gives you over the busconfig ACLs. Especially considering we don't currently have a whole lot of applications running as non-root. Something you're also going to take into account is that external interfaces, such as bmcweb, might be running as one user (root) but the operation is on behalf of some other user (ex. admin). The dbus-message is going to have the daemon's effective UID and not the authenticated requester. I guess that is perhaps something we could work on also.

It does look like systemd itself does do some amount of async polkit-based authentication. You might want to glance at that. I don't see a whole lot of APIs that are readily exposed, such that we could make sdbusplus wrappers around them, but it does appear that they do most of the polkit calls asynchronously.

I did see this little tidbit in the dbus-daemon manpage also (https://dbus.freedesktop.org/doc/dbus-daemon.1.html) :

Some existing system services rely on more complex rules to control the messages that the service can receive. However, the dbus-daemon's policy language is not well-suited to finer-grained policies: any policy has to be expressed in terms of D-Bus interfaces and method names, not in terms of higher-level domain-specific concepts like removable or built-in devices. It is recommended that new services should normally accept method call messages from all callers, then apply a sysadmin-controllable policy to decide whether to obey the requests contained in those method call messages, for example by consulting polkit.

superchalupa commented 2 years ago

The controls that dbus gives you out of the box for access control are only granular down to the method, interface, path, bus, or property level and can use the calling user, group, or local-console-logged-in to differentiate. However, if you want to do access control based on arguments to a method, internal state of a daemon, the time of day or phase of the moon, you cannot.

Example: if you have a daemon (configd) that has the API: GetConfig( key string ), SetConfig( key string, value string) and you have the following 'keys': StaticIP/Gateway/Netmask/DNS (ie. "Network settings") UniqueMachineID ManufacturingMode LicenseServerAddress PrivateKey

And you have the following daemons that use configd: RestWebService - Read/Write Network (IP/GW/NM/DNS), read-only UniqueMachineID, NO ACCESS PrivateKey, LicenseServerAddress, ManufacturingMode LicenseD - read/write LicenseServerAddress, read PrivateKey. WRITE PrivateKey IFF ManufacturingMode=1

Using standard dbus access controls, there is no method to ensure that the RestWebService CAN NEVER read or write the PrivateKey and LicenseServerAddress. Using standard dbus access controls you cannot enforce that licenced can only write the private key if in manufacturing mode.

You can do this with polkit. (and more)

You dont always need polkit, but when you need it, you need it.

superchalupa commented 2 years ago

To implement the above access control scheme, the configd daemon just does this:

sender = sd_bus_message_get_sender() key = key parameter user asked to set. ... allowed = CheckAuthorization( sender, "com.example-SetConfig", {"config-key": key}, flags)

The properties passed in third argument dictionary are completely arbitrary and can be made up to suite whatever a policy engine might need to make a decision.

Next, you can then write a polkit rule that matches the action ID "com.example-SetConfig", and it can pull out all the annotations, in this case 'config-key', and can then make a decision. Polkit rules are written in javascript, and run in secure environment inside polkit. The rules can do any arbitrary tests and return an answer.

superchalupa commented 2 years ago

based on the above check added to the hypothetical 'configd', each daemon can drop in policy rules to grant itself access to just the things it needs.

Then, finally, we can also implement user-level access controls using the same method.