Created attachment 105699 [details] [review] [PATCH] dbus-spec: introduce new PERMIT_INTERACTIVE_AUTHENTICATION
(dbus/doc is a trap for the unwary, Bug #67034; moving to dbus/core)
I'm personally fine with this being a new flag, rather than a new message header or something. I'm not sure whether maintainer consensus agrees with me. There is some discussion on the mailing list on whether this should be PERMIT_INTERACTIVE_AUTHENTICATION (i.e. "you may use PolicyKit or equivalent, I don't mind waiting"), or whether it should be PERMIT_INTERACTIVITY to include other user interactions in its scope (silly but hopefully illustrative example: StartFoo() could optionally use PackageKit to prompt whether to install Foo if it is not installed, rather than just failing). I don't think there's consensus in either direction on that.
Quoting the mailing-list: Thiago Macieira <thiago@kde.org> wrote: > On Thursday 04 September 2014 13:28:19 David Herrmann wrote: > > If interactivity is part of a operation that is done by a method call, > > it is totally acceptable to add a method-argument that selects > > interactivity. This is unlike authorization, which it is part of the > > transmission layer (is the source allowed to perform that operation?), > > not part of the actual operation. Authorization needs to be done for > > *all* method calls (standard dbus policies). This flag extends this by > > optional interactive authentication. I don't see how this has anything > > to do with an interactivity-flag for the actually performed operation. > [...] > > If we make this flag generic, there is no way to disable > > interactive-auth but enable an interactive operation. Maybe you want a > > "save download" dialog, but without any interactive authentication. > > Hi David > > That's a good argument, thank you for saying it. > > I still think we ought to give this a little more thought, but I won't object > anymore to the change in the spec. So this basically resolved the issue around ALLOW_INTERACTIVE_AUTHENTICATION against ALLOW_INTERACTIVITY, right? To summarize it: You may want to allow interactivity of the operation, but not interactive authentication to permit the operation. Those are two different intents that shouldn't be merged. v2 is pending on the list [1]. I haven't heard of any other issues, yet. [1] "[PATCH v2] dbus-spec: introduce new ALLOW_INTERACTIVE_AUTHENTICATION flag"
Created attachment 106785 [details] [review] [PATCH v2] dbus-spec: introduce new ALLOW_INTERACTIVE_AUTHENTICATION flag From: Lennart Poettering As discussed earlier on the dbus mailing list, let's add a new flag to the message header, that allows clients to indicate whether they are OK with possiably time-intensive interactive authentication. This is useful for authentication frameworks such as polkit, but this flag is supposed to be generic, and not be bound to any implementation of such a framework. The dbus specification already clarifies that unknown flags must be ignored, the reference implementation and the other implementations i checked indeed ignore any new flags, hence we should be fine with compatibility here. This patch simply updates the spec, it does not add new APIs for this to the reference implementation. This is version 2: - rename from PERMIT to ALLOW (as suggested by Marcel) - define a standardized error to return if interactive authentication is required (as suggested by Ted) --- Attaching Lennart's latest patch here so splinter can act on it.
Comment on attachment 106785 [details] [review] [PATCH v2] dbus-spec: introduce new ALLOW_INTERACTIVE_AUTHENTICATION flag Review of attachment 106785 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +1351,5 @@ > for the destination name in response to this message. > </entry> > </row> > + <row> > + <entry><literal>ALLOW_INTERACTIVE_AUTHENTICATION</literal></entry> I think the thing you're doing should actually be labelled interactive authorization, not interactive authentication, throughout. The distinction is significant. Authentication is "you claim to be Lennart, but are you really?", or alternatively "who are you, and can you prove it?", whereas authorization is "are you allowed to reconfigure the network?" (or whatever) - which might include confirming that you are really Lennart, or just answering yes to an "are you sure?" popup. @@ +1357,5 @@ > + <entry>This is a hint that may be set on a method call > + message that informs the receiving side that the > + caller is OK if possibly time-intensive interactive > + user authentication may take place before the method > + call will complete. A client may set this flag if it (I'd like a few <para></para> in here, maybe a paragraph break before "This flag is only useful" - it's pretty dense right now.) Perhaps something more like this wording: <entry> <para> This flag may be set on a method call message to inform the receiving side that the caller is prepared to wait for interactive authorization, which might take a considerable time to complete. For instance, if this flag is set, it would be appropriate to query the user for passwords or confirmation via Polkit or a similar framework. </para> <para> This flag is only useful (... what you said) @@ +1362,5 @@ > + is prepared to wait for a longer time before the > + method call returns, and if its UI may be interrupted > + by interactively querying the user for passwords or > + confirmation. This flag is only useful when > + unprivileged code calls a more priviliged method call, spelling: "more privileged method" @@ +1375,5 @@ > + ignored otherwise. If a this flag is not set on a > + method call, and a service determines that the > + requested operation is not allowed without interactive > + authentication, but could be after successful > + interactive authentication it should return the I'd say "may return". Existing services from before this flag was added might be performing interactive PolicyKit etc. authentication anyway, and we're not going to change that by writing about it in the spec: declaring existing correct code to be retroactively buggy is not the done thing. Perhaps a final paragraph like this, to point that out? <para> The absence of this flag does not guarantee that interactive authorization will not be applied, since existing services that pre-date this flag might already use interactive authorization. However, existing D-Bus APIs that will use interactive authorization should document that the call may take longer than usual, and new D-Bus APIs should avoid interactive authorization in the absence of this flag. </para> @@ +1376,5 @@ > + method call, and a service determines that the > + requested operation is not allowed without interactive > + authentication, but could be after successful > + interactive authentication it should return the > + <literal>org.freedesktop.DBus.Error.InteractiveAuthenticationRequired</literal> In the absence of a list of recommended error codes in the spec, I'd like this commit to add this error to dbus-protocol.h, which is the de facto reference list of errors. Adding dbus_message_{get,set}_allow_interactive_authorization() also seems somewhat important, since libdbus doesn't have an API for setting arbitrary numeric flags.
(In reply to comment #5) > For instance, if this flag is > set, it would be appropriate to query the user for passwords or confirmation > via Polkit or a similar framework. I'm not sure that either I or Lennart expressed this very well in our proposed text, and I can't really think of how to say it more clearly, but: Suppose I'm doing something that prompts for passwords for a reason other than determining whether the user is authorized to perform the requested action. Is that in the scope of ALLOW_INTERACTIVE_AUTHORIZATION? My understanding is that Lennart's intention is "no, only things like Polkit are in-scope". Example 1: I connect my XMPP account using Telepathy on the session bus. The session bus is not a privilege boundary, so no authorization step takes place. However, when the Telepathy XMPP backend connects to the server, it needs my password (assume that I haven't saved it in gnome-keyring or whatever) in order to authenticate to the server. My understanding is that in example 1, ALLOW_INTERACTIVE_AUTHORIZATION would have no effect because this is not a privilege boundary, and the password request from the server (proxied through Telepathy and D-Bus to the UI) is outside the scope of ALLOW_INTERACTIVE_AUTHORIZATION because it's part of the method's actual functionality, not the D-Bus mechanisms used to transport it. Example 2: I connect my laptop to a WLAN. The system bus is a privilege boundary, so NetworkManager may need to carry out an authorization step via Polkit to determine that I am allowed to change system-wide network settings. Additionally, when NM connects to the WLAN, it needs the WLAN's WPA pre-shared key, or my Radius credentials for WPA Enterprise. Again, assume I haven't stored the WPA key/credentials already. My understanding is that in example 2, ALLOW_INTERACTIVE_AUTHORIZATION would determine whether NM tells Polkit that it may use interactive (e.g. password) authentication; but it would have no effect on whether NM uses interactive prompting for the WPA credentials, because those are part of the method's actual functionality, not the D-Bus mechanisms used to transport it.
(In reply to comment #6) > I'm not sure that either I or Lennart expressed this very well in our > proposed text, and I can't really think of how to say it more clearly, but: > My understanding is that in example 1, ALLOW_INTERACTIVE_AUTHORIZATION would > have no effect because this is not a privilege boundary, and the password > request from the server (proxied through Telepathy and D-Bus to the UI) is > outside the scope of ALLOW_INTERACTIVE_AUTHORIZATION because it's part of > the method's actual functionality, not the D-Bus mechanisms used to > transport it. > My understanding is that in example 2, ALLOW_INTERACTIVE_AUTHORIZATION would > determine whether NM tells Polkit that it may use interactive (e.g. > password) authentication; but it would have no effect on whether NM uses > interactive prompting for the WPA credentials, because those are part of the > method's actual functionality, not the D-Bus mechanisms used to transport it. I fully agree. The flag should affect the transport-layer, not the application-layer. A service/interface provider is free to include external information to grant authorization ("Is the target's session active?", "am I running in a container?", ..), but it should always limit the authorization to the method that is invoked, not to the functionality provided.
(In reply to comment #5) > @@ +1351,5 @@ > > for the destination name in response to this message. > > </entry> > > </row> > > + <row> > > + <entry><literal>ALLOW_INTERACTIVE_AUTHENTICATION</literal></entry> > > I think the thing you're doing should actually be labelled interactive > authorization, not interactive authentication, throughout. The distinction > is significant. > > Authentication is "you claim to be Lennart, but are you really?", or > alternatively "who are you, and can you prove it?", whereas authorization is > "are you allowed to reconfigure the network?" (or whatever) - which might > include confirming that you are really Lennart, or just answering yes to an > "are you sure?" popup. I think we want both, "Are you sure?"-dialogs *AND* "Please authorize as an administrator". The former is authorization, the latter is authentication. As I understand it, interactive authentication is used as fallback to authorize the action, so your suggestion sounds fine. INTERACTIVE_AUTHORIZATION can imply INTERACTIVE_AUTHENTICATION, but not the other way around, I think. But I have no strong opinion on the wording, anyway..
(In reply to comment #8) > I think we want both, "Are you sure?"-dialogs *AND* "Please authorize as an > administrator". Yes. > The former is authorization, the latter is authentication. No, I think the former is authorization, and the latter is both. The primary purpose in both cases is to authorize an action. The latter is *additionally* performing authentication, in order to be sure that the person at the keyboard is one of the people who is allowed to authorize the action.
Created attachment 106852 [details] [review] [PATCH 1/2] dbus-spec, dbus-protocol: add ALLOW_INTERACTIVE_AUTHORIZATION flag Heavily based on a patch from Lennart Poettering. This is useful for authentication frameworks such as polkit, but this flag is supposed to be generic, and not be bound to any implementation of such a framework. The dbus specification already clarifies that unknown flags must be ignored, the reference implementation and the other implementations we checked indeed ignore any new flags, hence we should be fine with compatibility here. --- Here is a version of Lennart's patch that I'd be happy to apply. I used an example involving a VPN to clarify the answer to the questions I asked in Comment #6.
Created attachment 106853 [details] [review] [PATCH 2/2] Implement getter, setter for ALLOW_INTERACTIVE_AUTHORIZATION flag
I agree that this is "authorization". We already know who the user is ... we are (re)authorizing them to make sure they're still present. The easiest way to do this is to make them reauthenticate (type their password again), but this is a side-effect of what we're actually trying to do: authorization. As an example, some polkit agents reauthorize the user without reauthenticating them in the traditional way, such as Cockpit.
Comment on attachment 106852 [details] [review] [PATCH 1/2] dbus-spec, dbus-protocol: add ALLOW_INTERACTIVE_AUTHORIZATION flag Review of attachment 106852 [details] [review]: ----------------------------------------------------------------- Looks great!
Comment on attachment 106853 [details] [review] [PATCH 2/2] Implement getter, setter for ALLOW_INTERACTIVE_AUTHORIZATION flag Review of attachment 106853 [details] [review]: ----------------------------------------------------------------- Looks great, too!
I have also implemented this in sd-bus now (though this API isn't public yet).
I took the liberty and commited Simon's spec patch now. I am about to roll a new systemd release with support for the new flag, and I wanted to make sure the spec is updated accordingly before I do so. As everybody appeared to agree on the spec change now I figured I could just go ahead and commit it. Thanks for reviewing and updating the patch, Simon, thanks! The other patch looks great to me too, but there was no immediate need for me to push it, hence I left it as is. Would be great to merge this soon too, though.
I merged Attachment #106853 [details] too.
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.