4:46 a.m.
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(a)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,
> + ¶ms,
> + &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 :|