[libvirt] [PATCH 2/4] qemu: Introduce qemuBuildSecretObjectProps

[John Ferlan]

On 06/01/2016 06:43 AM, Peter Krempa wrote:
> On Tue, May 31, 2016 at 12:47:52 -0400, John Ferlan wrote:
>> On 05/31/2016 10:10 AM, Peter Krempa wrote:
>>> On Tue, May 31, 2016 at 09:49:45 -0400, John Ferlan wrote:
>>>> On 05/31/2016 08:10 AM, Peter Krempa wrote:
> 
> [...]
> 
>>>> >From that same 2.6 wiki pointer, you'll note that the qemu-img format
>>>> for luks will utilize "--object secret..."...  We currently have no way
>>>> to have a secret luks could use, but that's what I'm working towards. I
>>>> have also found through experimentation that creating an AES/secinfo
>>>
>>> That sounds like a bug to me if it doesn't work.
>>>
>>
>> Perhaps - only recently discovered though... have yet to determine
>> whether it was an expected way to provide the secret...
> 
> I'd say it makes sense to provide a secret that way. Especially since
> you need it for disk hotplug of LUKS volumes where you need to
> instantiate the secret objects via the monitor and in case of migration
> then via the command line. I don't think it's desired to use unencrypted
> secrets on the monitor though since it would require to use different
> code to create them.
> 

Turned out to be a fat-finger in my setup - Dan B pointed it out to me.
Difference between a "," and an "=" in a multi-line command which
resulted in the obtuse error message "qemu-img: Parameter 'qom-type' is
missing" - mystery solved.

>>>> like secret won't work. So the options are "raw" text, base64 encoded
>>>> raw text, or a file (with raw or base64 encoded text password contained).
>>>
>>> As said, first two are a non-option since it discloses the passphrase.
>>> File is an option but it requires setup to put it on disk with correct
>>> permissions and such which basically re-implemets the same thing we do
>>> for adding the master key. So basically it's desired to be able to do
>>> AES encrypted secrets for the LUKS key too.
>>>
>>
>> True, "file" is essentially the methodology used for the domain master
>> key.... And furthermore qemuBuildMasterKeyCommandLine could be modified
>> to actually call this common routine rather than inline building the
>> "object secret,id=$ALIAS,format=raw,file=$FILE". In the case of the
>> master key - the contents of $FILE are the 32 bytes of random data.
>>
>> I would think the storage_backend.c code should use the same "method" as
>> the qemu-kvm code in order to build the secret objects, don't you think?
>>  It obviously cannot share the domain master key, it would have to
>> generate it's own. Trying to taking a logical approach to getting there
> 
> Thats more due to the fact that storage volumes are not inherently tied
> to any domain so there isn't anything you could reuse.
> 

Right - I know that. That wasn't the question. What I was pointing out
is that wouldn't it be better to have one code path generating that
secret object than multiple?

If the common code isn't available, then there will be three different
virJSONValueObjectCreate calls (and four if a change was made to
qemuBuildMasterKeyCommandLine to use virJSONValueObjectCreate instead of
open coding as is done now).

I guess I just assumed wrongly that it would be desirable to have one
place to set things up.  Easily changed and less patches.

>> though.
>>
>> Regardless of the AES/luks anomaly I found, the purpose was to have a
>> common way to generate the same format whether being built for
>> qemu_command.c for qemu-kvm or storage_backend.c for qemu-img.
>>
>> I can agree that "secret,id=$alias,data=$data" is unwanted from the
>> viewpoint of disclosing the raw secret on the command line; however,
>> it's still possible to create 32 random bytes and encode it and pass as
>> "data=$data,format=base64" (the example I've taken from the qemu code):
> 
> That depens on what you are trying to use the random bits for. If it's
> for generating encryption keys they need to be treated the same as
> passwords since the encryption keys are directly derived from them.
> 

The storage/qemu-img key would be shorter lived than the domain key. It
would be needed only to encrypt/encode the secret for the "qemu-img
create -f luks..." command.

Later when the domain code goes to open the luks file, I would think it
would use the domain master key in order to encrypt/encode the secret
when it creates the qemu-kvm command (that part of the code isn't
written yet - went with create first).

>> openssl rand -base64 32 > key.b64
> 
> Erm. How exactly is that not creating a temporary file?
> 
>> KEY=$(base64 -d key.b64 | hexdump  -v -e '/1 "%02X"')
> 
> semi-granted. environment variables are in fact kept somewhat secret
> using unix permissions. (/proc/$PID/environ/ has 600 perms) but not
> using selinux.
> 
> Also as a side effect, environment may be leaked to child processes
> unless explicitly sanitized, whereas memory is by default sanitized.
> 

I was hoping you would "C" beyond the typed words... There is no need to
create a file nor is there a need to create environment variables, but
it's a lot easier to "show" than a code example. Although true that
passing an encoded key on the command line via the ",data=" parameter is
less desirable than the ",file=" option.

>> So that one doesn't have to deal with the on disk file permissions.
>>
>> It's simple enough to ensure that "raw" data isn't accepted... It's also
>> possible to ensure that someone providing a base64 encoded value to be
>> used for a master key would provide something valid.
> 
> Libvirt shall generate the master secret so why do you need to check
> it?
> 

True - it was an extrapolation of the semantic "someone" at least with
respect to the "common" code when building the object. IOW: in that
common secret object building helper.  Moot point now that I'll dump the
helper.

>> Similarly for the contents of a file, it's simple enough to check that
>> the length of data is proper for either raw or base64 encoding, if
>> that's desired. The whole file permissions wasn't as clear to me whether
>> the qemu-img needs to follow at least w/r/t security manager change made
>> in order to allow the domain secret key file to be used.
> 
> Why shouldn't it. As long as qemu-img supports the same infrastructure
> for the secret objects then we should treat both equally including the
> master key and commandline secret object instantiation.
> 

My quandary is less about the qemu-img infrastructure than the storage
driver code.

That is, it's "less clear" in my mind how the storage_backend.c code
would need to handle a ",file=" for its short lived master key. Where to
create the file?  What issues around permissions will there be?
Basically anything that virSecurityManagerDomainSetPathLabel handles for
the domain master key. I've been working under the assumption that it'll
need to be done, but hadn't quite got that far yet.

In a way I was hoping that the ",data=" option could have been used, but
that leaves a base64 encoded master key on the command line along with
the base64 encoded secret and iv, which yes, would allow someone
sufficiently privileged enough to read any logs the ability to decipher
the secret.

FWIW: The current "working" plan is a command such as:

qemu-img create -f luks \
--object secret,id=m0,file=$FILE \
--object secret,id=s0,data=$SECRET,keyid=m0,iv=$IV,format=base64 \
-o key-secret=s0 $LUKSFILE $SIZE

Thanks for the feedback so far...  trying to make sure I don't get too
far off into the weeds and the dog just keeps looking at me like I'm
crazy when I ask her.

John