Re: [gamin] socket credentials patch for NetBSD

[Daniel Veillard <veillard redhat com>]

On Thu, Sep 01, 2005 at 12:54:18PM -0400, Johnny C. Lam wrote:
> Daniel Veillard wrote:
> >
> >  The fact it won't change the protocol sounds good. I don't understand 
> >  your
> >unique ID idea, it won't solve the lack of authentification.
> 
> Please correct me if I'm wrong, but it looked like the extent of the 
> authentication that gamin does is that the server and the client 
> mutually check that the process at the other end of the socket shares 
> the same UID as itself.  Since we can easily get this information on all 
> of Linux, FreeBSD, NetBSD, OpenBSD, and BSD/OS, it's easy to make gamin 
> work on all of those platforms with the same "strength of authentication".

 The problem is to get it in a trusted way. Your initial patch was basically
believing informations sent from the client, and to me you can't trust those.

> The authentication routines in gam_channel.c also collect and save the 
> PID of the process at the other end of the socket, but that information 
> is used only in debugging messages to help distinguish between different 
> clients. 

  Once passed the authentication phase, the pid is only used for debugging
but good debugging is still quite important at this stage

> My point is that since the PID is used only for that purpose, 
> gamin doesn't actually care if instead of saving the PID, we saved 
> something else instead.  In fact, we could just be saving a "-1" every 
> time, and gamin would still work properly, but in the interests of 
> better debugging messages, we should still try to save a unique integer 
> for each connecting client.

  That won't be sufficient for debugging though. Have you used kill -USR2 
to debug on a system with a running desktop ? It's a serious mess already,
Without proper identification of clients it gets close to unusable IMHO.

> Is my understanding correct?

  We disagree on the importance of the debugging information.

> I've attached a patch that just saves a "-1" instead of the PID in the 
> connection information.  NetBSD and FreeBSD still pass all of the 
> regression test scenarios with this patch.

  It doesn't just change stuff making NetBSD work which is what I asked for,
there is still way too many changes affecting other code paths:

---------------------
 #endif
-    if (st.st_mode & (S_IRWXG|S_IRWXO)) {
-       gam_error(DEBUG_INFO,
-                 "Socket %s has wrong permissions\n",
-                 path);
-       goto cleanup;
-    }
     /*
---------------------

  Why is that code test removed without specific check against your platform
If it breaks on NetBSD I want to see something like

#ifndef NetBSD
    if (st.st_mode & (S_IRWXG|S_IRWXO)) {
       gam_error(DEBUG_INFO,
                 "Socket %s has wrong permissions\n",
                 path);
       goto cleanup;
    }
#endif

  Not just pure removal of that test. 

I think I was clear already about it. I want to se only your platform changes
in the initial patch, no change for other platform code !

-------------------
 static int
 gamin_write_credential_byte(int fd)
 {
-    char data[2] = { 0, 0 };
-    int written;
-#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
-    struct {
-           struct cmsghdr hdr;
-           struct cmsgcred cred;
-    } cmsg;
-    struct iovec iov;
     struct msghdr msg;
+    struct iovec iov;
+    char data = 0;
+    int written;

-    iov.iov_base = &data[0];
-    iov.iov_len = 1;
+#if defined(BSDCRED) && !defined(LOCAL_CREDS)
+    struct cmsghdr *cmsg;
+    char cmsgbuf[CMSG_SPACE(CRED_DATASIZE)];
+#endif
+
+    iov.iov_base = &data;
+    iov.iov_len = sizeof(data);
-------------------

and

-------------------
 retry:
-#if defined(HAVE_CMSGCRED) && !defined(LOCAL_CREDS)
     written = sendmsg(fd, &msg, 0);
-#else
-    written = write(fd, &data[0], 1);
-#endif
     if (written < 0) {
-------------------

  again non-conditionalized platform changes, you change data for all OSes
You may submit later a second patch explaining why this would need to be
changed, but I don't want to see this as part of a "make gamin work on NetBSD"
patch.

  So again, I would be very happy to get such a patch but not changing the
code path or protocol for the other OSes, but the patch you submitted is 
way too intrusive to may taste for other OSes. You should understand why 
I'm so careful, this also mean once we get NetBSD support then I will be
as careful for other arches not changing the behaviour of the code you
provided. And if I can't get that from contributors, I would rather drop
attempting to have gamin portable and focuse only on Linux than spend time
trying to reconciliate patches stepping on each other toes.

  I hope what I want and the rationale for it are clear, and I'm pretty
sure coming with such a platform only first patch should be relatively simple
based on this one. But I won't do this myself simply because I just could
not test it.

   Thanks,

Daniel

-- 
Daniel Veillard      | Red Hat Desktop team http://redhat.com/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/