From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39554) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afpVW-0003pF-VL for qemu-devel@nongnu.org; Tue, 15 Mar 2016 09:59:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afpVT-00041g-NY for qemu-devel@nongnu.org; Tue, 15 Mar 2016 09:59:34 -0400 References: <1456747261-22032-1-git-send-email-berrange@redhat.com> <1456747261-22032-20-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56E81537.5020005@redhat.com> Date: Tue, 15 Mar 2016 07:59:19 -0600 MIME-Version: 1.0 In-Reply-To: <1456747261-22032-20-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="BwPvN0TWE8IS8sQtBCbo4rdIMRjsK7qpV" Subject: Re: [Qemu-devel] [PATCH v4 19/26] block: add generic full disk encryption driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Fam Zheng , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --BwPvN0TWE8IS8sQtBCbo4rdIMRjsK7qpV Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/29/2016 05:00 AM, Daniel P. Berrange wrote: > Add a block driver that is capable of supporting any full disk > encryption format. This utilizes the previously added block > encryption code, and at this time supports the LUKS format. >=20 > The driver code is capable of supporting any format supported > by the QCryptoBlock module, so it registers one block driver > for each format. >=20 > At this time, the "luks" driver is registered. New LUKS > compatible volume can be formatted using qemu-img with > defaults for all settings. Sounds a bit redundant between the 1st and 3rd paragraph; but I'm not sure if I have any suggestions on shortening it for only a single mention that LUKS is the first (and so far only) supported driver. >=20 > $ qemu-img create --object secret,data=3D123456,id=3Dsec0 \ > -f luks -o key-secret=3Dsec0 demo.luks 10G >=20 > Alternatively the cryptographic settings can explicitly > set >=20 > $ qemu-img create --object secret,data=3D123456,id=3Dsec0 \ > -f luks -o key-secret=3Dsec0,cipher-alg=3Daes-256,\ > cipher-mode=3Dcbc,ivgen-alg=3Dplain64,hash-alg=3Dsha25= 6 \ > demo.luks 10G >=20 > And query its size >=20 > $ qemu-img info demo.img > image: demo.img > file format: luks > virtual size: 10G (10737418240 bytes) > disk size: 132K > encrypted: yes >=20 > Note that it was not neccessary to provide the password s/neccessary/necessary/ > when querying info for the volume. The password is only > required when performing I/O on the volume >=20 > All volumes created by this new 'luks' driver should be > capable of being opened by the kernel dm-crypt driver. >=20 > The only algorithms listed in the LUKS spec that are > not currently supported by this impl are sha512 and > ripemd160 hashes and cast6 cipher. >=20 > A new I/O test is added which is used to validate the > interoperability of the QEMU implementation of LUKS, > with the dm-crypt/cryptsetup implementation. Due to > the need to run cryptsetup as root, this test requires > that the user have password-less sudo configured. and therefore, the test better not be run by default, but by an explicit target. Worth spelling out what that target is? [1] >=20 > Signed-off-by: Daniel P. Berrange > --- > + > +static QCryptoBlockOpenOptions * > +block_crypto_open_opts_init(QCryptoBlockFormat format, > + QemuOpts *opts, > + Error **errp) > +{ > + OptsVisitor *ov; > + QCryptoBlockOpenOptions *ret; > + QCryptoBlockOptionsLUKS *tmp; > + Error *local_err =3D NULL; > + > + ret =3D g_new0(QCryptoBlockOpenOptions, 1); > + ret->format =3D format; > + > + ov =3D opts_visitor_new(opts); > + > + switch (format) { > + case Q_CRYPTO_BLOCK_FORMAT_LUKS: > + visit_type_QCryptoBlockOptionsLUKS(opts_get_visitor(ov), "luks= ", > + &tmp, &local_err); > + if (!local_err) { > + memcpy(&ret->u.luks, tmp, sizeof(*tmp)); > + g_free(tmp); > + } Commit 4d91e911 has landed; you should be using visit_type_QCryptoBlockOptionsLUKS_members() here rather than tmp and memcpy(). > + break; > + > + default: > + error_setg(&local_err, "Unsupported block format %d", format);= > + break; > + } > + > + if (local_err) { > + error_propagate(errp, local_err); > + opts_visitor_cleanup(ov); > + qapi_free_QCryptoBlockOpenOptions(ret); > + return NULL; > + } > + > + opts_visitor_cleanup(ov); > + return ret; > +} > + > + > +static QCryptoBlockCreateOptions * > +block_crypto_create_opts_init(QCryptoBlockFormat format, > + QemuOpts *opts, > + Error **errp) > +{ > + OptsVisitor *ov; > + QCryptoBlockCreateOptions *ret; > + QCryptoBlockCreateOptionsLUKS *tmp; > + Error *local_err =3D NULL; > + > + ret =3D g_new0(QCryptoBlockCreateOptions, 1); > + ret->format =3D format; > + > + ov =3D opts_visitor_new(opts); > + > + switch (format) { > + case Q_CRYPTO_BLOCK_FORMAT_LUKS: > + visit_type_QCryptoBlockCreateOptionsLUKS(opts_get_visitor(ov),= "luks", > + &tmp, &local_err); > + if (!local_err) { > + memcpy(&ret->u.luks, tmp, sizeof(*tmp)); > + g_free(tmp); And again. > +static int block_crypto_open_generic(QCryptoBlockFormat format, > + QemuOptsList *opts_spec, > + BlockDriverState *bs, > + QDict *options, > + int flags, > + Error **errp) > +{ > + > + bs->encrypted =3D 1; > + bs->valid_key =3D 1; These look like they should be bool; but then again, I think you are getting rid of them later in the series, so it's not worth the churn. > +++ b/qapi/block-core.json > @@ -242,11 +242,12 @@ > # @drv: the name of the block format used to open the backing device. = As of > # 0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg'= , > # 'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', > -# 'http', 'https', 'nbd', 'parallels', 'qcow', > +# 'http', 'https', 'luks', 'nbd', 'parallels', 'qcow', > # 'qcow2', 'raw', 'tftp', 'vdi', 'vmdk', 'vpc', 'vvfat' > # 2.2: 'archipelago' added, 'cow' dropped > # 2.3: 'host_floppy' deprecated > # 2.5: 'host_floppy' dropped > +# 2.6: 'luks' added Nice - you remembered docs. > +++ b/tests/qemu-iotests/146 > @@ -0,0 +1,521 @@ > + > +def verify_passwordless_sudo(): > + """Check whether sudo is configured to allow > + password-less access to commands""" > + > + args =3D ["sudo", "-n", "/bin/true"] > + > + proc =3D subprocess.Popen(args, > + stdin=3Dsubprocess.PIPE, > + stdout=3Dsubprocess.PIPE, > + stderr=3Dsubprocess.STDOUT) > + > + msg =3D proc.communicate()[0] > + > + if proc.returncode !=3D 0: > + iotests.notrun('requires password-less sudo access: %s' % msg)= [1] Okay, the check is part of qemu-iotests (not run as part of 'make check', but independently run), and looks like this should do the job of making it obvious whether the test was skipped due to setup reasons. Overall looks like a nice test. The changes above about using the more efficient _members() visit could be done as a followup if desired, so I'm okay if you fix the commit message typo and add: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --BwPvN0TWE8IS8sQtBCbo4rdIMRjsK7qpV Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJW6BU4AAoJEKeha0olJ0NqCNYH/0+ibZH+cguCe1HS92PYujOa MgXAVZt4EU/NGgzAGel9/ywMOoUJ0Ce41pfwn+FoXyEm6rgoISRO1nPz0UwVNybe clNLLt/fE1yjeFTsbEyB2ZMDShZhnVx5YJl8ohaCnQlFAl5S/gmhZGed1att5ZmP 9FMETmshbekSIZpPChiliaYRJvxOPYLYdZKXUR7ZKw3dXtxQJb2CIUd2LjpK8ZMW xFVGvCjy3uOZrzeIvVKqZfx366ZwqqUqF5CxsEc9VFSmw7n9RLnU+0FMADXfZTjf RnruShGsPiQSdl7/QC9YjrUqQcNl1Hfw3IeuTGIQFYUtODvamV9xcyfUuF1QsSw= =0gbH -----END PGP SIGNATURE----- --BwPvN0TWE8IS8sQtBCbo4rdIMRjsK7qpV--