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