GNOME Bugzilla – Bug 650885
implement glib credentials on OpenBSD (hackish)
Last modified: 2011-05-27 19:35:46 UTC
Hi. These patches implement glib credentials on OpenBSD. Please note that patch-gio_gunixconnection_c.diff is a hack because we have no way to pass SCM_CREDS over unix sockets on OpenBSD and this is not going to change anytime soon. Since this patch is a bit invasive (although it's all ifdef __OpenBSD__) I'd understand if you object to it. The way we do the workaround is the following: we implement the "intended" SCM_CREDS stack as if we had support for that in the kernel (by-pass it almost completely). send/recv a single null byte without creds, but on recv, just do a getsockopt(SO_PEERCRED) and return that as if it coming from the cmsg. This works as long as creds are not retrieved from an fd which has already been handed over to a different process via SCM_RIGHTS. Thoughts?
Created attachment 188400 [details] [review] openbsd-glib-creds-1
Created attachment 188401 [details] [review] openbsd-glib-creds-2
Created attachment 188402 [details] [review] openbsd-glib-creds-3
Created attachment 188403 [details] [review] openbsd-glib-creds-4
Created attachment 188404 [details] [review] openbsd-glib-creds-5
glib supports the SO_PEERCRED style too, via g_socket_get_credentials(). You should implement that, rather than faking send/receive_credentials. (It looks like gdbusauth currently only uses send/receive_credentials, and doesn't try get_credentials() if that's not supported, but that's a gdbus bug.)
(In reply to comment #6) > glib supports the SO_PEERCRED style too, via g_socket_get_credentials(). You > should implement that, rather than faking send/receive_credentials. > > (It looks like gdbusauth currently only uses send/receive_credentials, and > doesn't try get_credentials() if that's not supported, but that's a gdbus bug.) Indeed. And this is why I ended up going into this hackish way, I though there was a reason behind gdbus not trying g_socket_get_credentials. If that is fixed, then for sure using SO_PEERCRED will be straightforward...
(In reply to comment #7) > (In reply to comment #6) > > glib supports the SO_PEERCRED style too, via g_socket_get_credentials(). You > > should implement that, rather than faking send/receive_credentials. > > > > (It looks like gdbusauth currently only uses send/receive_credentials, and > > doesn't try get_credentials() if that's not supported, but that's a gdbus bug.) > > Indeed. And this is why I ended up going into this hackish way, I though there > was a reason behind gdbus not trying g_socket_get_credentials. If that is > fixed, then for sure using SO_PEERCRED will be straightforward... I agree with danw - we should just make gdbus use g_socket_get_credentials() if - g_unix_credentials_message_is_supported() returns NULL; or - we didn't receive any credentials any chance you can try that? As far as I can tell you should try to do this in the _g_dbus_auth_run_server() function in gio/dbusauth.c around here http://git.gnome.org/browse/glib/tree/gio/gdbusauth.c?id=2.28.7#n991 Thanks.
> g_unix_credentials_message_is_supported() returns NULL ^^^^ Eh, I meant FALSE, obviously.
> any chance you can try that? As far as I can tell you should try to do this in > the _g_dbus_auth_run_server() function in gio/dbusauth.c around here Sure I can give it a shot. Just give me a little time as I'm actually porting GNOME3 to OpenBSD as well ;) I'll keep you posted as soon as I have something. Thanks.
Actually, see bug 617483 comment 3. It look like actually we *did* intend for it to work basically the way it works in the original patch. Which makes sense, because we don't want every caller to have to do the "if supported then this else that" thing. So: - patch g_socket_get_credentials() to support OpenBSD - patch g_unix_connection_send_credentials() to send just a 0 byte if there is no credentials-sending support - patch g_unix_connection_receive_credentials() to call g_socket_get_credentials() if there is no credentials-sending support - don't patch gdbus at all and as mentioned in another bug, please attach it as a single git format patch.
(In reply to comment #11) > Actually, see bug 617483 comment 3. It look like actually we *did* intend for > it to work basically the way it works in the original patch. Which makes sense, > because we don't want every caller to have to do the "if supported then this > else that" thing. > > So: > > - patch g_socket_get_credentials() to support OpenBSD > - patch g_unix_connection_send_credentials() to send just a 0 byte > if there is no credentials-sending support > - patch g_unix_connection_receive_credentials() to call > g_socket_get_credentials() if there is no credentials-sending > support > - don't patch gdbus at all > > and as mentioned in another bug, please attach it as a single git format patch. Yeah, that's probably a better way to do it. Thanks for remembering!
> Yeah, that's probably a better way to do it. Thanks for remembering! Ok I started looking into doing that. While I made it work, I could use some input to come up with something clean. At first I wanted to use g_unix_credentials_message_is_supported() and make g_unix_connection_send_credentials() send a 0 byte is it returned FALSE. This works but if the OS does not support credential messages, then other things will start to fail (e.g. g_unix_credentials_message_new_with_credentials: assertion `g_unix_credentials_message_is_supported ()' failed) and some code in gdbusauth.c will also not be called. A solution could be for OpenBSD to lie about its credential message support and use g_credentials_get_native(credentials, G_CREDENTIALS_TYPE_OPENBSD_SOCKPEERCRED) to set the num_messages to 0 or 1 in g_unix_connection_send_credentials() accordingly. Of course, this is not pretty nor generic. So I'm looking for advise here :-) On a side note, is there some code out there that I could use to test g_unix_connection_receive_credentials() (which I'm currently making it fallback on g_socket_get_credentials() for OpenBSD). Thanks.
g_unix_credentials_message_is_supported() should mean exactly what it means now. It is possible that some code is using it incorrectly. > This > works but if the OS does not support credential messages, then other things > will start to fail (e.g. g_unix_credentials_message_new_with_credentials: > assertion `g_unix_credentials_message_is_supported ()' failed) g_unix_connection_send_credentials() should just not be calling g_unix_credentials_message_new_with_credentials() if !supported. > and some code in > gdbusauth.c will also not be called. Right, the two checks in gdbusauth.c should go away; it should always call g_unix_connection_send/receive_credentials() when G_IS_UNIX_CONNECTION(). (However, the receive_credentials() part needs to check the returned error, and if it's G_IO_ERROR/G_IO_ERROR_NOT_SUPPORTED, then it should discard the error and just continue with credentials set to NULL.) > On a side note, is there some code out there that I could use to test > g_unix_connection_receive_credentials() No, but gio/tests/socket.c has: /* TODO: add test for g_unix_connection_send_credentials() and * g_unix_connection_receive_credentials(). */ :-)
(In reply to comment #14) > g_unix_credentials_message_is_supported() should mean exactly what it means > now. It is possible that some code is using it incorrectly. That was my understanding, thanks for confirming. > > works but if the OS does not support credential messages, then other things > > will start to fail (e.g. g_unix_credentials_message_new_with_credentials: > > assertion `g_unix_credentials_message_is_supported ()' failed) > > g_unix_connection_send_credentials() should just not be calling > g_unix_credentials_message_new_with_credentials() if !supported. Duh, it's so obvious I feel stupid now :-) > > and some code in > > gdbusauth.c will also not be called. > > Right, the two checks in gdbusauth.c should go away; it should always call > g_unix_connection_send/receive_credentials() when G_IS_UNIX_CONNECTION(). Yeah this I had removed already. > (However, the receive_credentials() part needs to check the returned error, and > if it's G_IO_ERROR/G_IO_ERROR_NOT_SUPPORTED, then it should discard the error > and just continue with credentials set to NULL.) Yep, I'll do that. > No, but gio/tests/socket.c has: > > /* TODO: add test for g_unix_connection_send_credentials() and > * g_unix_connection_receive_credentials(). > */ > > :-) Hehe :) Yes I know I saw that some time ago, hence my question ;-) Anyway, cooking up a final patch for review, should be ready (hopefully) within 24h top. And thanks, I do appreciate all the inputs I got so far.
Ok, attaching the first attempt of a patch that should implement what was talked about.
Created attachment 188683 [details] [review] Add glib credentials support to OpenBSD
(In reply to comment #17) > Created an attachment (id=188683) [details] [review] > Add glib credentials support to OpenBSD Does this work with 'make check'? Or at least just some of the gdbus test cases in gio/tests? I'm asking because 'make check' actually exercises this code to communicate with dbus-daemon(1) (instances of dbus-daemon(1) are launched as needed)
> Does this work with 'make check'? Or at least just some of the gdbus test cases > in gio/tests? I'm asking because 'make check' actually exercises this code to > communicate with dbus-daemon(1) (instances of dbus-daemon(1) are launched as > needed) Seems most gdbus checks are happy, I'm attaching the regress log.
Created attachment 188691 [details] gdbus regression tests log
Review of attachment 188683 [details] [review]: Generally looks good to me but would like danw to review the gsocket.c / gunixconnection.c parts. Thanks. ::: gio/gioenums.h @@ +1215,3 @@ * @G_CREDENTIALS_TYPE_LINUX_UCRED: The native credentials type is a <type>struct ucred</type>. * @G_CREDENTIALS_TYPE_FREEBSD_CMSGCRED: The native credentials type is a <type>struct cmsgcred</type>. + * @G_CREDENTIALS_TYPE_OPENBSD_SOCKPEERCRED: The native credentials type is a <type>struct sockpeercred</type>. This should say "Added in 2.30."
(In reply to comment #21) > This should say "Added in 2.30." Sure. Since I'm not a committer I will leave it to whoever commits this patch to extend the comment (unless you want me to attached a new patch in which case just ask ;-)). Thanks for the review.
(In reply to comment #22) > (In reply to comment #21) > > This should say "Added in 2.30." > > Sure. Since I'm not a committer I will leave it to whoever commits this patch > to extend the comment (unless you want me to attached a new patch in which case > just ask ;-)). Thanks for the review. No problem; it's generally better to attach a new patch with the comment since the committer can simply do $ git-am -s path-to.patch In this case, I'd wait with generating a new patch until danw chimes in.
Comment on attachment 188683 [details] [review] Add glib credentials support to OpenBSD ok, besides what David said: >+ if (credentials == NULL && local_error->code != G_IO_ERROR && local_error->code != G_IO_ERROR_NOT_SUPPORTED) first "->code" should be "->domain", but it's easier to just say: if (credentials == NULL && !g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED)) >+#if defined(__linux__) || defined(__OpenBSD__) > { >- struct ucred native_creds; > socklen_t optlen; >+#ifndef __OpenBSD__ here and below, don't use #ifndef, since that guarantees that when we add another platform here we'd have to change it again. Do #ifdef __linux__ ... #else /* __OpenBSD__ */ ... #endif or #if defined(__linux__) ... #elif defined (__OpenBSD__) ... #endif >+ if (g_unix_credentials_message_is_supported()) space before the () (in both functions) >+ G_IO_ERROR_FAILED, >+ _("Expecting 1 control message, got %d"), >+ nscm); can you change the tabs to spaces in the _() line? (This was already broken, but if you're reindenting that line anyway, might as well fix it.) >+ _("Unexpected type of ancillary data")); likewise here. (Wonder why only the _() lines have tabs? Weird...) >+ if (nscm == 0) >+ { >+ g_set_error (error, >+ G_IO_ERROR, >+ G_IO_ERROR_FAILED, >+ _("Expecting 0 control message, got %d"), That should be "nscm != 0" shouldn't it? (Also, tabs again.) Also, I'd probably say "Not expecting control message, but got %d".
(In reply to comment #24) > first "->code" should be "->domain", but it's easier to just say: > > if (credentials == NULL && !g_error_matches (local_error, G_IO_ERROR, > G_IO_ERROR_NOT_SUPPORTED)) Fixed. > here and below, don't use #ifndef, since that guarantees that when we add > another platform here we'd have to change it again. Do That is what I usually do as well, I just wasn't sure what you guys preferred, fixed :) > >+ if (g_unix_credentials_message_is_supported()) > > space before the () (in both functions) Fixed. > can you change the tabs to spaces in the _() line? (This was already broken, > but if you're reindenting that line anyway, might as well fix it.) > > >+ _("Unexpected type of ancillary data")); > > likewise here. (Wonder why only the _() lines have tabs? Weird...) All fixed. > >+ if (nscm == 0) > >+ { > >+ g_set_error (error, > >+ G_IO_ERROR, > >+ G_IO_ERROR_FAILED, > >+ _("Expecting 0 control message, got %d"), > > That should be "nscm != 0" shouldn't it? (Also, tabs again.) Also, I'd Of course! Left over from a testing where I was forcing it to fail, fixed. > probably say "Not expecting control message, but got %d". Much better, fixed. New patch appended. Thanks.
Created attachment 188754 [details] [review] Port gcredentials to OpenBSD.
committed. thanks for the patch