Created attachment 161499 [details] [review]
Patch

See the patch for details.

Matthias later pointer me to http://pastebin.com/9q2A9fX2 which is a bit different but not much.

(In reply to comment #2)
> Matthias later pointer me to http://pastebin.com/9q2A9fX2 which is a bit
> different but not much.

which has

+gchar *
+g_credentials_get_windows_sid (GCredentials    *credentials,
+                              GError         **error)

I'm no Win32 expert but I think it needs to be _user_sid() instead of just _sid(). I also think that if we want GCredentials support on Win32 we need to export the entire access token as the "native" datatype - cf. g_credentials_[get,set]_native_type().

But now I don't think we really want GCredentials support on Win32 (I did when I wrote the initial D-Bus auth code). The reason I think we don't want it is that we don't need it (for D-Bus at least). Should probably first Win32 support when we have something that can return it (like e.g. g_socket_get_peer()). See bug 617483 for more details.

Adding Dan Winship to the Cc since he was involved in the GCredentials discussion. Thanks for any insight.

(In reply to comment #3)
> Should probably first Win32 support
> when we have something that can return it (like e.g. g_socket_get_peer()). See
> bug 617483 for more details.

What I meant to say here is that we probably don't need to implement the Win32 of GCredentials _until_ we have a GIOStream-based type for where we can reliably and securely determine the peer at the other end (like we can with unix domain sockets). For example, if we had a GWin32NamedPipe class (similar to GUnixConnection) then it would make sense to implement GCredentials for Win32 and have g_win32_named_pipe_get_peer_credentials().

(Actually having GWin32NamedPipe would be really nice. Then we could also use the EXTERNAL D-Bus authentication mechanism (which doesn't rely on a secure homedir) for D-Bus over it.)

(In reply to comment #2)
> Matthias later pointer me to http://pastebin.com/9q2A9fX2 which is a bit
> different but not much.

Another observation is that this patch brings back GFileDescriptorBased on non-UNIX and I think that's a bug. I mean, it's just a coincidence that the GLocalFile implementation we use on Win32 uses file descriptors. It could just as well have been Win32 HANDLE based or some other Win32-specific API. Anyway, if we do this we need to clean up GFileDescriptorBased to say that it's available outside UNIX etc. etc.

(In reply to comment #5)
> Another observation is that this patch brings back GFileDescriptorBased on
> non-UNIX and I think that's a bug.

Yeah. The problem with GFileDescriptorBased on windows is that the kind of file descriptor in GLocalFileInputStream is completely different from the kind of file descriptor in GSocketInputStream. Which makes the abstraction pointless.

Yeah, the credentals stuff in my patch was a very quick hack just to make it compile, without really thinking. You can certainly ignore my patch, David of course actually knows the code. The GFileDescriptorBased thing I couldn't really wrap my head around how to solve cleanly.

Comment on attachment 161499 [details] [review]
Patch

(checking just the GFileDescriptorBased stuff)

>- * Note that <filename>&lt;gio/gfiledescriptorbased.h&gt;</filename> belongs to
>- * the UNIX-specific GIO interfaces, thus you have to use the
>- * <filename>gio-unix-2.0.pc</filename> pkg-config file when using it.
>- *

That deletion is accidentally left over from an earlier patch, right? The text should be kept?

>@@ -50,6 +53,9 @@ g_local_file_io_stream_finalize (GObject *object)
>   G_OBJECT_CLASS (g_local_file_io_stream_parent_class)->finalize (object);
> }
> 
>+#ifndef G_OS_UNIX
>+#endif
>+

cruft

>+#ifdef G_OS_UNIX
>   fd = g_file_descriptor_based_get_fd (G_FILE_DESCRIPTOR_BASED (output_stream));
>   stream->input_stream = (GInputStream *)_g_local_file_input_stream_new (fd);
>+#else
>+  fd = _g_local_file_output_stream_get_fd (output_stream);
>+#endif

you're not actually creating stream->input_stream in the non-UNIX case. (And if you're going to add back the internal get_fd method, there's no real reason to not just use it in both cases...)

(In reply to comment #8)
> (From update of attachment 161499 [details] [review])
> (checking just the GFileDescriptorBased stuff)

Thanks for the review. I've pushed the patch with those fixes:

http://git.gnome.org/browse/glib/commit/?id=366b3ffcde4f19cabf8685efdc1ccd20dcade0ca

(In reply to comment #7)
> Yeah, the credentals stuff in my patch was a very quick hack just to make it
> compile, without really thinking. You can certainly ignore my patch, David of
> course actually knows the code. The GFileDescriptorBased thing I couldn't
> really wrap my head around how to solve cleanly.

OK, I pushed my patch. A Win32 questionn: How do I check that some directory is readable only by the logged in user? On UNIX it's a simple stat(). Or can we assume that the home directory on Windows is only readable by the user who owns it?

> How do I check that some directory is readable only by the logged in user?

In some way that is too complex manner to bother with... In fact I have no idea.

> On UNIX it's a simple stat()

Even on some weird installation that for some reason uses ACLs? ;)

> Or can we assume that the home directory on Windows 
> is only readable by the user who owns it?

Well, I think we can assume that if somebody deliberably sets it up differently, they deserve what they get...