wixtoolset / issues

WiX Toolset Issues Tracker
http://wixtoolset.org/
129 stars 36 forks source link

Firewall extension support for all Windows Vista+ firewall properties #7694

Open chrisbednarski opened 1 year ago

chrisbednarski commented 1 year ago

Feature request

Benefits:

Describe how you're accomplishing the feature today (if possible):

Breaking changes:

Non-breaking changes:

chrisbednarski commented 1 year ago

I've made the assumption (possibly incorrect) that if all columns added to the firewall extension table are optional, and all new firewall extension compiler properties are optional, the prefix can remain at WiX4 ??

Existing code (ie. wxs files) containing firewall exceptions should re-compile after updating to the WiX5 firewall extension without making any changes.

And the same assumption applies if changing an existing column from not nullable to nullable.

chrisbednarski commented 1 year ago

One thing I would like to get some guidance on:

The Name column in the Wix4FirewallException table is defined as a formatted string.

Microsoft recommends that firewall rule names should be unique, and if this guidance is followed, there is no issue.

However, firewall rules are looked up by their name for any repairs and modifications and this can cause issues when the unique name guidance is not adhered to.

When a package is repaired or modified, the firewall CA needs to be able to look up the old name, because the current name might be different. A new firewall rule would be created otherwise.

What's a good/preferred strategy to store a firewall rule name when the firewall rule is first created, or later modified? An additional column in the Wix4FirewallException table?

chrisbednarski commented 1 year ago

Also, what criteria do I use to select the correct modularizeType value for a column?

barnson commented 1 year ago

@chrisbednarski It's possible to mix a WiX v4 merge module with a WiX v5 MSI, so the table names probably have to change to avoid conflicts in that case. The other side is to decide on a policy: Do we want to bump the number quickly or only if it's vital?

If the name is formatted, you can't know what it will be until runtime, so to store the original formatted name on the system somehow. It's not documented to be formatted; as this is essentially an id, maybe it shouldn't be at all.

Modularization is for columns that hold ids/keys and need a modularization guid applied in merge modules. Some ids require special handling; ColumnModularizeType lists the special ones.

chrisbednarski commented 1 year ago

It's possible to mix a WiX v4 merge module with a WiX v5 MSI, so the table names probably have to change to avoid conflicts in that case. The other side is to decide on a policy: Do we want to bump the number quickly or only if it's vital?

If the table name is bumped to v5, will the namespace have to be changed from http://wixtoolset.org/schemas/v4/wxs/firewall to http://wixtoolset.org/schemas/v5/wxs/firewall as well?

I'm all for keeping things backwards compatible if possible. Especially for developers, who update to V5 and just try to rebuild their projects. One breaking change will obviously be the removal of Windows XP support. But that's more of a functional change, and will not affect the rebuild process as far as I can tell.

If the name is formatted, you can't know what it will be until runtime, so to store the original formatted name on the system somehow. It's not documented to be formatted; as this is essentially an id, maybe it shouldn't be at all.

Yes, that's exactly what I meant. The name has to be "crystalized" somewhere at runtime, each time a firewall rule is created or updated. So I'm not sure if keeping the crystalized firewall rule name in the Wix*FirewallException table, together with the rest of the properties would be ok, or is a smart idea in general. Does it make sense to update the Wix*FirewallException table at runtime?

Modularization is for columns that hold ids/keys and need a modularization guid applied in merge modules. Some ids require special handling; ColumnModularizeType lists the special ones.

As I'm understanding this, it's important to get the column modularization types right for the benefit of the next version. Is there any integration tests that test the merge of V3 into V4 (don't have to be related to the firewall) ? I haven't been involved with merge modules, but it sounds like it's worth covering this in a test.

barnson commented 1 year ago

If the table name is bumped to v5, will the namespace have to be changed from http://wixtoolset.org/schemas/v4/wxs/firewall to http://wixtoolset.org/schemas/v5/wxs/firewall as well?

That's a separate but related issue. So far, nothing planned for WiX v5 is a breaking language change, so we could keep the namespace the same. So far.

Does it make sense to update the Wix*FirewallException table at runtime?

It would -- but that's not possible. (You can add temporary rows only.) So you'd need to use the registry or file system and deal with all the associated hassles. I'm thinking it shouldn't be formatted...

As I'm understanding this, it's important to get the column modularization types right for the benefit of the next version.

Matters for the current version too. If it's an id, it's typically Column. The other values are special cases.

Is there any integration tests that test the merge of V3 into V4 (don't have to be related to the firewall) ? I haven't been involved with merge modules, but it sounds like it's worth covering this in a test.

Agreed! And I'm not aware of any. The current ModuleFixture builds the merge modules it uses. And that's why we've had extensions that don't work at first as merge modules... 🥇

chrisbednarski commented 1 year ago

It would -- but that's not possible. (You can add temporary rows only.) So you'd need to use the registry or file system and deal with all the associated hassles. I'm thinking it shouldn't be formatted...

OK.. I'll add that to the list of breaking changes and will remove the formatting from the name column.

And I think it will be wise to change the table name from Wix4FirewallException to Wix5FirewallException at this point then too.

barnson commented 1 year ago

Cool (x2)!

chrisbednarski commented 1 year ago

I have some suggestions, which would be breaking changes, that I'd like to get your input on.

This would be a breaking change if V4 firewall exception is simply re-compiled in V5 without adding EdgeTraversal property.

In my first PR I made this conditional, but I'd like to suggest this as the new default behavior. Hence the breaking change.

The exception to this would be the Enabled property, which will require some special considerations. eg. when a rule is created disabled by wix, an admin enables the rule, wix would not disable it again at runtime (unless repairing).

barnson commented 1 year ago

Breaking changes are definitely doable in a major version change. But typically, major version changes in WiX have meant a trip through Wixcop/wix convert where we can adjust the authoring to compensate for the change. So we'd need/want to change the schema namespace to "force" a wix convert run. So even if the rest of WiX v5 can use the v4 namespace, the firewall namespace would need to change.

barnson commented 1 year ago

TRIAGE, please opine on the following:

barnson commented 1 year ago

We should update the namespace if there are behavior changes (and that can be at the extension level). Likewise with prefixes: it's OK if they change per-extension.