|
Description
Matt Fischer
2013-02-05 22:36:13 UTC
(In reply to comment #0) > DBus has support for LOCAL_CREDS-based credential passing Which operating system(s) use(s) LOCAL_CREDS? Which operating system(s) have you tested this patch on? While I'm usually in favour of the Autoconf "test for features, not platforms" approach to portability, credentials-passing is sufficiently strange and platform-specific that we would benefit from having comments that indicate which platforms ought to be following which code path... Given that I'm not aware of any pair of kernels that implement identical credentials-passing mechanisms, I'm almost tempted to say we should do what GLib does, and do this entirely per-platform instead of per-feature. (See also https://bugzilla.gnome.org/show_bug.cgi?id=649302 for similar issues in GLib.) (In reply to comment #0) > DBus has support for LOCAL_CREDS-based credential passing (i.e. struct > sockcred) To me it looks as though we were always trying to support a version of LOCAL_CREDS that has struct cmsgcred, and you're adding support for a version of LOCAL_CREDS that has struct sockcred instead? From commit messages, it appears that the version we support may have been intended for NetBSD: commit e7563d502bc1400548f99dd339b5b0873eda0370 Author: John (J5) Palmieri <johnp@redhat.com> Date: 2006-09-14 05:07:11 +0000 * dbus/dbus-sysdeps.c: Add support for LOCAL_CREDS socket credentials. Fixes "external" authentication under e.g. NetBSD which does not support any other socket credentials mechanism. (Patch from Julio M. Merino Vidal <jmmv at NetBSD dot org>) (J5 no longer works on D-Bus.) Of course, it's also entirely possible that subsequent refactoring has never actually been tested on NetBSD. This is why I want this stuff to be better-documented :-( Now that I look at NetBSD's dbus package, I've discovered that they've implemented yet another credential-checking scheme using LOCAL_PEEREID, in an unsubmitted patch titled "_dbus_poll: Set the timeout value argument to poll to -1 whenever it is less than -1 to avoid kde4 session start hang". Nice... Can you point me to some somewhat canonical documentation on LOCAL_CREDS on the OS where it originated, or (if different) the OS where you're using it, or preferably both? This is for QNX. I've been working on getting DBus running on it--it basically works just fine, except for a few little build issues like this, and I know that others have had it running there in the past, so my guess is that this is just something that got broken during a refactor. I don't have any particular experience in the lore of socket credential passing--I've just been trying to fix up the build failures that I run across--but from what I've been able to glean from manpages, this method of using struct sockcred in conjunction with LOCAL_CREDS is a reasonably standard API that comes down from BSD. The official QNX documentation on the subject (http://www.qnx.com/developers/docs/6.5.0/index.jsp?topic=%2Fcom.qnx.doc.neutrino_lib_ref%2Fu%2Funix_proto.html) seems to jive with the FreeBSD manpages I've found (e.g. http://manpages.ubuntu.com/manpages/precise/man4/unix.4freebsd.html). In both cases, the only structure that is mentioned is struct sockcred, so I don't know if there is such a thing as "a version of LOCAL_CREDS that has struct cmsgcred". The two appear to be mutually exclusive. The only thing this patch changes is the way that a couple size calculations are done--the logic for actually using struct sockcred was already there, and worked correctly in my tests. I can easily believe that different kernels have their own quirks related to credential passing, so maybe it will ultimately be necessary to make this more platform-specific. But as far as I can tell, this code which was written for NetBSD still works fine here on QNX once this issue is dealt with, so unless there are some other environments which make use of LOCAL_CREDS that aren't represented here, I think it ought to be possible to support all of these cases without resorting to calling out specific OS's. Comment on attachment 74261 [details] [review] Fixed LOCAL_CREDS support in Unix backend Review of attachment 74261 [details] [review]: ----------------------------------------------------------------- Looks OK, except: ::: dbus/dbus-sysdeps-unix.c @@ +1668,4 @@ > dbus_uid_t uid_read; > dbus_pid_t pid_read; > int bytes_read; > + int controllen; This will provoke "unused variable" warnings unless either HAVE_CMSGCRED or LOCAL_CREDS is defined. Created attachment 74677 [details] [review] Unix credentials-passing: add comments explaining how we reach each case --- Having done this research, I think we should consider changing the order of precedence from: * SO_PEERCRED (Linux, OpenBSD) * struct cmsgcred, SCM_RIGHTS (FreeBSD) * LOCAL_CREDS (various) * getpeereid (various) * getpeerucred (Solaris) to: * SO_PEERCRED (Linux, OpenBSD) * struct cmsgcred (FreeBSD) * getpeerucred (Solaris) * getpeereid (various) * LOCAL_CREDS (various) Rationale for this ordering, in descending order of priority: SO_PEERCRED and struct cmsgcred tell us the process ID. SO_PEERCRED, getpeerucred, getpeereid just ask the kernel who the peer is, and don't require prior action from either us or the peer. getpeerucred runs additional Solaris-specific ADT logic which we presumably want to happen. getpeereid is type-safe. Created attachment 74679 [details] [review] Unix credentials-passing: emit a #warning if we can't This will break the build in developer mode (but not in release mode) on platforms where credentials passing doesn't work. That's positive, as far as I'm concerned: it'll give developers on obscure platforms some incentive to test and fix it. If we find a Unix OS that really doesn't support credentials-passing, we can make this #ifndef __THAT_BAD_UNIX__ or something. Created attachment 74680 [details] [review] Unix credentials-passing: emit a #error if we can't, on known-good platforms We know that D-Bus works well, with credentials-passing and GDBus interoperability, on Linux, FreeBSD and OpenBSD. If someone builds libdbus on these platforms and it ends up without credentials-passing support, then that build is broken and uninteroperable, and we should consider it a bug. (In reply to comment #6) > Unix credentials-passing: add comments explaining how we reach each case Apparently Cygwin (which emulates Unix sockets using IPv4!) also has getpeereid. Created attachment 74698 [details] [review] Fixed LOCAL_CREDS support in Unix backend Updated my patch to fix the unused variable issue. It's a little gross to duplicate the declaration, but it seemed cleaner than adding a new #ifdef clause, and the union declaration right below it was already duplicated anyway. Created attachment 75145 [details] [review] Fixed LOCAL_CREDS support in Unix backend Whoops, apparently I misread the documentation on how cmsghdr sizes work. This version of the patch does it correctly. (In reply to comment #11) > Fixed LOCAL_CREDS support in Unix backend > > Whoops, apparently I misread the documentation on how cmsghdr sizes work. > This version of the patch does it correctly. This looks OK, but I'm not going to apply it until I've had time to get a NetBSD virtual machine working well enough to compile D-Bus, so we can tell whether it broke that... Colin, Thiago, you both seem to know about esoteric Unix things. Any thoughts on this patch, or the patches I attached here? Sorry to bother you again, but are there any other thoughts on this patch? I know it's a pain to test stuff on these oddball platforms, but if there aren't any other objections, it'd be nice to get this in. Let me know if there's anything I can do to help out, and I'll be happy to oblige. (In reply to comment #13) > Sorry to bother you again, but are there any other thoughts on this patch? Sorry, I haven't had time to bring up a NetBSD virtual machine, get it working well enough to compile D-Bus, and verify that the patch doesn't make NetBSD regress. I would also appreciate opinions from the other D-Bus maintainers on your patch, and on my patches on this bug. (I now realise that Attachment #74677 [details] includes both Attachment #74679 [details] and Attachment #74680 [details]; it shouldn't.) (In reply to comment #11) > Created attachment 75145 [details] [review] > Fixed LOCAL_CREDS support in Unix backend > > Whoops, apparently I misread the documentation on how cmsghdr sizes work. > This version of the patch does it correctly. After fighting with the NetBSD 6 installer for a while, I tested this on NetBSD. It doesn't work, probably for the reasons described in <http://julipedia.meroh.net/2006/08/localcreds-socket-credentials.html>. The version of dbus in NetBSD pkgsrc has a patch that uses the LOCAL_PEEREID sysctl instead - it's basically Linux/OpenBSD SO_PEERCRED with a different syntax. Looks as though the patch that broke the LOCAL_CREDS case was 7bf132c7d1, for Bug #19432. Some bug archaeology led me to Bug #3931: > In fact, I have not > found an implementation of LOCAL_CREDS that does allow one to obtain the pid > (struct sockcred does not contain a pid member in NetBSD, FreeBSD, QNX, or > OpenBSD). So here is my plan. Delete LOCAL_CREDS support: - Linux and OpenBSD have SO_PEERCRED, so they don't need it - FreeBSD and Dragonfly BSD have SCM_CREDS, so they don't need it - NetBSD and QNX have getpeereid(), so they don't need it - Solaris has getpeerucred(), so it doesn't need it - Hurd is probably going to follow FreeBSD - anything even more obscure than Hurd gets to keep both pieces Created attachment 85786 [details] [review] 1/5] Remove BSD-style LOCAL_CREDS support It appears that this regressed back in 2009 (commit 7bf132c7) and doesn't compile. Also, with patches from Matt Fischer to make it compile again on QNX, it compiles but doesn't actually work on NetBSD, which was the platform for which this code was added. This might be for the reasons described in <http://julipedia.meroh.net/2006/08/localcreds-socket-credentials.html>. NetBSD pkgsrc has a large unsubmitted patch to use LOCAL_PEEREID, which is analogous to Linux/OpenBSD SO_PEERCRED. So, I think we can safely assume that nobody is relying on this: either they implement one of our many other supported credentials-passing mechanisms, or they're patching it locally anyway. LOCAL_CREDS is not actually very good - it's awkward to use, and doesn't provide the pid, only the uid. Of the platforms known to implement it, QNX and NetBSD both have getpeereid() which provides just as much information, while FreeBSD and Dragonfly BSD both have SCM_CREDS which provides the pid too. So, let's just get rid of it. --- dbus-test passes on NetBSD, when hacked to not assert that it didn't leak memory or file descriptors (both of which are bad, but orthogonal to this bug) and the next patch has also been applied. Created attachment 85787 [details] [review] [2/6] bus-test: only expect GetConnectionUnixProcessID to succeed sometimes On platforms that use getpeereid(), this can't work. --- ... so, yeah, the regression tests could never have passed on NetBSD. Great. Created attachment 85788 [details] [review] [3/6] _dbus_read_credentials_socket: document where we use each mechanism Created attachment 85789 [details] [review] [4/6] Prefer getpeerucred() over getpeereid() if a platform has both We want the process ID, and getpeerucred() provides that. Created attachment 85790 [details] [review] [5/6] _dbus_read_credentials_socket: warn or fail at compile time if no support On a whitelist of OSs known to have working credentials-passing (currently FreeBSD, Linux, OpenBSD and NetBSD), it would be a regression for us to not have credentials-passing, so fail hard. On other OSs, raise a warning, which is not normally fatal but will alert developers on those platforms. Created attachment 85791 [details]
[doesn't work] Implement NetBSD credentials-passing with LOCAL_PEEREID
I hoped this would let NetBSD get process IDs, but it turns out it still raises org.freedesktop.DBus.Error.UnixProcessIdUnknown and I'm not going to waste time debugging it. I'll spin this off into a separate bug if/when the rest get merged.
Comment on attachment 85786 [details] [review] 1/5] Remove BSD-style LOCAL_CREDS support Review of attachment 85786 [details] [review]: ----------------------------------------------------------------- My instinct when looking at this stuff is to first check what GLib does. In that case, "git grep LOCAL_CREDS" is empty. Although there are also no hits for getpeereid(), so presumably NetBSD...ah hah, they also have a patch for GLib, helpfully described as "patch-ap": http://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/devel/glib2/patches/patch-ap As well as this one for GSocket: http://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/devel/glib2/patches/patch-aq Both of which concur with this one. So, looks good to me! Comment on attachment 85787 [details] [review] [2/6] bus-test: only expect GetConnectionUnixProcessID to succeed sometimes Review of attachment 85787 [details] [review]: ----------------------------------------------------------------- Ok. Comment on attachment 85788 [details] [review] [3/6] _dbus_read_credentials_socket: document where we use each mechanism Review of attachment 85788 [details] [review]: ----------------------------------------------------------------- Definitely useful. Comment on attachment 85789 [details] [review] [4/6] Prefer getpeerucred() over getpeereid() if a platform has both Review of attachment 85789 [details] [review]: ----------------------------------------------------------------- OK. Comment on attachment 85790 [details] [review] [5/6] _dbus_read_credentials_socket: warn or fail at compile time if no support Review of attachment 85790 [details] [review]: ----------------------------------------------------------------- Makes sense. FWIW, these patches all appear to build and run fine on QNX. Thanks for all the work on these--sorry to have opened up such a hornet's nest! Fixed in git for 1.7.6, thanks. Distributions packaging dbus on BSD-derived Unix should probably backport this, but I'm not going to apply it to 1.6.x upstream until it's had more testing. Colin, any opinion on my two patches on Bug #69492, which follow this? Comment on attachment 85791 [details] [doesn't work] Implement NetBSD credentials-passing with LOCAL_PEEREID Using your patch instead of mine is fine, but if pid-passing is expected to work on NetBSD, please grab this part of my patch so the tests will fail if it doesn't: >@@ -1309,7 +1309,8 @@ check_get_connection_unix_process_id (BusContext *context, > { > #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \ > defined(__linux__) || \ >- defined(__OpenBSD__) >+ defined(__OpenBSD__) || \ >+ defined(__NetBSD__) > warn_unexpected (connection, message, "not this error"); > > goto out; ... assuming you have run the tests and they do pass :-) (In reply to comment #31) > Using your patch instead of mine is fine, but if pid-passing is expected to > work on NetBSD, please grab this part of my patch so the tests will fail if > it doesn't ... er, sorry, that was intended for a different bug. |
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.