|
Description
Simon McVittie
2017-12-12 13:43:12 UTC
Sorry, I'm getting a 500 Internal Server Error when I try to attach patches, so for now please see https://github.com/smcv/dbus/commits/spec-sasl-104224 commits newer than "_dbus_test_oom_handling: print TAP diagnostics". Created attachment 136109 [details] [review] 1/9] spec: Explicitly say that auth client and server take turns This was (hopefully) implicit in the protocol descriptions, but we never actually said it. Do so. Created attachment 136110 [details] [review] 2/9] spec: Move text about the BEGIN command to documentation of BEGIN Having the text about the message stream in the documentation of AUTH seemed rather odd, and made it likely to get out of sync with the rest of the spec. Move it to the BEGIN section, remove some duplication, and make it clearer that if the client pipelines the fd-negotiation, the server is expected to send exactly one reply per non-BEGIN command before switching to the D-Bus wire protocol. Created attachment 136111 [details] [review] 3/9] spec: Document the direction of each auth command Created attachment 136112 [details] [review] 4/9] spec: Document expected reply for each client-to-server auth command Client-to-server auth commands expect a reply, whereas server-to-client auth commands don't (the client is expected to send another command that is valid in the new state, but it isn't really a direct reply to the server-to-client command). Created attachment 136113 [details] [review] 5/9] spec: Document NEGOTIATE_UNIX_FD, AGREE_UNIX_FD in state machines --- We should have done this when Lennart added fd-passing in 2009, but we didn't. Created attachment 136114 [details] [review] 6/9] spec: ERROR takes an optional explanation in both directions The examples don't include an explanation, but the reference implementation always sends the human-readable explanation, in both directions. Created attachment 136115 [details] [review] [7/9] spec: Define what non-empty authorization identity strings mean The SASL RFC requires that we do this. I had previously thought that the D-Bus protocol on Unix requires the use of numeric user IDs, but in fact the reference implementation will also accept usernames. Created attachment 136116 [details] [review] 8/9] spec: Make example authentication transactions more realistic We don't need to invent a MAGIC_COOKIE mechanism when we have a perfectly good EXTERNAL. --- In particular the ones I've commented are a typical Unix uid and a Windows SID. For those paying particularly careful attention to the examples, I also made sure to include bare "AUTH EXTERNAL" (which means "whatever identity I appear to have, please authenticate me as that") and "AUTH EXTERNAL 736d6376" (the login name "smcv", which means the same thing as the corresponding numeric uid). These are probably less interoperable, since they would previously only have been discovered by reading the source code: libdbus clients will never attempt them, so they won't be found by reverse-engineering. GDBus only supports the numeric uid form. Created attachment 136117 [details] [review] [9/9] spec: Describe the EXTERNAL and ANONYMOUS auth mechanisms These are defined by standard RFCs rather than by D-Bus. What separates them from other standard mechanisms like PLAIN (RFC 4616) is that in practice, D-Bus implementations support EXTERNAL, DBUS_COOKIE_SHA1 and sometimes ANONYMOUS, but not PLAIN. Comment on attachment 136115 [details] [review] [7/9] spec: Define what non-empty authorization identity strings mean Review of attachment 136115 [details] [review]: ----------------------------------------------------------------- Is there a reason to make the spec mandate this? I would prefer if the spec said "uids" and that's it. The mere fact that dbus-daemon allows more should not require us to mandate this. I am not aware of any implementation that uses this feature, and limiting this to UIDs makes the implementation a lot simpler. (In reply to David Herrmann from comment #11) > Is there a reason to make the spec mandate this? I would prefer if the spec > said "uids" and that's it. The reference implementation accepts non-numeric authorization identities, so I feel as though we should write down what that means so that there is some hope of interoperability. The balancing act I was trying to achieve was: servers are not required to accept login names, but if a server on Unix does accept non-numeric strings, then they must be interpreted as login names (rather than some unpredictable implementation-specific meaning). Suggestions for better wording for that welcome. It's a pity other implementations have cargo-culted the use of a numeric uid as authorization identity and assumed it's required, because if all servers were willing to accept plain "AUTH EXTERNAL\r\n" with no explicit authorization identity, then we could recommend that clients should use it, and it would be one less thing that could go wrong (particularly if crossing a uid namespace boundary). libdbus (dbus-daemon) and sd-bus (dbus-broker?) seem to be OK with this, but unfortunately the libraries behind the other message bus implementations I know about (GDBus' gdbus-daemon and dbus-sharp's "managed dbus-daemon") both seem to require an explicit authorization identity at the moment. Would this be better? [...] represents a numeric user ID as defined by POSIX, for example <literal>0</literal> for the root user or <literal>1000</literal> for the first user created on many systems. Non-numeric authorization identities are not required to be accepted or supported, but if used, they must be interpreted as a login name as found in the <literal>pw_name</literal> field of POSIX <literal>struct passwd</literal> [...] Comment on attachment 136109 [details] [review] 1/9] spec: Explicitly say that auth client and server take turns Review of attachment 136109 [details] [review]: ----------------------------------------------------------------- Sure. Comment on attachment 136110 [details] [review] 2/9] spec: Move text about the BEGIN command to documentation of BEGIN Review of attachment 136110 [details] [review]: ----------------------------------------------------------------- OK. Comment on attachment 136111 [details] [review] 3/9] spec: Document the direction of each auth command Review of attachment 136111 [details] [review]: ----------------------------------------------------------------- Yup. Comment on attachment 136112 [details] [review] 4/9] spec: Document expected reply for each client-to-server auth command Review of attachment 136112 [details] [review]: ----------------------------------------------------------------- Yup. Comment on attachment 136113 [details] [review] 5/9] spec: Document NEGOTIATE_UNIX_FD, AGREE_UNIX_FD in state machines Review of attachment 136113 [details] [review]: ----------------------------------------------------------------- Minor nitpicks. ::: doc/dbus-specification.xml @@ +2803,5 @@ > + <itemizedlist> > + <listitem> > + <para> > + Receive AGREE_UNIX_FD → enable Unix fd passing, > + send BEGIN, terminate auth conversation Maybe clarify that this means the client is authenticated? ‘terminate auth conversation’ sounds like it’s being terminated in an error state. WaitingForBegin, for example, uses “terminate auth conversation, client authenticated”. @@ +2809,5 @@ > + </listitem> > + > + <listitem> > + <para> > + Receive ERROR → disable Unix fd passing, In one of the subsequent patches (the one to make the examples more realistic), you’ve used ‘Unix FD’ rather than ‘Unix fd’. Probably best to be consistent. Probably also best to fix that across the whole document in a follow-up commit. Comment on attachment 136114 [details] [review] 6/9] spec: ERROR takes an optional explanation in both directions Review of attachment 136114 [details] [review]: ----------------------------------------------------------------- Yup. Comment on attachment 136115 [details] [review] [7/9] spec: Define what non-empty authorization identity strings mean Review of attachment 136115 [details] [review]: ----------------------------------------------------------------- > limiting this to UIDs makes the implementation a lot simpler. Really? For the sake of one username lookup? Changing this would mean dropping full SASL support. I don’t think it’s massively necessary for us to have full SASL support (it’s not like we’re interoperating with much else, and being able to use a SASL library for authentication doesn’t seem to have massively aided D-Bus client library development), but it would be a backwards-incompatible change. Comment on attachment 136116 [details] [review] 8/9] spec: Make example authentication transactions more realistic Review of attachment 136116 [details] [review]: ----------------------------------------------------------------- r+ but with a question. ::: doc/dbus-specification.xml @@ +2620,3 @@ > S: OK 1234deadbeef > C: NEGOTIATE_UNIX_FD > S: ERROR Maybe whack a human-readable error message on the ERROR command? Comment on attachment 136117 [details] [review] [9/9] spec: Describe the EXTERNAL and ANONYMOUS auth mechanisms Review of attachment 136117 [details] [review]: ----------------------------------------------------------------- Yup. Mostly r+, but with a couple of questions/comments which should be answered before it’s entirely r+. (In reply to Simon McVittie from comment #13) > Would this be better? > > [...] represents a numeric user ID as defined by POSIX, for example > <literal>0</literal> for the root user or <literal>1000</literal> > for the first user created on many systems. Non-numeric authorization > identities are not required to be accepted or supported, but if used, > they must be interpreted as a login name as found in the > <literal>pw_name</literal> field of POSIX > <literal>struct passwd</literal> [...] I like this wording. Thanks! (In reply to Philip Withnall from comment #20) > Comment on attachment 136115 [details] [review] [review] > [7/9] spec: Define what non-empty authorization identity strings mean > > Review of attachment 136115 [details] [review] [review]: > ----------------------------------------------------------------- > > > limiting this to UIDs makes the implementation a lot simpler. > > Really? For the sake of one username lookup? Yes. Username lookups might end up in NSS, which might be backed by NSS-modules that use D-Bus. This has long been suspected to be the source of deadlocks in RHEL (and the deadlocks are easy to reproduce). Note that NSS lookups are usually done in a blocking manner, since glibc does not offer alternative APIs. Hence, making this asynchronous is harder than it looks, and also will not solve the problem of nss-modules recursing here. > Changing this would mean dropping full SASL support. I don’t think it’s > massively necessary for us to have full SASL support (it’s not like we’re > interoperating with much else, and being able to use a SASL library for > authentication doesn’t seem to have massively aided D-Bus client library > development), but it would be a backwards-incompatible change. I am not saying to change dbus-daemon. All I am saying is to not mandate it in the spec (as such "dropping" that part from the proposed patch). I agree with Simon that the preferred solution is for everyone to stop sending UIDs and make the server pick it (by omitting the identity argument). (In reply to David Herrmann from comment #25) > Username lookups might end up in NSS, which might be backed by > NSS-modules that use D-Bus. You can't have the semantics of the dbus-daemon without using NSS in any case: the abstract message bus protocol as documented in the spec doesn't need NSS, but practical Linux systems also rely on the access control/configuration language documented in dbus-daemon(1), and that needs to understand user and group names. Even if we can eventually stop using <allow> and <deny> for the firewall-style message sending and receiving permissions, I expect we'll need <policy user="_some-system-user"><allow own="com.example.Name"/></policy> indefinitely. If I understand correctly, you got around that in dbus-broker by having a separate process that reads dbus-daemon(1) configuration, turns it into a purely uid-based language, and injects that into dbus-broker, before normal D-Bus services have the opportunity to try to own names; and until that has happened, only uid 0 can use the dbus-broker, but that's hopefully enough to bootstrap NSS name resolution? Created attachment 136277 [details] [review] [5/9 v2] spec: Document NEGOTIATE_UNIX_FD, AGREE_UNIX_FD in state machines --- v2: - Previously the choice of BEGIN vs. NEGOTIATE_UNIX_FD was only offered in WaitingForOK state, but in fact it can also happen in WaitingForData state (and real D-Bus clients never reach WaitingForOK, because we don't implement any mechanisms where the client knows that it has been accepted before the server says so). - Attempt to clarify that when the client sends BEGIN, it is terminating the auth conversation successfully. I deliberately didn't say 'authenticated' here in the WaitingForAgreeUnixFD case, because conceptually the client is already successfully authenticated *before* it sends NEGOTIATE_UNIX_FD. Created attachment 136278 [details] [review] [7/9 v2] spec: Define what non-empty authorization identity strings mean --- v2: Make it clearer that servers are not required to accept usernames. Created attachment 136279 [details] [review] [8/9 v2] spec: Make example authentication transactions more realistic --- v2: Put a human-readable error message in one of the two uses of ERROR. Leave the response to FOOBAR as just a bare ERROR, to emphasize that both are possible. Created attachment 136280 [details] [review] [really 5/9 v2] spec: Document NEGOTIATE_UNIX_FD, AGREE_UNIX_FD in state machines --- It helps to attach the patch from the right ${srcdir}. Created attachment 136281 [details] [review] [really 7/9 v2] spec: Define what non-empty authorization identity strings mean Created attachment 136282 [details] [review] [really 8/9 v2] spec: Make example authentication transactions more realistic (In reply to Simon McVittie from comment #26) > (In reply to David Herrmann from comment #25) > > Username lookups might end up in NSS, which might be backed by > > NSS-modules that use D-Bus. > > You can't have the semantics of the dbus-daemon without using NSS in any > case: the abstract message bus protocol as documented in the spec doesn't > need NSS, but practical Linux systems also rely on the access > control/configuration language documented in dbus-daemon(1), and that needs > to understand user and group names. Even if we can eventually stop using > <allow> and <deny> for the firewall-style message sending and receiving > permissions, I expect we'll need <policy user="_some-system-user"><allow > own="com.example.Name"/></policy> indefinitely. That is true, but it is worth noting that NSS for loading configuration is quite different from NSS to connect. Both can in principle be problematic, but the former is unavoidable, though under admin control, the latter is avoidable and much harder to control/reason about. > If I understand correctly, you got around that in dbus-broker by having a > separate process that reads dbus-daemon(1) configuration, turns it into a > purely uid-based language, and injects that into dbus-broker, before normal > D-Bus services have the opportunity to try to own names; and until that has > happened, only uid 0 can use the dbus-broker, but that's hopefully enough to > bootstrap NSS name resolution? That is the basic idea. The way it works is: First: * The listening socket is created (or passed in from PID1). * The policy is read from disk and uid/gid's resolved via NSS At this point the dbus-broker is not operational as it is not yet listening to the socket. As such, no dbus-based NSS resolution is possible. This is ok because we assume any user/group names used in the configuration files are given statically in /etc/passwd and friends, rather than resolved over something like LDAP (local policy referencing remote users sounds very strange). This is not at all obvious, and it is probably something we should document better. I'd even propose to add this to the spec if we all agreed. Then: * The listening socket is passed, together with the uid/gid-based policy from the launcher to the broker. Now the broker is fully functional and no more blocking operations can occur. A similar logic applies when configuration is reloaded, only then the launcher does this (and re-resolves user/group names) asynchronously while the broker keeps dispatching messages. It is worth noting the problem with resolving the connecting user is that this is likely to be resolved via some DBus-using NSS module (if they are used at all), unlike the user names from the policy files, which are much more constrained and predictable. Moreover, a deadlock could obviously be triggered by a malicious user whenever a susceptible NSS module is installed: pass in an invalid user name and you are guaranteed to trigger all the NSS modules, blocking the whole bus until they (hopefully) time out. Comment on attachment 136280 [details] [review] [really 5/9 v2] spec: Document NEGOTIATE_UNIX_FD, AGREE_UNIX_FD in state machines Review of attachment 136280 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 136281 [details] [review] [really 7/9 v2] spec: Define what non-empty authorization identity strings mean Review of attachment 136281 [details] [review]: ----------------------------------------------------------------- r+ (see also David’s r+ in commnt #24) Comment on attachment 136282 [details] [review] [really 8/9 v2] spec: Make example authentication transactions more realistic Review of attachment 136282 [details] [review]: ----------------------------------------------------------------- r+ (In reply to Tom Gundersen from comment #33) > As such, no dbus-based NSS resolution is possible. This is ok > because we assume any user/group names used in the configuration files are > given statically in /etc/passwd and friends, rather than resolved over > something like LDAP (local policy referencing remote users sounds very > strange). This is not at all obvious, and it is probably something we should > document better. I'd even propose to add this to the spec if we all agreed. dbus-daemon's XML configuration language is not (currently) in the scope of the spec, but I'd welcome patches to dbus-daemon(1) that said this. (Long term, I'd also love to see a less horrible syntax for expressing at least the simplest cases of the best-practice policy for system services, namely "system user $foo may own name $bar, (user $foo|any user) may send method calls to connections that own or are in the queue for $bar".) > Moreover, a deadlock could obviously be triggered by a malicious user > whenever a susceptible NSS module is installed: pass in an invalid user name > and you are guaranteed to trigger all the NSS modules, blocking the whole > bus until they (hopefully) time out. Ouch. Our position on this at the moment is effectively "don't write or install those NSS modules", as far as I understand it; but making dbus-daemon (at least optionally) refuse to resolve usernames during the SASL handshake sounds like a reasonable change to make anyway. (In reply to Simon McVittie from comment #37) > (In reply to Tom Gundersen from comment #33) > > As such, no dbus-based NSS resolution is possible. This is ok > > because we assume any user/group names used in the configuration files are > > given statically in /etc/passwd and friends, rather than resolved over > > something like LDAP (local policy referencing remote users sounds very > > strange). This is not at all obvious, and it is probably something we should > > document better. I'd even propose to add this to the spec if we all agreed. > > dbus-daemon's XML configuration language is not (currently) in the scope of > the spec, but I'd welcome patches to dbus-daemon(1) that said this. ⇒ Bug #104586 > > Moreover, a deadlock could obviously be triggered by a malicious user > > whenever a susceptible NSS module is installed: pass in an invalid user name > > and you are guaranteed to trigger all the NSS modules, blocking the whole > > bus until they (hopefully) time out. > > Ouch. Our position on this at the moment is effectively "don't write or > install those NSS modules", as far as I understand it; but making > dbus-daemon (at least optionally) refuse to resolve usernames during the > SASL handshake sounds like a reasonable change to make anyway. ⇒ Bug #104588 (In reply to Philip Withnall from comment #38) > (In reply to Simon McVittie from comment #37) > > dbus-daemon's XML configuration language is not (currently) in the scope of > > the spec, but I'd welcome patches to dbus-daemon(1) that said this. > > ⇒ Bug #104586 > > > making > > dbus-daemon (at least optionally) refuse to resolve usernames during the > > SASL handshake sounds like a reasonable change to make anyway. > > ⇒ Bug #104588 I tried to say the same, but Bugzilla became unresponsive (down for maintenance?) before I could. Other than the parts that have been kicked out into those bugs, this is fixed in 1.13.0. (I might actually copy the spec from 1.13.0 into dbus-1.12, because I think we're overdue for a spec release, but I'm not sure I want to be releasing 1.13.0 right now.) |
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.