Created attachment 369508 [details] [review]
gdbus: add Windows SSPI NTLM authentication mechanism and  credentials

This is the same implementation as the one submitted to dbus[0],
but adapted to gdbus (which mostly means absence of out-of-memory
handlers).

This makes almost no changes to existing auth code, except for
G_PARAM_CONSTRUCT_ONLY flag for the "credentials" property
of gdbusauthmechanism. Because credentials are obtained
at the end of negotation (for which the mechanism must
already exist), it's not possible to specify them at construction
time only (unlike EXTERNAL mechanism, which can fetch credentials
immediately after establishing a connection, before the mechanism
is chosen and created).

GCredentials now supports Windows SID credentials.
SID can't be passed as metadata on a socket.
Currently it can only be obtained from gdbus.

This patch also adds the test for it.

[0] https://bugs.freedesktop.org/show_bug.cgi?id=96577

Comment on attachment 369508 [details] [review]
gdbus: add Windows SSPI NTLM authentication mechanism and  credentials

I don't know if this makes sense from a D-Bus perspective, or if the use of Windows APIs is exactly right, but I can comment on the GCredentials stuff...

>GCredentials now supports Windows SID credentials.
>SID can't be passed as metadata on a socket.
>Currently it can only be obtained from gdbus.

If it's possible to make it work with the other D-Bus stuff, I think it would make more sense to have the Windows native type be the whole Access Token, and then have g_credentials_get_windows_user() to get the SID, and g_credentials_get_windows_process_handle() to get a process handle (parallel to g_credentials_get_unix_user() and g_credentials_get_unix_pid()).

>-#if G_CREDENTIALS_USE_SOLARIS_UCRED
>+#if G_CREDENTIALS_USE_SOLARIS_UCRED || G_CREDENTIALS_USE_WINDOWS_SID
>   return credentials->native;
> #elif G_CREDENTIALS_SUPPORTED
>   return &credentials->native;

Maybe have Windows and Solaris define G_CREDENTIALS_NATIVE_TYPE_IS_POINTER in gcredentialsprivate.h?

>+    g_warning ("Trying to set credentials of type windows-sid "
>+               "from a Sid value 0x%p that is not valid.", native);

I think it should be "SID" in all caps in any user-facing context (error messages and doc strings). "Sid" is just how it appears in APIs for consistency with Windows API naming, I think.

>+++ b/gio/gsocket.c
>@@ -5481,6 +5481,8 @@ g_socket_receive_messages_with_timeout (GSocket        

>+#elif G_CREDENTIALS_USE_WINDOWS_ACCESS_TOKEN
>+  /* This function is not supported */

That symbol isn't defined anywhere, which is good, because there's no reason to not support g_socket_receive_messages_with_timeout(), which already works fine on Windows AFAIK.

>@@ -5721,6 +5723,8 @@ g_socket_get_credentials (GSocket   *socket,

>+#elif G_CREDENTIALS_USE_WINDOWS_SID
>+  /* This function is not supported */
> #else
>   #error "G_CREDENTIALS_SOCKET_GET_CREDENTIALS_SUPPORTED is set but this is no code for this platform"
> #endif

You don't need that; that's inside a check for #ifdef G_CREDENTIALS_SOCKET_GET_CREDENTIALS_SUPPORTED, which you're not defining, so the #error won't be reached. (Instead it will hit the code path that just returns a G_IO_ERROR_NOT_SUPPORTED.)

(So, you can just drop all the gsocket.c changes.)

>+++ b/gio/gwin32sid.c

This file needs to be transcoded from Windows-1251 to UTF-8.

>+_g_win32_sid_replace (SID **dest,

You don't need to underscore-prefix internal functions; anything that doesn't have a GLIB_AVAILABLE_IN_* tag in its header file will end up internal automatically.

Review of attachment 369508 [details] [review]:

Here’s a really quick high-level review from the GLib point of view:
 • This isn’t going to get accepted or reviewed in detail for GLib until it’s landed in dbus.git, so I suggest you focus your efforts there.
 • The patch needs splitting up into smaller, atomic commits. That will help it get reviewed and merged faster (once it’s landed in dbus.git).
 • The patch needs meson support.
 • Please make sure there is extensive test coverage; I don’t know how much coverage the tests you’ve got so far give, but (without checking using lcov) they don’t look comprehensive.

I’m going to mark the patch as rejected for now, to get it off the list of pending patches, but please don’t take that as a permanent rejection. Once the SSPI auth mechanism has been merged into dbus.git upstream, we can look at this again. Thanks!

(In reply to Dan Winship from comment #2)
> Comment on attachment 369508 [details] [review] [review]
> gdbus: add Windows SSPI NTLM authentication mechanism and  credentials
> 
> I don't know if this makes sense from a D-Bus perspective, or if the use of
> Windows APIs is exactly right, but I can comment on the GCredentials stuff...
> 
> >GCredentials now supports Windows SID credentials.
> >SID can't be passed as metadata on a socket.
> >Currently it can only be obtained from gdbus.
> 
> If it's possible to make it work with the other D-Bus stuff, I think it
> would make more sense to have the Windows native type be the whole Access
> Token, and then have g_credentials_get_windows_user() to get the SID, and
> g_credentials_get_windows_process_handle() to get a process handle (parallel
> to g_credentials_get_unix_user() and g_credentials_get_unix_pid()).

I kind of wanted to do that originally (current version of the patch even has a check for G_CREDENTIALS_USE_WINDOWS_ACCESS_TOKEN leftover, which i'll fix later). It's just that access tokens require more massaging than the other native credentials to get the useful data, and they can also be (ab)used for other purposes (though the arguments that i feed to SSPI ensure that these purposes are practically limited to identification only). SID is just a chunk of opaque memory, nothing exotic about it, so it's easier to work with (can be easily copied, for example - there's an API for that, CopySid()). I could try access tokens again, but the code will be uglier, mark my words

Note that access tokens[0] do not contain information about processes. There's no normal way to get process ID or process handle out of an access token, AFAICS. Really, all you get is a SID (well, you can get some other things, like different SIDs, and DACL, but i'm not sure glib wants these). So short-circuiting it made sense at the time, getting SID right away and then using that as the credentials.

This is also why credentials.c test patch is so small - it only does half the testing, because there's no "credentials" vs "uid or pid you get from credentials" dichotomy - W32 credentials is just SID, this SID is a W32 equivalent of UNIX uid, and there's nothing else [useful] you can get from the access token.

> 
> >-#if G_CREDENTIALS_USE_SOLARIS_UCRED
> >+#if G_CREDENTIALS_USE_SOLARIS_UCRED || G_CREDENTIALS_USE_WINDOWS_SID
> >   return credentials->native;
> > #elif G_CREDENTIALS_SUPPORTED
> >   return &credentials->native;
> 
> Maybe have Windows and Solaris define G_CREDENTIALS_NATIVE_TYPE_IS_POINTER
> in gcredentialsprivate.h?

No opinion either way.

> >+++ b/gio/gsocket.c
> >@@ -5481,6 +5481,8 @@ g_socket_receive_messages_with_timeout (GSocket        
> 
> >+#elif G_CREDENTIALS_USE_WINDOWS_ACCESS_TOKEN
> >+  /* This function is not supported */
> 
> That symbol isn't defined anywhere, which is good, because there's no reason
> to not support g_socket_receive_messages_with_timeout(), which already works
> fine on Windows AFAIK.

Yeah, looking at it now, i think it's a badly-applied patch. There's another occurrence of "/* This function is not supported */", in g_socket_get_credentials(), which is actually *not* supported. Though it looks like i won't need to touch gsocket.c at all.

> 
> (So, you can just drop all the gsocket.c changes.)

Yay!

> 
> >+++ b/gio/gwin32sid.c
> 
> This file needs to be transcoded from Windows-1251 to UTF-8.

My bad.

(In reply to Philip Withnall from comment #3)
> Review of attachment 369508 [details] [review] [review]:
> 
> Here’s a really quick high-level review from the GLib point of view:
>  • This isn’t going to get accepted or reviewed in detail for GLib until
> it’s landed in dbus.git, so I suggest you focus your efforts there.

Not much i can do, the dbus patch is currently as good as it could be, just sitting there...waiting for review...indefinitely...slowly bitrotting into oblivion.

>  • The patch needs splitting up into smaller, atomic commits. That will help
> it get reviewed and merged faster (once it’s landed in dbus.git).

One obvious way to do that is to split GCredentials and GDbus changes into separate patches. Other than that...i don't see a way to split it further while keeping the codebase in a buildable state. Okay, maybe the testcase can also be split. That's 3 pieces; 4 pieces, if auth and credential tests are to be separate. Meson support could be a 5th, depending on how essential you think it is (if "can't be built with meson" is a build-breaker, than it'll have to stay with the rest). Generally, my policy is that it is very desirable for a branch to be buildable after each commit (otherwise git-bisecting is a PitA), and for each commit to introduce meaningful changes (so i shouldn't be adding functions that are only going to be used by code introduced in the next commit - that's the same thing as pushing just one big commit).


[0] https://msdn.microsoft.com/en-us/library/windows/desktop/aa374909(v=vs.85).aspx

(In reply to LRN from comment #4)
> (In reply to Philip Withnall from comment #3)
> > Review of attachment 369508 [details] [review] [review] [review]:
> > 
> > Here’s a really quick high-level review from the GLib point of view:
> >  • This isn’t going to get accepted or reviewed in detail for GLib until
> > it’s landed in dbus.git, so I suggest you focus your efforts there.
> 
> Not much i can do, the dbus patch is currently as good as it could be, just
> sitting there...waiting for review...indefinitely...slowly bitrotting into
> oblivion.

I’ve commented there. I will see if I can find time to help out with the review there, but D-Bus is quite underresourced and I don’t know how far up Simon’s priority list this SSPI stuff is.

> >  • The patch needs splitting up into smaller, atomic commits. That will help
> > it get reviewed and merged faster (once it’s landed in dbus.git).
> 
> One obvious way to do that is to split GCredentials and GDbus changes into
> separate patches.

Sure, that’s mostly what I had in mind, especially if the GSocket changes are to be dropped.

> Other than that...i don't see a way to split it further
> while keeping the codebase in a buildable state. Okay, maybe the testcase
> can also be split. That's 3 pieces; 4 pieces, if auth and credential tests
> are to be separate. Meson support could be a 5th, depending on how essential
> you think it is (if "can't be built with meson" is a build-breaker, than
> it'll have to stay with the rest).

Keep the test cases with the commits which introduce the code they test. Similarly, Meson support should be in the commits which make the equivalent autotools changes.

> Generally, my policy is that it is very
> desirable for a branch to be buildable after each commit (otherwise
> git-bisecting is a PitA),

Yes, definitely.

> and for each commit to introduce meaningful
> changes (so i shouldn't be adding functions that are only going to be used
> by code introduced in the next commit - that's the same thing as pushing
> just one big commit).

I think this one is situation-dependent. It’s not quite the same as pushing one big commit, as the two commits can be reviewed, iterated on and pushed separately.

(In reply to LRN from comment #4)
> Note that access tokens[0] do not contain information about processes.

Oh. Indeed. I guess I saw that there was OpenProcessToken(), and that tokens contained lots and lots of different information, and then just assumed/hallucinated the existence of a function to get back the process that the token came from.

OK. Well, the design of GCredentials allows for the possibility that a platform might implement multiple types of credential, and might even allow translating between them. So we could still potentially add a SID+process credential in the future without conflicting with this code (assuming that there's a plausible implementation of such a token, and a use case for it, which maybe there isn't.)

-- 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/1351.