[libvirt] [PATCH v3 48/48] remote: pass identity across to newly opened daemons

[Daniel P. Berrangé]

On Tue, Jul 30, 2019 at 11:32:22AM +0200, Christophe de Dinechin wrote:
> 
> Daniel P. Berrangé writes:
> 
> > When opening a connection to a second driver inside the daemon, we must
> > ensure the identity of the current user is passed across. This allows
> > the second daemon to perform access control checks against the real end
> > users, instead of against the libvirt daemon that's proxying across the
> > API calls.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >  src/libvirt_remote.syms             |   1 +
> >  src/remote/remote_daemon_dispatch.c | 110 +++++++++++++++++++++++++---
> >  src/remote/remote_driver.c          |   1 +
> >  src/remote/remote_protocol.x        |  18 ++++-
> >  src/remote_protocol-structs         |   8 ++
> >  src/rpc/virnetserverclient.c        |  12 +++
> >  src/rpc/virnetserverclient.h        |   2 +
> >  7 files changed, 140 insertions(+), 12 deletions(-)
> >


> > +static int
> > +remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
> > +                                 virNetServerClientPtr client,
> > +                                 virNetMessagePtr msg ATTRIBUTE_UNUSED,
> > +                                 virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
> 
> Why ATTRIBUTE_UNUSED? Seems used in the cleanup?

copy+paste mistake


> > +                                 remote_connect_set_identity_args *args)
> > +{
> > +    virTypedParameterPtr params = NULL;
> > +    int nparams = 0;
> > +    int rv = -1;
> > +    virConnectPtr conn = remoteGetHypervisorConn(client);
> > +    virIdentityPtr ident = NULL;
> 
> (Trying to learn about coding style and conventions)
> Why is this not autounref here? Is there a convention that if you
> have explicit cleanup, you don't autounref?

autounref is our preferred modern style. I'm just not in the habit
well enough, especially when copying existnig code.

> 
> > +    if (!conn)
> > +        goto cleanup;
> > +
> > +    VIR_DEBUG("Received forwarded identity");
> > +    if (virTypedParamsDeserialize((virTypedParameterRemotePtr) args->params.params_val,
> > +                                  args->params.params_len,
> > +                                  REMOTE_CONNECT_IDENTITY_PARAMS_MAX,
> > +                                  &params,
> > +                                  &nparams) < 0)
> > +        goto cleanup;
> 
> Would it be useful to change the value rv over these cases,
> and if rv < 0, add a VIR_DEBUG with its value? Or is there
> sufficient debugging info from the individual calls already?

By convention in libvirt the return values are usually only ever
-1 or 0. We have only a few places with return '-errno' in the
code. So we don't need to report the return value most of the
time.

> 
> > +
> > +    VIR_TYPED_PARAMS_DEBUG(params, nparams);
> > +
> > +    if (virConnectSetIdentityEnsureACL(conn) < 0)
> > +        goto cleanup;
> > +
> > +    if (!(ident = virIdentityNew()))
> > +        goto cleanup;
> > +
> > +    if (virIdentitySetParameters(ident, params, nparams) < 0)
> > +        goto cleanup;
> > +
> > +    virNetServerClientSetIdentity(client, ident);
> > +
> > +    rv = 0;
> > +
> > + cleanup:
> > +    virTypedParamsFree(params, nparams);
> > +    virObjectUnref(ident);
> > +    if (rv < 0)
> > +        virNetMessageSaveError(rerr);
> > +    return rv;
> > +}
> > +
> > +
> > +
> >  static int
> >  remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED,
> >                                       virNetServerClientPtr client,

> > diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> > index a42b4a9671..05229f00c5 100644
> > --- a/src/remote_protocol-structs
> > +++ b/src/remote_protocol-structs
> > @@ -3105,6 +3105,13 @@ struct remote_domain_checkpoint_delete_args {
> >          remote_nonnull_domain_checkpoint checkpoint;
> >          u_int                      flags;
> >  };
> > +struct remote_connect_set_identity_args {
> > +        struct {
> > +                u_int              params_len;
> > +                remote_typed_param * params_val;
> 
> Indent by 8 spaces and try to align variables in the same file?
> Nothing good could come out of it ;-)

This particular file content has to match the auto-generated
output from the 'pdwtags' command. It is basically a sanity
check to catch people who mistakenly change something in the
remote protocol which would break ABI.

> 
> > +        } params;
> > +        u_int                      flags;
> > +};

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 :|