On Thu, Oct 20, 2022 at 16:59:07 -0500, Jonathon Jongsma wrote:
> Rather than passing passwords and cookies (which could contain
> passwords) to nbdkit via commandline arguments, use the alternate format
> that nbdkit supports where we can specify a file descriptor which nbdkit
> will read to get the password or cookies.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> build-aux/syntax-check.mk | 4 +-
> src/qemu/qemu_nbdkit.c | 64 ++++++++++++-----
> src/util/vircommand.c | 3 +-
> src/util/virutil.h | 2 +-
> .../disk-cdrom-network.args.disk0 | 7 ++
> .../disk-cdrom-network.args.disk1 | 9 +++
> .../disk-cdrom-network.args.disk1.pipe.1778 | 1 +
> .../disk-cdrom-network.args.disk2 | 9 +++
> .../disk-cdrom-network.args.disk2.pipe.1780 | 1 +
> .../disk-network-http.args.disk0 | 7 ++
> .../disk-network-http.args.disk1 | 6 ++
> .../disk-network-http.args.disk2 | 7 ++
> .../disk-network-http.args.disk2.pipe.1778 | 1 +
> .../disk-network-http.args.disk3 | 8 +++
> .../disk-network-http.args.disk3.pipe.1780 | 1 +
> ...work-source-curl-nbdkit-backing.args.disk0 | 8 +++
> ...e-curl-nbdkit-backing.args.disk0.pipe.1778 | 1 +
> .../disk-network-source-curl.args.disk0 | 8 +++
> ...k-network-source-curl.args.disk0.pipe.1778 | 1 +
> .../disk-network-source-curl.args.disk1 | 8 +++
> ...k-network-source-curl.args.disk1.pipe.1780 | 1 +
> .../disk-network-source-curl.args.disk2 | 8 +++
> ...k-network-source-curl.args.disk2.pipe.1782 | 1 +
> .../disk-network-source-curl.args.disk3 | 7 ++
> .../disk-network-source-curl.args.disk4 | 7 ++
> tests/qemunbdkittest.c | 69 +++++++++++++++++--
> 26 files changed, 219 insertions(+), 30 deletions(-)
> create mode 100644 tests/qemunbdkitdata/disk-cdrom-network.args.disk0
> create mode 100644 tests/qemunbdkitdata/disk-cdrom-network.args.disk1
> create mode 100644 tests/qemunbdkitdata/disk-cdrom-network.args.disk1.pipe.1778
> create mode 100644 tests/qemunbdkitdata/disk-cdrom-network.args.disk2
> create mode 100644 tests/qemunbdkitdata/disk-cdrom-network.args.disk2.pipe.1780
> create mode 100644 tests/qemunbdkitdata/disk-network-http.args.disk0
> create mode 100644 tests/qemunbdkitdata/disk-network-http.args.disk1
> create mode 100644 tests/qemunbdkitdata/disk-network-http.args.disk2
> create mode 100644 tests/qemunbdkitdata/disk-network-http.args.disk2.pipe.1778
> create mode 100644 tests/qemunbdkitdata/disk-network-http.args.disk3
> create mode 100644 tests/qemunbdkitdata/disk-network-http.args.disk3.pipe.1780
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl-nbdkit-backing.args.disk0
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl-nbdkit-backing.args.disk0.pipe.1778
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk0
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk0.pipe.1778
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk1
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk1.pipe.1780
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk2
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk2.pipe.1782
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk3
> create mode 100644 tests/qemunbdkitdata/disk-network-source-curl.args.disk4
>
> diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
> index 68cd9dff5f..d44b1e5b17 100644
> --- a/build-aux/syntax-check.mk
> +++ b/build-aux/syntax-check.mk
> @@ -1355,10 +1355,10 @@ exclude_file_name_regexp--sc_prohibit_strdup = \
> ^(docs/|examples/|tests/virnetserverclientmock.c|tests/commandhelper.c|tools/nss/libvirt_nss_(leases|macs)\.c$$)
>
> exclude_file_name_regexp--sc_prohibit_close = \
> - (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/vir(file|event)\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c)|tools/nss/libvirt_nss_(leases|macs)\.c)|tools/virt-qemu-qmp-proxy$$)
> + (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/vir(file|event)\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c|qemunbdkittest\.c)|tools/nss/libvirt_nss_(leases|macs)\.c)|tools/virt-qemu-qmp-proxy$$)
In other cases we simply close a non-existing FD, so this might not be
needed.
> @@ -729,6 +736,29 @@ qemuNbdkitStopStorageSource(virStorageSource *src)
> }
>
>
> +static int
> +qemuNbdkitCommandPassDataByPipe(virCommand *cmd,
> + const char *argName,
> + unsigned char *buf,
> + size_t buflen)
> +{
> + g_autofree char *fdfmt = NULL;
> + int fd = virCommandSetSendBuffer(cmd, buf, buflen);
This function now takes a unsigned char ** for 'buf' and clears it.
You'll need to adapt the prototype.
> +
> + if (fd < 0)
> + return -1;
> +
> + /* some nbdkit arguments accept a variation where nbdkit will read the data
> + * from a file descriptor, e.g. password=-FD */
> + fdfmt = g_strdup_printf("-%i", fd);
> + virCommandAddArgPair(cmd, argName, fdfmt);
> +
> + virCommandDoAsyncIO(cmd);
> +
> + return 0;
> +}
> +
> +
> static int
> qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
> virCommand *cmd)
> @@ -744,10 +774,10 @@ qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
>
> if (proc->source->auth) {
> g_autoptr(virConnect) conn = virGetConnectSecret();
> - g_autofree uint8_t *secret = NULL;
> + uint8_t *secret = NULL;
... and then you can keep this here because it will be cleared
approrpately.
> size_t secretlen = 0;
> - g_autofree char *password = NULL;
> int secrettype;
> + virStorageAuthDef *authdef = proc->source->auth;
>
> virCommandAddArgPair(cmd, "user",
> proc->source->auth->username);
> @@ -760,7 +790,7 @@ qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
> }
>
> if (virSecretGetSecretString(conn,
> - &proc->source->auth->seclookupdef,
> + &authdef->seclookupdef,
> secrettype,
> &secret,
> &secretlen) < 0) {
This hunk belongs to the commit adding this bit.
> @@ -769,24 +799,20 @@ qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
> return -1;
> }
>
> - /* ensure that the secret is a NULL-terminated string */
> - password = g_strndup((char*)secret, secretlen);
> -
> - /* for now, just report an error rather than passing the password in
> - * cleartext on the commandline */
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s",
> - "Password not yet supported for nbdkit sources");
> - return -1;
> + if (qemuNbdkitCommandPassDataByPipe(cmd, "password",
> + secret, secretlen) < 0)
> + return -1;
> }
>
> - if (proc->source->ncookies > 0) {
> - /* for now, just report an error rather than passing cookies in
> - * cleartext on the commandline */
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s",
> - "Cookies not yet supported for nbdkit sources");
> - return -1;
> + /* Create a pipe to send the cookies to the nbdkit process. */
> + if (proc->source->ncookies) {
> + char *cookies =
> + qemuBlockStorageSourceGetCookieString(proc->source);
no need for linebreak after = even when it exceeds line length slightly.
Also after changing to doulbe pointer you can add an autofree here too
to be consistent.
> +
> + if (qemuNbdkitCommandPassDataByPipe(cmd, "cookie",
> + (unsigned char*)cookies,
> + strlen(cookies)) < 0)
> + return -1;
> }
>
> if (proc->source->sslverify == VIR_TRISTATE_BOOL_NO) {
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 014bab9196..838eb6bd16 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -1703,7 +1703,8 @@ virCommandSetSendBuffer(virCommand *cmd,
> return -1;
> }
>
> - if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) < 0) {
> + if (!(dryRunBuffer || dryRunCallback) &&
> + fcntl(pipefd[1], F_SETFL, O_NONBLOCK) < 0) {
> cmd->has_error = errno;
> VIR_FORCE_CLOSE(pipefd[0]);
> VIR_FORCE_CLOSE(pipefd[1]);
This belongs to a separate commit with separate justification.
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index ab8511bf8d..094b399859 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -186,7 +186,7 @@ char *virGetPassword(void);
> *
> * Returns: -1 on error, 0 on success
> */
> -int virPipe(int fds[2]);
> +int virPipe(int fds[2]) G_NO_INLINE;
>
Same here.
> /*
> * virPipeQuiet:
> /* Some mock implementations for testing */
[...]
> +#define PIPE_FD_START 1777
> +static int mockpipefd = PIPE_FD_START;
> +int
> +virPipeQuiet(int fds[2])
> +{
> + fds[0] = mockpipefd++;
> + fds[1] = mockpipefd++;
> +
> + if (fcntl(fds[0], F_GETFD) != -1 ||
> + fcntl(fds[1], F_GETFD) != -1)
> + abort();
You can even create a real pipe and after this check use dup2() to move
it to the required FD numbers so that the test output will be stable and
you'll have real pipes to work with.
In fact I plan to eventually fix all other cases where we use fake FDs
to use real FDs moved to stable numbers.
In your case it will actually work well because you already account for
multiple instances of the object which is not the case elsewhere.
> +
> + return 0;
> +}
> +
> +static int (*real_close)(int fd);
> +static void
> +init_syms(void)
> +{
> + VIR_MOCK_REAL_INIT(close);
> +}
> +
> +int
> +close(int fd)
> +{
> + int ret;
In other cases we simply try to close() an invalid FD. I'd not bother
with mocking this.
> +
> + init_syms();
> +
> + if (fd >= PIPE_FD_START)
> + ret = 0;
> + else
> + ret = real_close(fd);
> +
> + return ret;
> +}
