[wix-devs] WIXBUG3704 - Allow access to persisted variables from related bundles

[Sean Hall]

We discussed this issue in
https://www.firegiant.com/blog/2020/10/15/wix-online-meeting-198-highlights/
(starts at 39:03). Most of that discussion was settling the
hidden+persisted issue (compiler error - done) and the encryption issue
(don't do it, pull the existing variable encryption - done). We agreed that
the registry would be a good place to persist the variables but kicked the
issue back to wix-devs to design how to store the types of the variables.
That never happened, so here we are. Jacob has a draft PR at
https://github.com/wixtoolset/burn/pull/56.

The current variant types are NONE, NUMERIC, STRING, FORMATTED, and
VERSION. I think we can map these to the registry types REG_NONE,
REG_QWORD, REG_SZ, REG_EXPAND_SZ, and REG_SZ, respectively. You might
notice that REG_SZ is used for both STRING and VERSION. The only difference
between these types is their comparison logic in conditions. When we
designed FORMATTED, we decided to keep the v3 API surface where the BA was
not able to ask what the current type of a variable is. So I think there's
no problem here. This would mean that all types are stored transparently
and easily consumable with minimal code. It also leaves REG_BINARY reserved
in case something changes in the future, like adding new variable types.

This does have implications on the APIs Jacob is adding to allow looking up
arbitrary variables for arbitrary bundles. There is currently a method in
those APIs that returns the type of the variable. It is currently returning
a DWORD but I think that it should be returning an enum. This brings up a
question I had about how to share an enum between the Burn headers and the
dutil headers, which don't have any relationship today (I don't remember
the context of that earlier discussion). I think we can sidestep that issue
again by either removing the ability to query the type, or simply return
the registry type. I think we should probably leave it in the dutil API but
remove it from the engine API. Either way, I think the engine API needs to
do the same coercion that it does for the normal variable get methods.

On Wed, Aug 5, 2020 at 5:31 PM Sean Hall <r.sean.hall at gmail.com> wrote:

> I don't like the binary blob. I don't think we could reasonably change the
> format later. Like triage said in the issue "This creates a very tight
> binding to the data file. We'd need to do this very carefully."
>
> On Wed, Aug 5, 2020 at 4:28 PM Hoover, Jacob via wix-devs <
> wix-devs at lists.wixtoolset.org> wrote:
>
>> I have to admit I’m not experienced with WIP’s, but I did attempt to do a
>> draft of this a while back
>> https://github.com/wixtoolset/web/blob/develop/src/Web/Static/documents/development/wips/3704-sharing-bundle-variables-across-bundle-versions.html.md
>>
>> I am hearing several points in this thread and am trying to gather them
>> into one list so hopefully we can discuss them today.
>>
>>
>>   1.  Don’t use a shared context, but instead migrate all variables to
>> the registry (No more rsm blob).
>>   2.  The ability to encrypt some or all the data is desirable
>>   3.  Non-hidden but persisted variables need to be accessible
>>
>> We could implement persistence in the registry, with all the hidden
>> variables being serialized into a Binary value, using the DPAPI to protect
>> them, and have all the persisted but not hidden variables be written as a
>> native type.  String -> REG_SZ, and try to coerce a numeric into either a
>> QWORD or a binary value.
>>
>> From: Bob Arnson <bob at firegiant.com>
>> Sent: Wednesday, August 5, 2020 4:36 PM
>> To: Edwin Castro <egcastr at gmail.com>; WiX Toolset Developer Mailing List
>> <wix-devs at lists.wixtoolset.org>
>> Cc: Hoover, Jacob <Jacob.Hoover at greenheck.com>; Rob Mensching <
>> rob at firegiant.com>
>> Subject: RE: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> I consider this an “all other things being equal” scenario. So, all other
>> things being equal, if we’re implementing a feature whose implementation
>> solves the designated problem and also – for minimal extra effort – solves
>> another problem, great. I wouldn’t go out of my way to do it, however, and
>> this scenario is one that could go either way: Today, variables are
>> persisted in a blob so implementing 3704 might require teasing that apart,
>> which would be useful to also allow “naked” access. But the blob contains
>> the variable types, which might be difficult to pull off without encoding
>> it somehow. It might be better to provide a butil API to read the blob,
>> even if we moved from using a file to a binary registry value.
>>
>> But this is why we require WIPs for this kind of change, so someone digs
>> deeper into the code and discusses the considerations and alternatives.
>>
>> From: Edwin Castro
>> Sent: Wednesday, 5 August, 2020 16:37
>> To: WiX Toolset Developer Mailing List
>> Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> As a bundle author, if I can persist non-hidden and hidden variables in
>> the registry, then I'd like to have the option of reading non-hidden
>> variables from the registry without needing to use butil to do so. Not all
>> applications can consume butil.
>>
>> Would it make sense to persist variables in an implementation dependent
>> way _and_ mirror non-hidden variables to a visible and findable location in
>> the registry? I'd support such a mirror being strictly "read-only" meaning
>> that any writes done "out of band" do not affect future bundle execution.
>> Effectively burn would ignore their current values and overwrite them with
>> the one-true-value from real storage.
>>
>> --
>> Edwin G. Castro
>>
>> On Wed, Aug 5, 2020 at 11:35 AM Bob Arnson via wix-devs wrote:
>> Meh. YAGNI. If we're writing individual persisted variables as individual
>> registry values, then obfuscation is just annoying to someone who needs the
>> data. If we're attempting to replace the state file with a binary blob in
>> the registry, then sure, encode and encrypt, why not?
>>
>> From: Hoover, Jacob
>> Sent: Wednesday, 5 August, 2020 14:10
>> To: WiX Toolset Developer Mailing List
>> Subject: RE: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> Right, but if we ever need to change the storage location/encryption in
>> the future, the API will isolate the consumer from the implementation. I
>> don't think we want to be painted into a corner with no ability to change
>> it. In addition, I would expect the butil API to handle the
>> encryption/decryption.
>>
>> From: Bob Arnson
>> Sent: Wednesday, August 5, 2020 12:58 PM
>> To: WiX Toolset Developer Mailing List
>> Subject: RE: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> That doesn't cover a lot of scenarios. If we're going to use the registry
>> without some kind of encoding (not encryption), blocking "plain" use of the
>> registry seems like overkill.
>>
>> From: Hoover, Jacob
>> Sent: Wednesday, 5 August, 2020 13:53
>> To: WiX Toolset Developer Mailing List
>> Subject: RE: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> No...  Don't look it up in the registry, use butil API to get to it.
>>
>> From: Bob Arnson
>> Sent: Wednesday, August 5, 2020 12:52 PM
>> To: WiX Toolset Developer Mailing List
>> Cc: Hoover, Jacob
>> Subject: RE: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> There's non-sensitive data that a bundle author might want to expose for
>> non-bundle use (e.g., "go ahead and look up Foo in the registry <here>."
>>
>> -----Original Message-----
>> From: wix-devs  On Behalf Of Hoover, Jacob via wix-devs
>> Sent: Wednesday, 5 August, 2020 13:44
>> To: Rob Mensching
>> Cc: Hoover, Jacob
>> Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> I guess from my standpoint, if we were to encrypt one variable, we might
>> as well encrypt "them" all. At least then it would be a single behavior for
>> storage and retrieval. Then the only thing you'd have to worry about is the
>> compiler warning when persisting if the variable is defined as hidden. Are
>> we only encrypting stings, or would we encrypt all variable types?
>>
>> From: Rob Mensching
>> Sent: Wednesday, August 5, 2020 12:39 PM
>> To: WiX Toolset Developer Mailing List
>> Cc: Hoover, Jacob
>> Subject: RE: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> 1. I don't completely disagree about the futility. However, storing
>> secrets in plain text is hard to defend. So, if we store them it seems
>> reasonable to at least do the minimum described in the Win32 documentation:
>> *
>> https://docs.microsoft.com/en-us/windows/win32/api/dpapi/nf-dpapi-cryptprotectdata
>> * https://docs.microsoft.com/en-us/windows/win32/secbp/handling-passwords
>>
>> 2. I think it would definitely be a good idea to have at least a warning
>> when Persisting Hidden variables. I could see someone argue that the
>> warning should be an error if the above doesn't sound secure enough to
>> allow anyone to store hidden variables easily.
>>
>>
>>
>> -----Original Message-----
>> From: wix-devs  On Behalf Of Hoover, Jacob via wix-devs
>> Sent: Wednesday, August 5, 2020 10:23 AM
>> To: WiX Toolset Developer Mailing List
>> Cc: Hoover, Jacob
>> Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> The question I have is what is the value of encrypting them? If were just
>> trying to deter the casual observer, then that's fine, but I would expect
>> butil to expose an API for reading variables. Are you suggesting that
>> hidden variables would be excluded from being shared? (I could see using
>> the bundle id as a seed for encrypting them, but even then it's only going
>> to protect from the casual observer as WIX is open source so a skilled
>> individual could always write the code to do the same as the bundle is
>> doing.)
>>
>>
>> From: wix-devs  On Behalf Of Bob Arnson via wix-devs
>> Sent: Wednesday, August 5, 2020 9:30 AM
>> To: WiX Toolset Developer Mailing List
>> Cc: Bob Arnson
>> Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> We could separate "hidden" from "secure" or "encryptedAtRest" in v2.
>>
>> -----Original Message-----
>> From: wix-devs On Behalf Of Rob Mensching via wix-devs
>> Sent: Wednesday, 5 August, 2020 10:25
>> To: WiX Toolset Developer Mailing List
>> Cc: Rob Mensching
>> Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> > If Windows Installer really doesn't protect Hidden variables with
>> encryption, then I'm not sure there's value in Burn doing the encryption
>> during runtime or at rest.
>>
>> Reasonable argument but persisted MSI Properties is an oft requested
>> feature (that's why my Remember Property Pattern blog post is very popular)
>> and doing it with secure variables requires more complicated machinery.
>>
>> I am of the opinion (and I could be convinced I'm wrong), that protecting
>> secrets in memory is overkill. There is always a point somewhere in the
>> process where the secret is not encrypted in memory (like being typed into
>> a password text box or sent to an external process). So, the goal should be
>> to protect the secret from non-authorized users. That's why I thought DAPI
>> would work because it prevents a hard drive from being stolen and
>> inspected. AFAIK, there is no protecting secrets from users on the machine
>> who run processes that operate on those secrets.
>>
>> Anyway, I totally understand if Persisted Hidden variables are not
>> encrypted in the first version of the feature.
>>
>> Also, I think it is a bit of a hassle for a BA to securely store Hidden
>> variables. Namely, it'd have to create a parallel set of persisted
>> variables that are updated when their non-secure variables are changed.
>> Doable, just a bit of a hassle.
>>
>>
>> -----Original Message-----
>> From: wix-devs  On Behalf Of Sean Hall via wix-devs
>> Sent: Sunday, August 2, 2020 4:48 PM
>> To: WiX Toolset Developer Mailing List
>> Cc: Sean Hall
>> Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted variables
>> from related bundles
>>
>> If Windows Installer really doesn't protect Hidden variables with
>> encryption, then I'm not sure there's value in Burn doing the encryption
>> during runtime or at rest. It would always need to be decrypted and sent to
>> an external process to actually be used which defeats the purpose. At which
>> point we should probably copy Windows Installer and say that Hidden only
>> protects it from being written to the log. If the BA wants it decrypted, it
>> can encrypt it and set a string variable to the base64 value.
>>
>> On Sat, Aug 1, 2020 at 1:08 PM Rob Mensching via wix-devs wrote:
>>
>> > > added a warning or something to Variables marked Hidden and Persisted.
>> >
>> > Or... should we investigate DAPI encrypting Hidden variables in the
>> > registry? Just throwing that out there if Jacob (or someone) wanted to
>> > noodle on it. I have not thought through the issues to the bottom.
>> >
>> > -----Original Message-----
>> > From: wix-devs
>> > Sent: Saturday, August 1, 2020 11:13 AM
>> > To: WiX Toolset Developer Mailing List
>> > Cc: Rob Mensching
>> > Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted
>> > variables from related bundles
>> >
>> > Quick historical note:
>> >
>> > > I feel like the interaction between Hidden and Persisted wasn't
>> > > fully thought through
>> >
>> > It wasn't that it wasn't thought through, the problem was just punted
>> > in the first version of Burn. Maybe that's the same thing but the
>> > intent was different. <smile/>
>> >
>> > In the end, we probably should have added a warning or something to
>> > Variables marked Hidden and Persisted. Maybe not too late to consider...
>> > especially if the values are going to be more obvious in the registry?
>> >
>> >
>> > -----Original Message-----
>> > From: wix-devs
>> > Sent: Wednesday, July 29, 2020 10:46 AM
>> > To: WiX Toolset Developer Mailing List
>> > Cc: Sean Hall
>> > Subject: Re: [wix-devs] WIXBUG3704 - Allow access to persisted
>> > variables from related bundles
>> >
>> > By "secure", you mean a Variable that is Hidden="yes"? The state file
>> > is just as secure as the registry. I feel like the interaction between
>> > Hidden and Persisted wasn't fully thought through, hence the issue I
>> > created -
>> > https://github.com/wixtoolset/issues/issues/4862. I've been following
>> the .NET Core
>> > discussion about obsoleting SecureString (
>> > https://github.com/dotnet/designs/pull/147) and I'm wondering if
>> there's actually value in the
>> > engine encrypting Hidden variables. I had hoped that MSI was also
>> > trying to be careful with Hidden properties, but I think the WiX
>> documentation is right - it only hides it from being written to the log.
>> >
>> > I didn't see anything in your POC that removed the "shared" variables
>> > from the state file.
>> >
>> > I'm suggesting that "shared" is unnecessary, and that all
>> Persisted="yes"
>> > variables should be stored in the registry instead of the state file.
>> >
>> > On Wed, Jul 29, 2020 at 11:07 AM Hoover, Jacob
>> > wrote:
>> >
>> > > I was trying to find a happy medium. As the registry isn't a "secure"
>> > > location, I was envisioning that it would be limited use so the
>> > > author could explicitly define what they wanted to share. Going from
>> > > memory, I don't think my POC implementation stores in both locations.
>> > >
>> > >
>> > >
>> > > Are you suggesting of eliminating local storage of variables and
>> > > place them all in the registry?
>> > >
>> > >
>> > >
>> > > I'll work on a WIP as soon as I can garner enough feedback to at
>> > > least ensure I am not totally off base.
>> > >
>> > >
>> > >
>> > > *From:* wix-devs  *On
>> > > Behalf Of *Sean Hall via wix-devs
>> > > *Sent:* Tuesday, July 28, 2020 6:46 PM
>> > > *To:* WiX Toolset Developer Mailing List
>> > > >
>> > > *Cc:* Sean Hall
>> > > *Subject:* Re: [wix-devs] WIXBUG3704 - Allow access to persisted
>> > > variables from related bundles
>> > >
>> > >
>> > >
>> > > I think it's a good idea to persist the variables in the registry. I
>> > > think there should be a single source of truth so I don't think they
>> > > should also be persisted to the state file.
>> > >
>> > > I'm having trouble coming up with a use case for persisting a
>> > > variable for the bundle but not for related bundles. Why shouldn't
>> > > all persisted variables be "shared"?
>> > >
>> > > Please write a WIP.
>> > >
>> > > On Fri, Jul 24, 2020 at 10:14 AM Hoover, Jacob via wix-devs  wrote:
>> > >
>> > > > https://github.com/wixtoolset/issues/issues/3704
>> > > >
>> > > > I haven't done my searching yet, but this is related to some work
>> > > > I had done internally. My thought was instead of allowing access
>> > > > to persisted variables, to allow for "shared" variables which
>> > > > would be written to the registry. In addition to not needing to be
>> > > > concerned with format/version and/or invoking the other bundle to
>> > > > ask it for data, we could simply
>> > > allow
>> > > > for shared variables to be accessed via the registry.
>> > > >
>> > > > I believe this was my rough in of the POC:
>> > > >
>> > > https://github.com/wixtoolset/wix3/compare/develop...jchoover:WIXFEA
>> T3704
>> > > >
>> > > > Thoughts?
>> > > >
>> > > > Thanks,
>> > > > Jacob
>>
>