Comment on attachment 245880 [details] [review]
Define preprocessor symbols for the extent of GCredentials  support

>I've used separate macros to support OSs where you can
>credentials-pass a uid but not a pid. I have no idea whether such
>OSs really exist

Yes. OS X.

> /**
>+ * G_CREDENTIALS_HAVE_NATIVE:
>+ *
>+ * Defined to 1 if g_credentials_get_native() can work on the current
>+ * platform; undefined if it cannot.
>+ *
>+ * Since: 2.38
>+ */

This basically means "does glib have GCredentials support for this platform?". There's no particular connection to g_credentials_get_native() as opposed to any other GCredentials method. So I'd call it G_CREDENTIALS_SUPPORTED or G_CREDENTIALS_IMPLEMENTED or something like that, and document it more generically.

(In reply to comment #1)
> (From update of attachment 245880 [details] [review])
> >I've used separate macros to support OSs where you can
> >credentials-pass a uid but not a pid. I have no idea whether such
> >OSs really exist
> 
> Yes. OS X.

I didn't see any support for OS X credentials in GLib. Do they take the FreeBSD code path, or is this a feature that hasn't been implemented in GLib yet?

> >+ * G_CREDENTIALS_HAVE_NATIVE:
...
> This basically means "does glib have GCredentials support for this platform?".

At the moment yes, but is that always going to be true?

Imagine a platform where the platform doesn't define a struct { uid, gid, pid }, but you can individually credentials-pass the uid (and maybe other process attributes). What would GLib do there? Would GCredentials exist, and support get_uid() but not get_native()? Or would get_native() return a uid_t*, or would we invent a GLib-specific struct for get_native() to return, or what?

(I don't know whether this is hypothetical or not - apparently Cygwin, Blackberry and QNX all have getpeereid(), so one of them might actually fit this description.)

I wouldn't oppose adding G_CREDENTIALS_SUPPORTED, and I wouldn't mind omitting G_CREDENTIALS_HAVE_NATIVE if you don't think it's actually any use. I think there's a valid argument that to be able to get any use out of get_native(), you have to know which flavour this platform uses (and know a platform API to which it can be passed), which means you already need a whitelist of supported platforms.

+/* This version is what gtk-doc and gobject-introspection will see.
+ * It gets undefined below if not appropriate for the platform. */
+#define G_CREDENTIALS_HAVE_NATIVE 1
+#define G_CREDENTIALS_HAVE_UNIX_PID 1
+#define G_CREDENTIALS_HAVE_UNIX_USER 1
+
+#ifndef __GTK_DOC_IGNORE__
+#ifndef __GI_SCANNER__
...

With hindsight, I should have made all of this #ifndef __GI_SCANNER__ - g-i users should just call the method and see whether it works.

(In reply to comment #2)
> I didn't see any support for OS X credentials in GLib. Do they take the FreeBSD
> code path, or is this a feature that hasn't been implemented in GLib yet?

It hasn't been implemented. GCredentials has mostly only been used with GDBus so far, and presumably people don't run dbus on OS X, so no one has cared to add support.

> Imagine a platform where the platform doesn't define a struct { uid, gid, pid
> }, but you can individually credentials-pass the uid (and maybe other process
> attributes). What would GLib do there? Would GCredentials exist, and support
> get_uid() but not get_native()?

Yes. So I guess SUPPORTED and HAVE_NATIVE aren't actually identical, but as you note, get_native() is only useful in contexts where you already know it's going to work (it's defined to be a programmer error to pass the wrong GCredentialsType), so there's not much use for HAVE_NATIVE.

OK, I'll try to sort out a patch with these changes:

* drop HAVE_NATIVE (useless)

* add SUPPORTED or something (whose semantics are something like:
  HAVE_PID || HAVE_UID || HAVE_NATIVE || any future thing)

* have these symbols show in gtk-doc but not g-i

What would you think of an additional patch something like this (pseudo-patch)?

>  G_CREDENTIALS_TYPE_LINUX_UCRED
>  The native credentials type is a struct ucred.
> +Available on Linux (in C, check for the __linux__ macro).
>
>  G_CREDENTIALS_TYPE_FREEBSD_CMSGCRED
>  The native credentials type is a struct cmsgcred.
> +Available on FreeBSD and GNU/kFreeBSD
> +(in C, check for the __FreeBSD__ or __FreeBSD_kernel__ macro).
>
>  G_CREDENTIALS_TYPE_OPENBSD_SOCKPEERCRED
>  The native credentials type is a struct sockpeercred. Added in 2.30.
> +Available on OpenBSD (in C, check for the __OpenBSD__ macro).

I would hope that if more platforms are added to an existing credentials type, the same patch that added support would document the new platforms. For instance, when GNU/Hurd eventually implements credentials-passing, it seems their plan is to use FreeBSD-style cmsgcred. If so, the patch adding that would change the FreeBSD case to say "Available on FreeBSD, GNU/kFreeBSD and Hurd (in C, check for the __FreeBSD__, __FreeBSD_kernel__ or __GNU__ macro)".

yeah, that makes sense

bug 705029 adds Solaris credential support, but ends up needing to add this in several different files:

>+# if defined(__sun__) || defined(__illumos__) || defined (__OpenSolaris_kernel__)
>+#  define USE_SOLARIS_UCRED
>+# endif

An alternative would be to add something like G_CREDENTIALS_NATIVE_TYPE, and gcredentials.h would do:

  #ifdef __linux__
    #define G_CREDENTIALS_NATIVE_TYPE G_CREDENTIALS_TYPE_LINUX_UCRED
  #elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
    #define G_CREDENTIALS_NATIVE_TYPE G_CREDENTIALS_TYPE_FREEBSD_CMSGCRED
  #elif defined(__OpenBSD__)
    #define G_CREDENTIALS_NATIVE_TYPE G_CREDENTIALS_TYPE_OPENBSD_SOCKPEERCRED
  #elif defined(__sun__) || defined(__illumos__) || defined (__OpenSolaris_kernel__)
    #define G_CREDENTIALS_NATIVE_TYPE G_CREDENTIALS_TYPE_SOLARIS_UCRED
  #else
    #define G_CREDENTIALS_NATIVE_TYPE G_CREDENTIALS_TYPE_INVALID
  #endif

and then just

  #define G_CREDENTIALS_HAVE_NATIVE (G_CREDENTIALS_NATIVE_TYPE != G_CREDENTIALS_TYPE_INVALID)

and then everywhere we manipulate credentials we would use G_CREDENTIALS_NATIVE_TYPE rather than the platform #ifdefs. Eg:

  struct _GCredentials
  {
    /*< private >*/
    GObject parent_instance;

  #if G_CREDENTIALS_NATIVE_TYPE == G_CREDENTIALS_TYPE_LINUX_UCRED
    struct ucred native;
  #elif G_CREDENTIALS_NATIVE_TYPE == G_CREDENTIALS_TYPE_FREEBSD_CMSGCRED
    struct cmsgcred native;
  #elif G_CREDENTIALS_NATIVE_TYPE == G_CREDENTIALS_TYPE_OPENBSD_SOCKPEERCRED
    struct sockpeercred native;
  #else
  #ifdef __GNUC__
  #warning Please add GCredentials support for your OS
  #endif
  #endif
  };

The clean-up-y parts are now in wip/danw/creds. I did it all internally so we can commit it to the stable branch once the freeze is over, but we can move parts of it into gcredentials.h for 2.40.

pushed the cleanup stuff

Do we actually need to know the native struct used on a particular platform?

I was about to copy'n'paste the same code yet again for my OS which uses struct unpcbid. This is yet another struct which contains a pid_t, a uid_t and a gid_t.

Could at least the linux, freebsd and openbsd all be combined, use a e.g.

typedef struct
  {
    pid_t pid;
    uid_t uid;
    gid_t gid;
  }
g_cred_t;

all be filled with getpid() etc., and use some autoconfigury around the call to {g,s}etsockopt() to take care of the various native structures?

Have I missed something (e.g., a use outside of glib), or go forth and generate a patch?

(In reply to comment #10)
> I was about to copy'n'paste the same code yet again for my OS which uses struct
> unpcbid. This is yet another struct which contains a pid_t, a uid_t and a
> gid_t.

Ideally, OSs with a credentials API identical to Linux's, FreeBSD's or OpenBSD's would offer it under the same name as the corresponding better-known one, like the way the Hurd developers apparently plan to offer something that is identical to the FreeBSD API rather than inventing their own thing :-)

(To some extent we've already lost there - the Linux and OpenBSD APIs are very similar, modulo naming.)

> typedef struct
>   {
>     pid_t pid;
>     uid_t uid;
>     gid_t gid;
>   }
> g_cred_t;
> 
> all be filled with getpid() etc., and use some autoconfigury around the call to
> {g,s}etsockopt() to take care of the various native structures?

That's pretty much what GCredentials is. I would prefer portable applications to be able to use g_credentials_get_pid(), g_unix_connection_send_credentials(), etc. rather than messing about with autoconf and setsockopt.

Having done the work for this in libdbus (which does not use GLib, and contains its own GLib-style "make C not awful" library), I am firmly opposed to having code paths for different auto-detected credentials APIs (HAVE_SCM_CREDS, etc.) like libdbus does, without documenting very, very clearly which OSs can be expected to use each of those APIs.

In general I'm in favour of "test for features, not OSs", but I think credentials-passing is one situation where the approach GLib is taking here (test for specific OSs) is actually better. All the OSs do this differently (except FreeBSD and kFreeBSD, which are different test macros for the same kernel, and Hurd, which is going to mimic FreeBSD), and perhaps more importantly, the credentials-passing APIs are not type-safe due to using setsockopt and similar generic OS interfaces, so something compiling successfully is still no indication that it will work.

(In reply to comment #10)
> Do we actually need to know the native struct used on a particular platform?

If we need to be able to map between "native" and GCredentials (which we do, partly for GIO's internal use and partly because it's part of the API now) then the developer should be able to write assertions about it.

My original motivation for writing this patch was that gdm is specifically only portable to platforms where credentials-passing transfers the process ID. According to my notes in libdbus, with enough support code in GLib this could cover Linux, OpenBSD, (k)FreeBSD, DragonflyBSD and Solaris, but not platforms that rely on getpeereid() like NetBSD and QNX.

Older versions of gdm had a conditional #error so that they would not even compile on platforms where they weren't going to work anyway, but this was lost in commit 87f18148. It would be nice to be able to write:

#ifndef G_CREDENTIALS_HAVE_UNIX_PID
#error gdm will not work on this platform: credentials-passing does not include the pid
#endif

G_CREDENTIALS_HAVE_NATIVE was a "would be nice" more than anything else.

(In reply to comment #11)
> (In reply to comment #10)
> > I was about to copy'n'paste the same code yet again for my OS which uses struct
> > unpcbid. This is yet another struct which contains a pid_t, a uid_t and a
> > gid_t.
> 
> Ideally, OSs with a credentials API identical to Linux's, FreeBSD's or
> OpenBSD's would offer it under the same name as the corresponding better-known
> one, like the way the Hurd developers apparently plan to offer something that
> is identical to the FreeBSD API rather than inventing their own thing :-)
> 
> (To some extent we've already lost there - the Linux and OpenBSD APIs are very
> similar, modulo naming.)
> 
> > typedef struct
> >   {
> >     pid_t pid;
> >     uid_t uid;
> >     gid_t gid;
> >   }
> > g_cred_t;
> > 
> > all be filled with getpid() etc., and use some autoconfigury around the call to
> > {g,s}etsockopt() to take care of the various native structures?
> 
> That's pretty much what GCredentials is.

As per the listing in Comment 7, I think it could be, but it isn't. I don't understand why it is preferable to use a struct cmsgcred or whatever, when a generic g_cred_t would do?

The other side to this, is that everywhere which uses e.g. struct cmsgcred, needs to #include the header it is defined in. If this ends up being a socket.h, that might be alright, but if I add a version which uses a struct unpcbid, then I need to pollute glib with #include <sys/un.h>.

It seems to me that the only place where we need to know the actual struct type is at the getsockopt call. In which case, why not play the #ifdef dance there?

I was in fact surprised to find that in gsocket.c, the one place where I would have expected the #ifdef forest, there is an amalgam of the Solaris #ifdef, and a native attempt (which is actually tricky, given that the name of the option is platform dependent(!) (oh for posix as you say in your dbus bug...))

> I would prefer portable applications
> to be able to use g_credentials_get_pid(),
> g_unix_connection_send_credentials(), etc. rather than messing about with
> autoconf and setsockopt.

Well yes, glib provides a portable g_credentials_get_unix_pid(), and we do the autoconf / #ifdef dance within glib, so that portable apps don't need to. If we can't figure out how to get the pid, we rig g_credentials_get_unix_pid() to return -1 and set an error.


> Having done the work for this in libdbus (which does not use GLib, and contains
> its own GLib-style "make C not awful" library), I am firmly opposed to having
> code paths for different auto-detected credentials APIs (HAVE_SCM_CREDS, etc.)
> like libdbus does, without documenting very, very clearly which OSs can be
> expected to use each of those APIs.

I may be missing something again: here we are focusing on grabbing the credentials via socket options rather than sending out messages to get them?

> In general I'm in favour of "test for features, not OSs", but I think
> credentials-passing is one situation where the approach GLib is taking here
> (test for specific OSs) is actually better. All the OSs do this differently
> (except FreeBSD and kFreeBSD, which are different test macros for the same
> kernel, and Hurd, which is going to mimic FreeBSD), and perhaps more
> importantly, the credentials-passing APIs are not type-safe due to using
> setsockopt and similar generic OS interfaces, so something compiling
> successfully is still no indication that it will work.

It's more the "why use an OS specific struct when a generic one might do" which I am bothered about. I would then go for "test for features" as icing on the cake ;-)

(In reply to comment #12)
> (In reply to comment #10)
> Older versions of gdm had a conditional #error so that they would not even
> compile on platforms where they weren't going to work anyway, but this was lost
> in commit 87f18148. It would be nice to be able to write:
> 
> #ifndef G_CREDENTIALS_HAVE_UNIX_PID
> #error gdm will not work on this platform: credentials-passing does not include
> the pid
> #endif

That sounds fine...

(In reply to comment #13)
> > That's pretty much what GCredentials is.
> 
> As per the listing in Comment 7, I think it could be, but it isn't. I don't
> understand why it is preferable to use a struct cmsgcred or whatever, when a
> generic g_cred_t would do?

You use GCredentials when you want to be generic and when there's a GLib API for what you want to do. You extract the native type when you need to pass the credential to a non-GLib API.

> The other side to this, is that everywhere which uses e.g. struct cmsgcred,
> needs to #include the header it is defined in.

This is apparently not documented, but

  #include <gio/gnetworking.h>

will pull in all of the correct headers on all platforms.

(Perhaps it would be better to have a "gcredentialsnative.h"?)

> It seems to me that the only place where we need to know the actual struct type
> is at the getsockopt call. In which case, why not play the #ifdef dance there?

gunixcredentialsmessage.c needs to use the native type as well.

> I was in fact surprised to find that in gsocket.c, the one place where I would
> have expected the #ifdef forest, there is an amalgam of the Solaris #ifdef, and
> a native attempt (which is actually tricky, given that the name of the option
> is platform dependent(!) (oh for posix as you say in your dbus bug...))

On what platform is it not SO_PEERCRED?

At any rate, if we just had a dumb GCredentials type, then we'd need to have separate code to marshal it into the correct native type for each of the three SO_PEERCRED-using platforms, and then we'd need to have all that code again in gunixcredentialsmessage.c, and again in gdm, etc.

(In reply to comment #15)
> On what platform is it not SO_PEERCRED?

NetBSD has a LOCAL_PEEREID getsockopt() option. Patches for libdbus exist in "ports", but when I tried implementing it upstream based on those patches, it didn't seem to be passing the pid through correctly, giving it no advantage over getpeereid(), so I stopped wasting time on it. <https://bugs.freedesktop.org/show_bug.cgi?id=69702>

Other OSs supported by libdbus (according to comments in the source code, which are as accurate as I could get them in a reasonable time, but might still be full of lies):

Linux and OpenBSD share SO_PEERCRED for the getsockopt option name, and have basically the same semantics, but pointlessly different names for the data-type.

FreeBSD and DragonflyBSD use SCM_CREDS messages which require cooperation from the sending peer. (Linux can also do a similar SCM_CREDENTIALS, but libdbus doesn't use that, because SO_PEERCRED is better for what D-Bus wants.)

Solaris uses getpeerucred() which isn't getsockopt, but could conceivably be implemented in terms of it.

AIX (?), Blackberry (?), QNX (?), Cygwin, Minix and an assortment of BSD-derived OSs use getpeereid() which, again, isn't getsockopt but could conceivably be implemented in terms of it.

libdbus also used to support LOCAL_CREDS, which is a bit like SCM_CREDS, but I took out the implementation, because it regressed at some point, and didn't even compile by the time I looked at it.

(In reply to comment #16)
> (In reply to comment #15)
> > On what platform is it not SO_PEERCRED?

I guess I should have clarified that I meant "on what platform is there a sockopt that has the semantics of SO_PEERCRED but which is not called SO_PEERCRED", since that's what I read comment 13 as implying. Obviously if the platform has some mechanism that doesn't have the semantics of SO_PEERCRED, then we're going to have to have separate code for it.

> FreeBSD and DragonflyBSD use SCM_CREDS messages

Right, which gio implements via GUnixCredentialsMessage. And then g_unix_connection_send/receive_credentials() abstracts away the difference between the SCM_CREDS/SCM_CREDENTIALS style and the SO_PEERCRED/getpeerucred() style.

(In reply to comment #8)
> The clean-up-y parts are now in wip/danw/creds. I did it all internally so we
> can commit it to the stable branch once the freeze is over, but we can move
> parts of it into gcredentials.h for 2.40.

The internally-in-gcredentialsprivate.h stuff happened, but none of it ever got made public. Simon, do you want to update your patch in terms of the current code and gcredentialsprivate?

(In reply to comment #15)
> > The other side to this, is that everywhere which uses e.g. struct cmsgcred,
> > needs to #include the header it is defined in.
> 
> This is apparently not documented, but
> 
>   #include <gio/gnetworking.h>
> 
> will pull in all of the correct headers on all platforms.
> 
> (Perhaps it would be better to have a "gcredentialsnative.h"?)

Maybe gcredentialsnative.h could be all the appropriate platform-specific #includes, plus the current contents of gcredentialsprivate.h, plus whatever additional macros gdm needs. Then people would include gio/gio.h if they just needed generic GLib-API-level-only credentials support, and gio/gcredentialsnative.h if they also needed to do platform-specific stuff.

(In reply to comment #18)
> The internally-in-gcredentialsprivate.h stuff happened, but none of it ever got
> made public. Simon, do you want to update your patch in terms of the current
> code and gcredentialsprivate?

Thanks. I'll try to get round to it, but no guarantees (I have other priorities right now).

(In reply to comment #17)
> I guess I should have clarified that I meant "on what platform is there a
> sockopt that has the semantics of SO_PEERCRED but which is not called
> SO_PEERCRED", since that's what I read comment 13 as implying.

Yes. It looks as though NetBSD LOCAL_PEEREID is that.

(In reply to comment #18)
> Maybe gcredentialsnative.h could be all the appropriate platform-specific
> #includes, plus the current contents of gcredentialsprivate.h, plus whatever
> additional macros gdm needs. Then people would include gio/gio.h if they just
> needed generic GLib-API-level-only credentials support, and
> gio/gcredentialsnative.h if they also needed to do platform-specific stuff.

That sounds like a nice division. I'm not sure how to get there though:
g_credentials_get_unix_pid() would be in the GLib-API-level-only header.
I would expect the G_CREDENTIALS_HAVE_UNIX_PID to go with it.
In order to decide whether to define it or not, I would have to pull in
the platform-specific stuff.

Maybe I shouldn't expect G_CREDENTIALS_HAVE_UNIX_PID to go with g_credentials_get_unix_pid() and rely on the function returning an error.
In which case the define isn't very useful any more?

Thank you all for the various explanations!

I'm still unsure of the use sockopt / use a message separation - I had assumed that one would use the sockopt if available, but then I wouldn't need to worry about SCM_CREDS et al.

And yes, I was looking into a glib NetBSD implementation - it seems that you have all bases covered, so I just need to cut'n'paste. One big concern is that it seems you have found that NetBSD LOCAL_PEEREID is plain broken - is it more than the problem that there is no guarantee that the pid won't be reused?

(In reply to comment #20)
> (In reply to comment #18)
> And yes, I was looking into a glib NetBSD implementation - it seems that you
> have all bases covered, so I just need to cut'n'paste. One big concern is that
> it seems you have found that NetBSD LOCAL_PEEREID is plain broken - is it more
> than the problem that there is no guarantee that the pid won't be reused?

I can't remember the details, but I think it was a functional failure in a regression test ("I don't get the pid I expect I should get") rather than a design concern about pid reuse. I don't use NetBSD myself, and my employer has no commercial interest in it, so the amount of time I can justify spending on debugging it is very small. I would be happy to review libdbus patches (based on mine, or reimplemented) if you can make it work reliably.

Any pid-based API is susceptible to pid reuse (either by process exit + new process, or more insidiously, by the authenticated process sending a request and then racing to exec() a setuid executable before its credentials are checked, as described in <https://bugs.freedesktop.org/show_bug.cgi?id=47581#c3>), but that's orthogonal to whether you get the correct pid in the first place.

(In reply to comment #20)
> Maybe I shouldn't expect G_CREDENTIALS_HAVE_UNIX_PID to go with
> g_credentials_get_unix_pid() and rely on the function returning an error.
> In which case the define isn't very useful any more?

Simon originally wanted it because he wanted gdm to refuse to build if it wasn't defined, so it would still be useful for that case.

> I'm still unsure of the use sockopt / use a message separation - I had assumed
> that one would use the sockopt if available, but then I wouldn't need to worry
> about SCM_CREDS et al.

The issue is that neither a sockopt nor SCM_CREDS is available on all platforms. (Of the currently-supported set, FreeBSD doesn't support SCM_CREDS and OpenBSD doesn't support anything sockopt-like.) So GCredentials has to support both models.

From the perspective of someone porting GCredentials to a new platform, you don't have to worry about SCM_CREDS, other than setting G_CREDENTIALS_UNIX_CREDENTIALS_MESSAGE_SUPPORTED in gcredentialsprivate.h if it's available.

(FYI, bug 617483, the original GCredentials bug, has all the original discussion that led to the current API.)

(In reply to comment #16)
> (In reply to comment #15)
> > On what platform is it not SO_PEERCRED?
> 
> NetBSD has a LOCAL_PEEREID getsockopt() option. Patches for libdbus exist in
> "ports", but when I tried implementing it upstream based on those patches, it
> didn't seem to be passing the pid through correctly, giving it no advantage
> over getpeereid(), so I stopped wasting time on it.
> <https://bugs.freedesktop.org/show_bug.cgi?id=69702>

The patch I attached to your libdbus bug "works for me". (Not sure why yours
didn't - I'll have to try it properly.)


> Other OSs supported by libdbus

Seems like a complete list!

(In reply to comment #22)
> (FYI, bug 617483, the original GCredentials bug, has all the original
> discussion that led to the current API.)

Thanks for the pointer!

> From the perspective of someone porting GCredentials to a new platform, you
> don't have to worry about SCM_CREDS, other than setting
> G_CREDENTIALS_UNIX_CREDENTIALS_MESSAGE_SUPPORTED in gcredentialsprivate.h if
> it's available.

I just popped a patch into bug 728256 - SCM_CREDS appears as the
g_unix_credentials_message_get_msg_type() - but presumably one wouldn't actually use it?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/711.