After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 650885 - implement glib credentials on OpenBSD (hackish)
implement glib credentials on OpenBSD (hackish)
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gdbus
2.28.x
Other OpenBSD
: Normal major
: ---
Assigned To: David Zeuthen (not reading bugmail)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-05-23 16:43 UTC by Antoine Jacoutot
Modified: 2011-05-27 19:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
openbsd-glib-creds-1 (4.49 KB, patch)
2011-05-23 16:43 UTC, Antoine Jacoutot
none Details | Review
openbsd-glib-creds-2 (855 bytes, patch)
2011-05-23 16:43 UTC, Antoine Jacoutot
none Details | Review
openbsd-glib-creds-3 (634 bytes, patch)
2011-05-23 16:44 UTC, Antoine Jacoutot
none Details | Review
openbsd-glib-creds-4 (3.34 KB, patch)
2011-05-23 16:44 UTC, Antoine Jacoutot
none Details | Review
openbsd-glib-creds-5 (2.57 KB, patch)
2011-05-23 16:44 UTC, Antoine Jacoutot
none Details | Review
Add glib credentials support to OpenBSD (12.03 KB, patch)
2011-05-26 16:46 UTC, Antoine Jacoutot
needs-work Details | Review
gdbus regression tests log (5.68 KB, text/plain)
2011-05-26 17:41 UTC, Antoine Jacoutot
  Details
Port gcredentials to OpenBSD. (11.79 KB, patch)
2011-05-27 14:06 UTC, Antoine Jacoutot
committed Details | Review

Description Antoine Jacoutot 2011-05-23 16:43:00 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?
Comment 1 Antoine Jacoutot 2011-05-23 16:43:34 UTC
Created attachment 188400 [details] [review]
openbsd-glib-creds-1
Comment 2 Antoine Jacoutot 2011-05-23 16:43:51 UTC
Created attachment 188401 [details] [review]
openbsd-glib-creds-2
Comment 3 Antoine Jacoutot 2011-05-23 16:44:03 UTC
Created attachment 188402 [details] [review]
openbsd-glib-creds-3
Comment 4 Antoine Jacoutot 2011-05-23 16:44:15 UTC
Created attachment 188403 [details] [review]
openbsd-glib-creds-4
Comment 5 Antoine Jacoutot 2011-05-23 16:44:29 UTC
Created attachment 188404 [details] [review]
openbsd-glib-creds-5
Comment 6 Dan Winship 2011-05-23 16:52:05 UTC
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.)
Comment 7 Antoine Jacoutot 2011-05-23 17:07:16 UTC
(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...
Comment 8 David Zeuthen (not reading bugmail) 2011-05-23 17:19:06 UTC
(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.
Comment 9 David Zeuthen (not reading bugmail) 2011-05-23 17:19:58 UTC
> g_unix_credentials_message_is_supported() returns NULL
                                                    ^^^^

Eh, I meant FALSE, obviously.
Comment 10 Antoine Jacoutot 2011-05-24 08:32:42 UTC
> 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.
Comment 11 Dan Winship 2011-05-24 14:14:40 UTC
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.
Comment 12 David Zeuthen (not reading bugmail) 2011-05-24 14:17:39 UTC
(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!
Comment 13 Antoine Jacoutot 2011-05-26 08:49:50 UTC
> 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.
Comment 14 Dan Winship 2011-05-26 13:33:59 UTC
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().
   */

:-)
Comment 15 Antoine Jacoutot 2011-05-26 13:56:39 UTC
(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.
Comment 16 Antoine Jacoutot 2011-05-26 16:45:42 UTC
Ok, attaching the first attempt of a patch that should implement what was talked about.
Comment 17 Antoine Jacoutot 2011-05-26 16:46:31 UTC
Created attachment 188683 [details] [review]
Add glib credentials support to OpenBSD
Comment 18 David Zeuthen (not reading bugmail) 2011-05-26 17:12:54 UTC
(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)
Comment 19 Antoine Jacoutot 2011-05-26 17:41:11 UTC
> 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.
Comment 20 Antoine Jacoutot 2011-05-26 17:41:39 UTC
Created attachment 188691 [details]
gdbus regression tests log
Comment 21 David Zeuthen (not reading bugmail) 2011-05-27 12:51:15 UTC
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."
Comment 22 Antoine Jacoutot 2011-05-27 13:03:47 UTC
(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.
Comment 23 David Zeuthen (not reading bugmail) 2011-05-27 13:14:19 UTC
(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 24 Dan Winship 2011-05-27 13:35:22 UTC
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".
Comment 25 Antoine Jacoutot 2011-05-27 14:05:20 UTC
(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.
Comment 26 Antoine Jacoutot 2011-05-27 14:06:11 UTC
Created attachment 188754 [details] [review]
Port gcredentials to OpenBSD.
Comment 27 Dan Winship 2011-05-27 19:35:43 UTC
committed. thanks for the patch