On 07/18/2017 11:11 PM, ashish mittal wrote: > On Mon, Jul 17, 2017 at 4:50 PM, John Ferlan <jferlan@xxxxxxxxxx> wrote: >> >> >> On 07/12/2017 06:09 PM, ashish mittal wrote: >>> On Fri, Jun 30, 2017 at 1:29 PM, John Ferlan <jferlan@xxxxxxxxxx> wrote: >>>> >>>> >>>> On 06/29/2017 10:02 PM, Ashish Mittal wrote: >>>>> From: Ashish Mittal <ashish.mittal@xxxxxxxxxxx> >>>>> >>>>> Add a new TLS X.509 certificate type - "vxhs". This will handle the >>>>> creation of a TLS certificate capability for properly configured >>>>> VxHS network block device clients. >>>>> >>>>> Signed-off-by: Ashish Mittal <ashish.mittal@xxxxxxxxxxx> >>>>> --- >>>>> Changelog: >>>>> >>>>> (1) Add two new options in /etc/libvirt/qemu.conf >>>>> to control TLS behavior with VxHS block devices >>>>> "vxhs_tls" and "vxhs_tls_x509_cert_dir". >>>>> (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable >>>>> TLS for VxHS block devices. >>>>> (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the >>>>> TLS certificates and keys are saved. If this value is missing, >>>>> the "default_tls_x509_cert_dir" will be used instead. >>>>> >>>>> src/qemu/libvirtd_qemu.aug | 4 ++++ >>>>> src/qemu/qemu.conf | 23 +++++++++++++++++++++++ >>>>> src/qemu/qemu_conf.c | 7 +++++++ >>>>> src/qemu/qemu_conf.h | 3 +++ >>>>> src/qemu/test_libvirtd_qemu.aug.in | 2 ++ >>>>> 5 files changed, 39 insertions(+) >>>>> >>>> >>>> FWIW: I have another patch upstream: >>>> >>>> https://www.redhat.com/archives/libvir-list/2017-June/msg01341.html >>>> >>>> Which is making some textual and code changes to qemu.conf and >>>> qemu_conf.c - just so you are aware. >>>> >>>> >>>>> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug >>>>> index e1983d1..c19bf3a 100644 >>>>> --- a/src/qemu/libvirtd_qemu.aug >>>>> +++ b/src/qemu/libvirtd_qemu.aug >>>>> @@ -115,6 +115,9 @@ module Libvirtd_qemu = >>>>> >>>>> let memory_entry = str_entry "memory_backing_dir" >>>>> >>>>> + let vxhs_entry = bool_entry "vxhs_tls" >>>>> + | str_entry "vxhs_tls_x509_cert_dir" >>>>> + >>>>> (* Each entry in the config is one of the following ... *) >>>>> let entry = default_tls_entry >>>>> | vnc_entry >>>>> @@ -133,6 +136,7 @@ module Libvirtd_qemu = >>>>> | nvram_entry >>>>> | gluster_debug_level_entry >>>>> | memory_entry >>>>> + | vxhs_entry >>>>> >>>>> let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] >>>>> let empty = [ label "#empty" . eol ] >>>>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf >>>>> index e6c0832..83c2377 100644 >>>>> --- a/src/qemu/qemu.conf >>>>> +++ b/src/qemu/qemu.conf >>>>> @@ -250,6 +250,29 @@ >>>>> #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000" >>>>> >>>>> >>>>> +# Enable use of TLS encryption on the VxHS network block devices. >>>>> +# >>>>> +# When the VxHS network block device server is set up appropriately, >>>>> +# x509 certificates are used for authentication between the clients >>>>> +# (qemu processes) and the remote VxHS server. >>>>> +# >>>>> +# It is necessary to setup CA and issue client and server certificates >>>>> +# before enabling this. >>>>> +# >>>>> +#vxhs_tls = 1 >>>>> + >>>>> + >>>>> +# In order to override the default TLS certificate location for VxHS >>>>> +# device TCP certificates, supply a valid path to the certificate directory. >>>>> +# This is used to authenticate the VxHS block device clients to the VxHS >>>>> +# server. >>>>> +# >>>>> +# If the provided path does not exist then the default_tls_x509_cert_dir >>>>> +# path will be used. >>>>> +# >>>>> +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs" >>>>> + >>>>> + >>>>> # In order to override the default TLS certificate location for migration >>>>> # certificates, supply a valid path to the certificate directory. If the >>>>> # provided path does not exist then the default_tls_x509_cert_dir path >>>> >>>> In patch 3, the call to qemuBuildTLSx509CommandLine has a @verifypeer >>>> parameter = true, thus it's always going to expect to verify the client >>>> rather than making that determination using *_tls_x509_verify. So that >>>> means you have to describe the environment as requiring the >>>> client-cert.pem and client-key.pem files (like the description for default). >>>> > > As you have rightly noted later, we are actually setting up the client > TLS env here (libvirt/qemu = vxhs client). I hope the default > libvirt/qemu TLS env can be used for setting up the client connection > also! If not, then I guess I just have to mandate a dedicated vxhs TLS > env. > > vxhs server TLS env setup would happen separately. So the @verifypeer > parameter = true in qemuBuildTLSx509CommandLine would apply to the > client connection and be ignored anyway (is true by default for the > client). If, on the vxhs server side, we were to set up the env with > @verifypeer parameter = false, then the client (libvirt/qemu) would > not need client-cert.pem and client-key.pem files. That said, we do > desire authentication of the client, and the client-cert.pem and > client-key.pem files would be needed in that case. I hope my > understanding is not terribly wrong. > After doing some more thinking about this, my initial assessment may have missed the mark. Even though you're doing libvirt/qemu/client to vxhs/server processing, you still are processing the QEMU command line arguments, in this case "-object tls-creds-x509" in order to fetch the certificate location (and whatever else that object supports). I could be wrong, but IIUC, QEMU will process those arguments generically and doesn't special case the VxHS environment to "ignore anything". I assume you know the QEMU VxHS command line processing better than I do though! I assume when you pass along the following command line option (from your example) that processing sees: -object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ and QEMU will look in @dir for specific files and will allow the QEMU VxHS code to utilize that certificate directory in order to authenticate to the VxHS server. If verify_peer is 'yes', then that means your properly set up VxHS server will want to do that client handshake (at least that's my understanding). The QEMU code doesn't "present" those certificates, but rather processes them. The presentation is done by the QEMU/VxHS client code using the object that was created (objvxhs_tls0). When the server certificate is presented if it was encrypted, but the QEMU command line processing didn't decrypt it, then the target server won't be able to read it. If it was encrypted, I assume the QEMU command line processing will see that 'passwordid' and get the secret to decrypt the certificate before you have to present it to your VxHS server. In any case, since you have the QEMU/VxHS environment available, it should be very easy for you to make that determination. That is what happens when "verify-peer" is "yes" and what happens when it's "no". Check your @dir - does it have the client*.pem files? If so, then the verification is going on. If not, then either the server isn't doing the verification or I'm missing something. I think the next step would be to test what happens under various scenarios that have come up during the review process: 1. TLS environment that doesn't expect passphrase and verify-peer=no (there's no client*.pem files). I assume you've been using this model for your testing) 2. Same as 1, but alter verify-peer=yes a. With no client*.pem files b. With client*.pem files 3. TLS environment that expects a passphrase a. Without providing an "-object secret" to decode it b. With an "-object secret" to decode it That would mean (at least) 5 separate directories that you can change the @dir argument to test with. There could be more test options, but those 5 came to mind quickly. Again, I suggest looking at Dan Berrange's blog for an example: https://www.berrange.com/posts/2016/04/01/improving-qemu-security-part-3-securely-passing-in-credentials/ You should be able to set up a very simple example that uses a clear text secret such as "-object secret,id=sec0,data=letmein" http://wiki.qemu.org/ChangeLog/2.6#Secret_passing_system >>>> Since you set the @addpasswordid to false on input, that just means the >>>> certificate server-key.pem file cannot be encrypted - something else >>>> that would need to be described. >>>> >>> >>> During the QEMU discussions on VxHS support for TLS, it was a agreed >>> that it was not necessary to encrypt the cert files. >>> >>>> Based on these two configuration settings, that means if vxhs_tls=1, >>>> then vxhs_tls_x509_cert_dir must be set to something other than the >>>> default environment. IOW: both must be defined and that would need to be >>>> validated during config read and cause failure (e.g. vxhsTLSx509certdir >>>> != defaultTLSx509certdir >>>> >>>> IOW: You're essentially *requiring* someone to set this up and don't >>>> handle the/a default environment. Hopefully this makes sense, it's >>>> Friday afternoon and I have a TLS migraine ;-). >>>> >>> >>> The ideal way is for VxHS to be able to use the default TLS >>> environment. These settings are for the VxHS client i.e. QEMU. VxHS >>> server would be running on a remote host and would have it's own >>> configuration settings. >>> >>> Could you please give me some pointers on what changes would be >>> required to reuse the default TLS environment? I am hoping these would >>> not necessitate any change in the qemu vxhs code. >>> >> >> The reality is - I don't believe you have to do anything other than >> describe if the default environment is using a password, then the >> consumer must create their own vxhs certificate environment that doesn't >> require a password/secret to decrypt the certificate. >> > > Thanks for your response. I think I understand this better now! > > Please correct me if the understanding below is not accurate - > > (1) the default TLS environment could potentially use encrypted TLS > certificates (I guess it's only the *key.pem files that would need to > be encrypted). Well, whomever configured the environment can set it up that way. That's part of the processing done outside of libvirt. Let's not overthink this... Perhaps rather than patch 2 having: + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + goto cleanup; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + goto cleanup; Let's change that to if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) goto cleanup; GET_CONFIG_TLS_CERTINFO(vhxs); What does this do? Well if the vxhs* specific parameters aren't set, the macro will use/copy the default* values. So if you modify virQEMUDriverConfig to add vxhsTLSx509secretUUID and vxhsTLSx509verify, then you can just use them. Let's take this a step further, if someone set up a default TLS environment with a password protected certificate, then they'd have to change the default_tls_x509_secret_uuid value to provide the secret UUID for that certificate. That's part of the "processing" required. So now, someone sets up a VxHS environment and wants to use that certificate. So, they define "vxhs_tls = 1" and let the defaults take over. That's it as long as the code is then modified to pass along the vxhsTLSx509verify and vxhsTLSx509SecretUUID when creating the tls-creds-x509 object. NB: If passphrases are used, then the qemuDomainSecretDiskPrepare may need some love to recognize that the VxHS disk would be creating a secret similar to what the similar Chardev routine does. OTOH: if VxHS wants to have it's own environment, someone sets it up, does all the certificate magic, points the vxhs_tls_x509_cert_dir at the environment. If the environment doesn't care about client verification, then verify-peer would be 0. If the environment doesn't care about secrets, then it doesn't uncomment that line (although I think there's a bug in the logic that sets the value to the default if the default exists - I sent a patch today on that). Anyway, if there are VxHS specific values, that doesn't change any other code - those values then get used. > (2) if it is indeed using encrypted TLS key.pem files, then it passes > along a secret password to decrypt the key files. This secret is > passed along to QEMU for actual processing. Therefore, if I wanted to > support secrets with vxhs, I would first have to change qemu code. After much typing and thinking about how QEMU command line probably processes things, I don't believe so. There's a lot of magic happening with the -object tls-creds-x509 and the 'passwordid' argument to reference an -object secret argument. There's lots of libvirt logic to handle creating the objects properly based on settings. There's quite a bit of code you can just copy or adjust to add VxHS processing, don't make it more complicated than it needs to be. > (3) VxHS does not accept secrets, therefore it cannot use the default > TLS env if it has been encrypted. This has nothing to do with secrets and VxHS - I think all this "magic" just happens for you. I could be wrong, but the only way to know is to actually test with the environment and the options. > (4) I guess, as you have pointed out, I would have to add code to > check if the default TLS env is using secrets, and fail the operation > if a dedicated VxHS env has not been provided in this scenario. > I'm merely pointing out you shouldn't attempt to use a default environment that may be configured in such a way that you don't pass/use the correct arguments to make all the magic happen. In that case, you'd have to direct someone to create their own VxHS TLS environment. The best I can say is to work through this logically and test, test, test. I don't believe you should add any code to check whether the default TLS env is being used. QEMU will let you know when you launch it that it cannot decode the server certificate if that's the problem! I really don't see this being much different than the ChardevTLS which does something very similar with respect to presenting certificates to some backend server. Migration is a bit "different" insomuch as it's two qemu processes (source/client and target/server), but it's still processing all values in the config file regardless of whether it's using the default or migration specific environment. At this point it's a matter of getting the code in place and testing. > >> Of course this is all predicated on the thought that the certificate >> being used is not validating libvirt<->qemu, rather its validating >> qemu<->vxhs-server. There's a bit of black magic that happens that I > > That is correct! For the VxHS block device, qemu is the client and the > vxhs block device server would be remote. We are setting up the qemu > client TLS environment via libvirt. The vxhs server TLS setup would be > done separately from within the vxhs server code. > Again, I assume that when passing along those -object's that QEMU will still process them. You just end up *using* the @id for the tls-creds-x509 in order to present that to your server (my assumption again). It's getting very difficult to follow along - we're now 6 levels deeps w/ replies and a few weeks later than when the patches were first posted. My short term memory has other things clogging it up ;-) John >> haven't dug into. I'm assuming that you'd be testing both a cert and >> non-cert environment... Add to your testing a 'what-if' someone provides >> a default environment *and* that environment required the secret. In >> that case, you should expect a failure. >> > > I will add try to add this test case. Thanks! > >>> Thanks, >>> Ashish >>> >>> >>>> John >>>> >>>>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c >>>>> index 73c33d6..f3813d4 100644 >>>>> --- a/src/qemu/qemu_conf.c >>>>> +++ b/src/qemu/qemu_conf.c >>>>> @@ -280,6 +280,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) >>>>> SET_TLS_X509_CERT_DEFAULT(spice); >>>>> SET_TLS_X509_CERT_DEFAULT(chardev); >>>>> SET_TLS_X509_CERT_DEFAULT(migrate); >>>>> + SET_TLS_X509_CERT_DEFAULT(vxhs); >>>>> >>>>> #undef SET_TLS_X509_CERT_DEFAULT >>>>> >>>>> @@ -395,6 +396,8 @@ static void virQEMUDriverConfigDispose(void *obj) >>>>> VIR_FREE(cfg->chardevTLSx509certdir); >>>>> VIR_FREE(cfg->chardevTLSx509secretUUID); >>>>> >>>>> + VIR_FREE(cfg->vxhsTLSx509certdir); >>>>> + >>>>> VIR_FREE(cfg->migrateTLSx509certdir); >>>>> VIR_FREE(cfg->migrateTLSx509secretUUID); >>>>> >>>>> @@ -533,6 +536,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, >>>>> goto cleanup; >>>>> if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) >>>>> goto cleanup; >>>>> + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) >>>>> + goto cleanup; >>>>> + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) >>>>> + goto cleanup; >>>>> >>>>> #define GET_CONFIG_TLS_CERTINFO(val) \ >>>>> do { \ >>>>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h >>>>> index 1407eef..96c0225 100644 >>>>> --- a/src/qemu/qemu_conf.h >>>>> +++ b/src/qemu/qemu_conf.h >>>>> @@ -201,6 +201,9 @@ struct _virQEMUDriverConfig { >>>>> unsigned int glusterDebugLevel; >>>>> >>>>> char *memoryBackingDir; >>>>> + >>>>> + bool vxhsTLS; >>>>> + char *vxhsTLSx509certdir; >>>>> }; >>>>> >>>>> /* Main driver state */ >>>>> diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in >>>>> index 3e317bc..dfe88f4 100644 >>>>> --- a/src/qemu/test_libvirtd_qemu.aug.in >>>>> +++ b/src/qemu/test_libvirtd_qemu.aug.in >>>>> @@ -25,6 +25,8 @@ module Test_libvirtd_qemu = >>>>> { "chardev_tls_x509_cert_dir" = "/etc/pki/libvirt-chardev" } >>>>> { "chardev_tls_x509_verify" = "1" } >>>>> { "chardev_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } >>>>> +{ "vxhs_tls" = "1" } >>>>> +{ "vxhs_tls_x509_cert_dir" = "/etc/pki/libvirt-vxhs" } >>>>> { "migrate_tls_x509_cert_dir" = "/etc/pki/libvirt-migrate" } >>>>> { "migrate_tls_x509_verify" = "1" } >>>>> { "migrate_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } >>>>> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
