Message ID | CAAo6VWPyQrjSp2qjvxqXRPS8=5N85i0-DJdkWhiGaXmND_8+qA@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, Feb 24, 2017 at 03:30:21PM -0800, ashish mittal wrote: > Thanks! > > I hope the following is in line with what you suggested - Yes, that looks suitable for password auth > > We will error out in case either of username, secret-id, or password > are missing. > > Good case, passing password via a file - > $ ./qemu-io --trace enable=vxhs* --object > secret,id=xvxhspasswd,file=/tmp/some/file/path -c 'read 66000 128k' > 'json:{"server.host": "127.0.0.1", "server.port": "9999", "vdisk-id": > "/test.raw", "driver": "vxhs", "user": "ashish", "password-secret": > "xvxhspasswd"}' > 1132@1487977829.151064:vxhs_open_vdiskid Opening vdisk-id /test.raw > > 1132@1487977829.151141:vxhs_get_creds User ashish, SecretID > xvxhspasswd, Password Str0ngP@ssw0rd <=== **** NOTE WILL NOT PRINT > PASSWORD IN FINAL CODE **** > > 1132@1487977829.151168:vxhs_open_hostinfo Adding host 127.0.0.1:9999 > to BDRVVXHSState > 1132@1487977829.173062:vxhs_get_vdisk_stat vDisk /test.raw stat ioctl > returned size 196616 > read 131072/131072 bytes at offset 66000 > 128 KiB, 1 ops; 0.0012 sec (99.049 MiB/sec and 792.3930 ops/sec) > 1132@1487977829.175141:vxhs_close Closing vdisk /test.raw > > > Bad case, missing user - > $ ./qemu-io --trace enable=vxhs* --object > secret,id=xvxhspasswd,data=/tmp/some/file/path -c 'read 66000 128k' > 'json:{"server.host": "127.0.0.1", "server.port": "9999", "vdisk-id": > "/test.raw", "driver": "vxhs"}' > 1310@1487978547.771234:vxhs_open_vdiskid Opening vdisk-id /test.raw > can't open device json:{"server.host": "127.0.0.1", "server.port": > "9999", "vdisk-id": "/test.raw", "driver": "vxhs"}: please specify the > user for authenticating to target > > diff --git a/block/vxhs.c b/block/vxhs.c > index 4f0633e..9b60ddf 100644 > --- a/block/vxhs.c > +++ b/block/vxhs.c > @@ -17,12 +17,16 @@ > #include "qemu/uri.h" > #include "qapi/error.h" > #include "qemu/uuid.h" > +#include "crypto/secret.h" > > #define VXHS_OPT_FILENAME "filename" > #define VXHS_OPT_VDISK_ID "vdisk-id" > #define VXHS_OPT_SERVER "server" > #define VXHS_OPT_HOST "host" > #define VXHS_OPT_PORT "port" > +#define VXHS_OPT_USER "user" > +#define VXHS_OPT_PASSWORD "password" > +#define VXHS_OPT_SECRETID "password-secret" > #define VXHS_UUID_DEF "12345678-1234-1234-1234-123456789012" > > QemuUUID qemu_uuid __attribute__ ((weak)); > @@ -136,6 +140,22 @@ static QemuOptsList runtime_opts = { > .type = QEMU_OPT_STRING, > .help = "UUID of the VxHS vdisk", > }, > + { > + .name = VXHS_OPT_USER, > + .type = QEMU_OPT_STRING, > + .help = "username for authentication to target", > + }, > + { > + .name = VXHS_OPT_PASSWORD, > + .type = QEMU_OPT_STRING, > + .help = "password for authentication to target", > + }, > + { > + .name = VXHS_OPT_SECRETID, > + .type = QEMU_OPT_STRING, > + .help = "ID of the secret providing password for" > + "authentication to target", > + }, > { /* end of list */ } > }, > }; > @@ -257,6 +277,9 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, > const char *server_host_opt; > char *str = NULL; > int ret = 0; > + const char *user = NULL; > + const char *secretid = NULL; > + const char *password = NULL; > > ret = vxhs_init_and_ref(); > if (ret < 0) { > @@ -320,6 +343,35 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, > goto out; > } > > + /* check if we got username and secretid via the options */ > + user = qemu_opt_get(opts, VXHS_OPT_USER); > + if (!user) { > + error_setg(&local_err, "please specify the user for authenticating to " > + "target"); > + qdict_del(backing_options, str); Not sure why you're deleting this ? Likewise the 2 cases below too > + ret = -EINVAL; > + goto out; > + } > + > + secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID); > + if (!secretid) { > + error_setg(&local_err, "please specify the ID of the secret to be " > + "used for authenticating to target"); > + qdict_del(backing_options, str); > + ret = -EINVAL; > + goto out; > + } > + > + /* check if we got password via the --object argument */ > + password = qcrypto_secret_lookup_as_utf8(secretid, &local_err); > + if (local_err != NULL) { > + trace_vxhs_get_creds(user, secretid, password); > + qdict_del(backing_options, str); > + ret = -EINVAL; > + goto out; > + } > + trace_vxhs_get_creds(user, secretid, password); > + > s->vdisk_hostinfo.host = g_strdup(server_host_opt); > > s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts, Regards, Daniel
On Mon, Feb 27, 2017 at 1:22 AM, Daniel P. Berrange <berrange@redhat.com> wrote: > On Fri, Feb 24, 2017 at 03:30:21PM -0800, ashish mittal wrote: >> Thanks! >> >> I hope the following is in line with what you suggested - > > Yes, that looks suitable for password auth > Thanks! >> >> We will error out in case either of username, secret-id, or password >> are missing. >> >> Good case, passing password via a file - >> $ ./qemu-io --trace enable=vxhs* --object >> secret,id=xvxhspasswd,file=/tmp/some/file/path -c 'read 66000 128k' >> 'json:{"server.host": "127.0.0.1", "server.port": "9999", "vdisk-id": >> "/test.raw", "driver": "vxhs", "user": "ashish", "password-secret": >> "xvxhspasswd"}' >> 1132@1487977829.151064:vxhs_open_vdiskid Opening vdisk-id /test.raw >> >> 1132@1487977829.151141:vxhs_get_creds User ashish, SecretID >> xvxhspasswd, Password Str0ngP@ssw0rd <=== **** NOTE WILL NOT PRINT >> PASSWORD IN FINAL CODE **** >> >> 1132@1487977829.151168:vxhs_open_hostinfo Adding host 127.0.0.1:9999 >> to BDRVVXHSState >> 1132@1487977829.173062:vxhs_get_vdisk_stat vDisk /test.raw stat ioctl >> returned size 196616 >> read 131072/131072 bytes at offset 66000 >> 128 KiB, 1 ops; 0.0012 sec (99.049 MiB/sec and 792.3930 ops/sec) >> 1132@1487977829.175141:vxhs_close Closing vdisk /test.raw >> >> >> Bad case, missing user - >> $ ./qemu-io --trace enable=vxhs* --object >> secret,id=xvxhspasswd,data=/tmp/some/file/path -c 'read 66000 128k' >> 'json:{"server.host": "127.0.0.1", "server.port": "9999", "vdisk-id": >> "/test.raw", "driver": "vxhs"}' >> 1310@1487978547.771234:vxhs_open_vdiskid Opening vdisk-id /test.raw >> can't open device json:{"server.host": "127.0.0.1", "server.port": >> "9999", "vdisk-id": "/test.raw", "driver": "vxhs"}: please specify the >> user for authenticating to target >> >> diff --git a/block/vxhs.c b/block/vxhs.c >> index 4f0633e..9b60ddf 100644 >> --- a/block/vxhs.c >> +++ b/block/vxhs.c >> @@ -17,12 +17,16 @@ >> #include "qemu/uri.h" >> #include "qapi/error.h" >> #include "qemu/uuid.h" >> +#include "crypto/secret.h" >> >> #define VXHS_OPT_FILENAME "filename" >> #define VXHS_OPT_VDISK_ID "vdisk-id" >> #define VXHS_OPT_SERVER "server" >> #define VXHS_OPT_HOST "host" >> #define VXHS_OPT_PORT "port" >> +#define VXHS_OPT_USER "user" >> +#define VXHS_OPT_PASSWORD "password" >> +#define VXHS_OPT_SECRETID "password-secret" >> #define VXHS_UUID_DEF "12345678-1234-1234-1234-123456789012" >> >> QemuUUID qemu_uuid __attribute__ ((weak)); >> @@ -136,6 +140,22 @@ static QemuOptsList runtime_opts = { >> .type = QEMU_OPT_STRING, >> .help = "UUID of the VxHS vdisk", >> }, >> + { >> + .name = VXHS_OPT_USER, >> + .type = QEMU_OPT_STRING, >> + .help = "username for authentication to target", >> + }, >> + { >> + .name = VXHS_OPT_PASSWORD, >> + .type = QEMU_OPT_STRING, >> + .help = "password for authentication to target", >> + }, >> + { >> + .name = VXHS_OPT_SECRETID, >> + .type = QEMU_OPT_STRING, >> + .help = "ID of the secret providing password for" >> + "authentication to target", >> + }, >> { /* end of list */ } >> }, >> }; >> @@ -257,6 +277,9 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, >> const char *server_host_opt; >> char *str = NULL; >> int ret = 0; >> + const char *user = NULL; >> + const char *secretid = NULL; >> + const char *password = NULL; >> >> ret = vxhs_init_and_ref(); >> if (ret < 0) { >> @@ -320,6 +343,35 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, >> goto out; >> } >> >> + /* check if we got username and secretid via the options */ >> + user = qemu_opt_get(opts, VXHS_OPT_USER); >> + if (!user) { >> + error_setg(&local_err, "please specify the user for authenticating to " >> + "target"); >> + qdict_del(backing_options, str); > > Not sure why you're deleting this ? Likewise the 2 cases below too > Picked it up from qemu_gluster_parse_json(). I will get rid of them in the final version. >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID); >> + if (!secretid) { >> + error_setg(&local_err, "please specify the ID of the secret to be " >> + "used for authenticating to target"); >> + qdict_del(backing_options, str); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + /* check if we got password via the --object argument */ >> + password = qcrypto_secret_lookup_as_utf8(secretid, &local_err); >> + if (local_err != NULL) { >> + trace_vxhs_get_creds(user, secretid, password); >> + qdict_del(backing_options, str); >> + ret = -EINVAL; >> + goto out; >> + } >> + trace_vxhs_get_creds(user, secretid, password); >> + >> s->vdisk_hostinfo.host = g_strdup(server_host_opt); >> >> s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts, > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| The next thing we need consensus on, is to decide exactly what additional information to pass. (1) Current security implementation in VxHS uses the SSL key and certificate files. Do you think we can achieve all the intended security goals if we pass these two files paths (and names?) from the command line? (2) If we are OK to continue with the present security scheme (using key and cert), then the only additional change needed might be to accept these files on the command line. (3) If we decide to rely on file permissions and SELinux policy to protect these key/cert files, then we don't need to pass the file names as a secret, instead, passing them as regular qemu_opt_get() options might be enough. Let us know what you think about the above? Thanks, Ashish
On Tue, Feb 28, 2017 at 02:51:39PM -0800, ashish mittal wrote: > On Mon, Feb 27, 2017 at 1:22 AM, Daniel P. Berrange <berrange@redhat.com> wrote: > >> + ret = -EINVAL; > >> + goto out; > >> + } > >> + > >> + secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID); > >> + if (!secretid) { > >> + error_setg(&local_err, "please specify the ID of the secret to be " > >> + "used for authenticating to target"); > >> + qdict_del(backing_options, str); > >> + ret = -EINVAL; > >> + goto out; > >> + } > >> + > >> + /* check if we got password via the --object argument */ > >> + password = qcrypto_secret_lookup_as_utf8(secretid, &local_err); > >> + if (local_err != NULL) { > >> + trace_vxhs_get_creds(user, secretid, password); > >> + qdict_del(backing_options, str); > >> + ret = -EINVAL; > >> + goto out; > >> + } > >> + trace_vxhs_get_creds(user, secretid, password); > >> + > >> s->vdisk_hostinfo.host = g_strdup(server_host_opt); > >> > >> s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts, > > The next thing we need consensus on, is to decide exactly what > additional information to pass. > > (1) Current security implementation in VxHS uses the SSL key and > certificate files. Do you think we can achieve all the intended > security goals if we pass these two files paths (and names?) from the > command line? Yes, that's how other parts of QEMU deal with SSL NB, QEMU needs to pass 3 paths to libqnoio - the client cert, the client key, and the certificate authority certlist > (2) If we are OK to continue with the present security scheme (using > key and cert), then the only additional change needed might be to > accept these files on the command line. Yep, I think that's the minimum required change to make this mergable. > (3) If we decide to rely on file permissions and SELinux policy to > protect these key/cert files, then we don't need to pass the file > names as a secret, instead, passing them as regular qemu_opt_get() > options might be enough. That's correct - you can assume file permissions protect the cert files. I would expect the syntax to work as follows $QEMU -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client \ -drive driver=vxhs,...other..opts...,tls-creds=tls0 When you see the 'tls-creds' flag, you can lookup the corresponding QCryptoTLSCredsX509 object and extract the 'dir' property from it. The include/crypto/tlscredsx509.h file has constants defined for the standard filenames - eg you would concatenate the directory with the constants QCRYPTO_TLS_CREDS_X509_CLIENT_KEY. This gives the filenames you can then pass to libqnio Regards, Daniel
diff --git a/block/vxhs.c b/block/vxhs.c index 4f0633e..9b60ddf 100644 --- a/block/vxhs.c +++ b/block/vxhs.c @@ -17,12 +17,16 @@ #include "qemu/uri.h" #include "qapi/error.h" #include "qemu/uuid.h" +#include "crypto/secret.h" #define VXHS_OPT_FILENAME "filename" #define VXHS_OPT_VDISK_ID "vdisk-id" #define VXHS_OPT_SERVER "server" #define VXHS_OPT_HOST "host" #define VXHS_OPT_PORT "port" +#define VXHS_OPT_USER "user" +#define VXHS_OPT_PASSWORD "password" +#define VXHS_OPT_SECRETID "password-secret" #define VXHS_UUID_DEF "12345678-1234-1234-1234-123456789012" QemuUUID qemu_uuid __attribute__ ((weak)); @@ -136,6 +140,22 @@ static QemuOptsList runtime_opts = { .type = QEMU_OPT_STRING, .help = "UUID of the VxHS vdisk", }, + { + .name = VXHS_OPT_USER, + .type = QEMU_OPT_STRING, + .help = "username for authentication to target", + }, + { + .name = VXHS_OPT_PASSWORD, + .type = QEMU_OPT_STRING, + .help = "password for authentication to target", + }, + { + .name = VXHS_OPT_SECRETID, + .type = QEMU_OPT_STRING, + .help = "ID of the secret providing password for" + "authentication to target", + }, { /* end of list */ } }, }; @@ -257,6 +277,9 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, const char *server_host_opt; char *str = NULL; int ret = 0; + const char *user = NULL; + const char *secretid = NULL; + const char *password = NULL; ret = vxhs_init_and_ref(); if (ret < 0) { @@ -320,6 +343,35 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, goto out; } + /* check if we got username and secretid via the options */ + user = qemu_opt_get(opts, VXHS_OPT_USER); + if (!user) { + error_setg(&local_err, "please specify the user for authenticating to " + "target"); + qdict_del(backing_options, str); + ret = -EINVAL; + goto out; + } + + secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID); + if (!secretid) { + error_setg(&local_err, "please specify the ID of the secret to be " + "used for authenticating to target"); + qdict_del(backing_options, str); + ret = -EINVAL; + goto out; + } + + /* check if we got password via the --object argument */ + password = qcrypto_secret_lookup_as_utf8(secretid, &local_err); + if (local_err != NULL) { + trace_vxhs_get_creds(user, secretid, password); + qdict_del(backing_options, str); + ret = -EINVAL; + goto out; + } + trace_vxhs_get_creds(user, secretid, password); + s->vdisk_hostinfo.host = g_strdup(server_host_opt); s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,