Libguestfs - Oct 2020 - Re: [PATCH nbdkit v2 1/3] server: Add new APIs for reading the client’s SO_PEERCRED.

On 10/3/20 1:50 PM, Richard W.M. Jones wrote:
> New nbdkit_peer_pid, nbdkit_peer_uid and nbdkit_peer_gid calls can be
> used on Linux (only) to read the peer PID, UID and GID from clients
> connected over a Unix domain socket.  This can be used in the
> preconnect phase to add additional filtering.
> 
> One use for this is to add an extra layer of authentication for local
> connections.  A subsequent commit will enhance the now misnamed
> nbdkit-ip-filter to allow filtering on these extra fields.
> 
> It appears as if it would be possible to implement this for FreeBSD
> too (see comment in code).
> ---
>  docs/nbdkit-plugin.pod  |  47 +++++++++++++++--
>  include/nbdkit-common.h |   3 ++
>  server/nbdkit.syms      |   3 ++
>  server/public.c         | 108 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 156 insertions(+), 5 deletions(-)
> 
> +=head2 C<nbdkit_peer_pid>
> +
> +(nbdkit E<ge> 1.24)
> +
> + int nbdkit_peer_pid (void);
> +
> +Return the peer process ID.  This is only available when the client
> +connected over a Unix domain socket, and only works for Linux.
> +
> +On success this returns the peer process ID.  On error,
> +C<nbdkit_error> is called and this call returns C<-1>.
Is int always going to be sufficient?  Or are there platforms with
64-bit pid_t?  Mingw is an interesting beast; I've seen conflicting
stories on whether 64-bit windows has 32- or 64-bit pids (the spawn APIs
manage 64-bit handles, but other windows APIs return 32-bit int), so
64-bit pid_t on mingw does seem to be a real concern.
> +
> +=head2 C<nbdkit_peer_uid>
> +
> +(nbdkit E<ge> 1.24)
> +
> + int nbdkit_peer_uid (void);
> +
> +Return the peer user ID.  This is only available when the client
> +connected over a Unix domain socket, and only works for Linux.
> +
> +On success this returns the user ID.  On error, C<nbdkit_error> is
> +called and this call returns C<-1>.
> +
> +=head2 C<nbdkit_peer_gid>
> +
> +(nbdkit E<ge> 1.24)
> +
> + int nbdkit_peer_gid (void);
int for these two is probably fine.
> +
> +Return the peer group ID.  This is only available when the client
> +connected over a Unix domain socket, and only works for Linux.
> +
> +On success this returns the user ID.  On error, C<nbdkit_error> is
> +called and this call returns C<-1>.
> +
>  =head1 DEBUGGING
>  
> +static int
> +get_peercred (int s, int *pid, int *uid, int *gid)
> +{
> +  struct ucred ucred;
> +  socklen_t n = sizeof ucred;
> +
> +  if (getsockopt (s, SOL_SOCKET, SO_PEERCRED, &ucred, &n) == -1) {
> +    nbdkit_error ("getsockopt: SO_PEERCRED: %m");
> +    return -1;
> +  }
> +
> +  if (pid && ucred.pid >= 1) {
> +    if (ucred.pid <= INT_MAX)
> +      *pid = ucred.pid;
> +    else
> +      nbdkit_error ("pid out of range: cannot be mapped to
int");
> +  }
well, at least you are acknowledging that int might not always map to pid_t.

Otherwise, looks fine to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Oct 05, 2020 at 08:21:50AM -0500, Eric Blake
wrote:
> On 10/3/20 1:50 PM, Richard W.M. Jones wrote:
> > New nbdkit_peer_pid, nbdkit_peer_uid and nbdkit_peer_gid calls can be
> > used on Linux (only) to read the peer PID, UID and GID from clients
> > connected over a Unix domain socket.  This can be used in the
> > preconnect phase to add additional filtering.
> > 
> > One use for this is to add an extra layer of authentication for local
> > connections.  A subsequent commit will enhance the now misnamed
> > nbdkit-ip-filter to allow filtering on these extra fields.
> > 
> > It appears as if it would be possible to implement this for FreeBSD
> > too (see comment in code).
> > ---
> >  docs/nbdkit-plugin.pod  |  47 +++++++++++++++--
> >  include/nbdkit-common.h |   3 ++
> >  server/nbdkit.syms      |   3 ++
> >  server/public.c         | 108
++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 156 insertions(+), 5 deletions(-)
> > 
> 
> > +=head2 C<nbdkit_peer_pid>
> > +
> > +(nbdkit E<ge> 1.24)
> > +
> > + int nbdkit_peer_pid (void);
> > +
> > +Return the peer process ID.  This is only available when the client
> > +connected over a Unix domain socket, and only works for Linux.
> > +
> > +On success this returns the peer process ID.  On error,
> > +C<nbdkit_error> is called and this call returns C<-1>.
> 
> Is int always going to be sufficient?  Or are there platforms with
> 64-bit pid_t?  Mingw is an interesting beast; I've seen conflicting
> stories on whether 64-bit windows has 32- or 64-bit pids (the spawn APIs
> manage 64-bit handles, but other windows APIs return 32-bit int), so
> 64-bit pid_t on mingw does seem to be a real concern.
> 
> > +
> > +=head2 C<nbdkit_peer_uid>
> > +
> > +(nbdkit E<ge> 1.24)
> > +
> > + int nbdkit_peer_uid (void);
> > +
> > +Return the peer user ID.  This is only available when the client
> > +connected over a Unix domain socket, and only works for Linux.
> > +
> > +On success this returns the user ID.  On error, C<nbdkit_error>
is
> > +called and this call returns C<-1>.
> > +
> > +=head2 C<nbdkit_peer_gid>
> > +
> > +(nbdkit E<ge> 1.24)
> > +
> > + int nbdkit_peer_gid (void);
> 
> int for these two is probably fine.
> 
> > +
> > +Return the peer group ID.  This is only available when the client
> > +connected over a Unix domain socket, and only works for Linux.
> > +
> > +On success this returns the user ID.  On error, C<nbdkit_error>
is
> > +called and this call returns C<-1>.
> > +
> >  =head1 DEBUGGING
> >  
> 
> > +static int
> > +get_peercred (int s, int *pid, int *uid, int *gid)
> > +{
> > +  struct ucred ucred;
> > +  socklen_t n = sizeof ucred;
> > +
> > +  if (getsockopt (s, SOL_SOCKET, SO_PEERCRED, &ucred, &n) ==
-1) {
> > +    nbdkit_error ("getsockopt: SO_PEERCRED: %m");
> > +    return -1;
> > +  }
> > +
> > +  if (pid && ucred.pid >= 1) {
> > +    if (ucred.pid <= INT_MAX)
> > +      *pid = ucred.pid;
> > +    else
> > +      nbdkit_error ("pid out of range: cannot be mapped to
int");
> > +  }
> 
> well, at least you are acknowledging that int might not always map to
pid_t.
> 
> Otherwise, looks fine to me.
I wonder if I should just change all of them to int64_t?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Mon, Oct 05, 2020 at 08:21:50AM -0500, Eric Blake
wrote:
> On 10/3/20 1:50 PM, Richard W.M. Jones wrote:
> > New nbdkit_peer_pid, nbdkit_peer_uid and nbdkit_peer_gid calls can be
> > used on Linux (only) to read the peer PID, UID and GID from clients
> > connected over a Unix domain socket.  This can be used in the
> > preconnect phase to add additional filtering.
> > 
> > One use for this is to add an extra layer of authentication for local
> > connections.  A subsequent commit will enhance the now misnamed
> > nbdkit-ip-filter to allow filtering on these extra fields.
> > 
> > It appears as if it would be possible to implement this for FreeBSD
> > too (see comment in code).
> > ---
> >  docs/nbdkit-plugin.pod  |  47 +++++++++++++++--
> >  include/nbdkit-common.h |   3 ++
> >  server/nbdkit.syms      |   3 ++
> >  server/public.c         | 108
++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 156 insertions(+), 5 deletions(-)
> > 
> 
> > +=head2 C<nbdkit_peer_pid>
> > +
> > +(nbdkit E<ge> 1.24)
> > +
> > + int nbdkit_peer_pid (void);
> > +
> > +Return the peer process ID.  This is only available when the client
> > +connected over a Unix domain socket, and only works for Linux.
> > +
> > +On success this returns the peer process ID.  On error,
> > +C<nbdkit_error> is called and this call returns C<-1>.
> 
> Is int always going to be sufficient?  Or are there platforms with
> 64-bit pid_t?  Mingw is an interesting beast; I've seen conflicting
> stories on whether 64-bit windows has 32- or 64-bit pids (the spawn APIs
> manage 64-bit handles, but other windows APIs return 32-bit int), so
> 64-bit pid_t on mingw does seem to be a real concern.
IIUC,  POSIX says  pid_t is a signed integer, but doesn't specify the
size.  Thus libvirt exposed  pid_t as  "signed long long" in our APIs
to be futureproof.
> > +
> > +=head2 C<nbdkit_peer_uid>
> > +
> > +(nbdkit E<ge> 1.24)
> > +
> > + int nbdkit_peer_uid (void);
> > +
> > +Return the peer user ID.  This is only available when the client
> > +connected over a Unix domain socket, and only works for Linux.
> > +
> > +On success this returns the user ID.  On error, C<nbdkit_error>
is
> > +called and this call returns C<-1>.
> > +
> > +=head2 C<nbdkit_peer_gid>
> > +
> > +(nbdkit E<ge> 1.24)
> > +
> > + int nbdkit_peer_gid (void);
> 
> int for these two is probably fine.
IIUC, gid_t/uid_t don't have their signed-ness specified by POSIX,
nor size, but you're required to cast negative values eg

    gid_t foo = (gid_t)-1;

based on this, libvirt chose to expose them as "unsigned long long" to
maximise future proofing.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Oct 05, 2020 at 02:38:37PM +0100, Daniel P. Berrangé
wrote:
> On Mon, Oct 05, 2020 at 08:21:50AM -0500, Eric Blake wrote:
> > On 10/3/20 1:50 PM, Richard W.M. Jones wrote:
> > > New nbdkit_peer_pid, nbdkit_peer_uid and nbdkit_peer_gid calls
can be
> > > used on Linux (only) to read the peer PID, UID and GID from
clients
> > > connected over a Unix domain socket.  This can be used in the
> > > preconnect phase to add additional filtering.
> > > 
> > > One use for this is to add an extra layer of authentication for
local
> > > connections.  A subsequent commit will enhance the now misnamed
> > > nbdkit-ip-filter to allow filtering on these extra fields.
> > > 
> > > It appears as if it would be possible to implement this for
FreeBSD
> > > too (see comment in code).
> > > ---
> > >  docs/nbdkit-plugin.pod  |  47 +++++++++++++++--
> > >  include/nbdkit-common.h |   3 ++
> > >  server/nbdkit.syms      |   3 ++
> > >  server/public.c         | 108
++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 156 insertions(+), 5 deletions(-)
> > > 
> > 
> > > +=head2 C<nbdkit_peer_pid>
> > > +
> > > +(nbdkit E<ge> 1.24)
> > > +
> > > + int nbdkit_peer_pid (void);
> > > +
> > > +Return the peer process ID.  This is only available when the
client
> > > +connected over a Unix domain socket, and only works for Linux.
> > > +
> > > +On success this returns the peer process ID.  On error,
> > > +C<nbdkit_error> is called and this call returns
C<-1>.
> > 
> > Is int always going to be sufficient?  Or are there platforms with
> > 64-bit pid_t?  Mingw is an interesting beast; I've seen
conflicting
> > stories on whether 64-bit windows has 32- or 64-bit pids (the spawn
APIs
> > manage 64-bit handles, but other windows APIs return 32-bit int), so
> > 64-bit pid_t on mingw does seem to be a real concern.
> 
> IIUC,  POSIX says  pid_t is a signed integer, but doesn't specify the
> size.  Thus libvirt exposed  pid_t as  "signed long long" in our
APIs
> to be futureproof.
> 
> > > +
> > > +=head2 C<nbdkit_peer_uid>
> > > +
> > > +(nbdkit E<ge> 1.24)
> > > +
> > > + int nbdkit_peer_uid (void);
> > > +
> > > +Return the peer user ID.  This is only available when the client
> > > +connected over a Unix domain socket, and only works for Linux.
> > > +
> > > +On success this returns the user ID.  On error,
C<nbdkit_error> is
> > > +called and this call returns C<-1>.
> > > +
> > > +=head2 C<nbdkit_peer_gid>
> > > +
> > > +(nbdkit E<ge> 1.24)
> > > +
> > > + int nbdkit_peer_gid (void);
> > 
> > int for these two is probably fine.
> 
> IIUC, gid_t/uid_t don't have their signed-ness specified by POSIX,
> nor size, but you're required to cast negative values eg
> 
>     gid_t foo = (gid_t)-1;
> 
> based on this, libvirt chose to expose them as "unsigned long
long" to
> maximise future proofing.
We need an in-band error indication.  I wonder if there are systems
with valid UID or GID == (uint64_t)-1 ?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org