Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
chrisbednarski opened this issue Aug 25, 2023 · 12 comments
Open
Assignees

Comments

@chrisbednarski
Copy link

chrisbednarski commented Aug 25, 2023

Feature request

  • complex firewall rules can be added and modified
  • all properties of INetFwRule interface can be modified
  • all properties of INetFwRule2 interface can be modified
  • all properties of INetFwRule3 interface can be modified
  • firewall rules can be created enabled or disabled
  • firewall rules can be nested under a ServiceInstall/ServiceConfig tags, so the service name attribute is automatically derived
  • backwards compatible with WiX4.0.1 as much as possible

Benefits:

  • complex firewall rules can be created and managed in a WiX installer

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

  • a custom build of the WiX firewall extension

Breaking changes:

  • removes Windows XP support
  • Name is no longer a formatted string

Non-breaking changes:

  • custom table name changed from Wix4FirewallException to Wix5FirewallException
@chrisbednarski
Copy link
Author

chrisbednarski commented Aug 25, 2023

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
Copy link
Author

chrisbednarski commented Aug 25, 2023

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
Copy link
Author

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

@barnson
Copy link
Member

barnson commented Aug 26, 2023

@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
Copy link
Author

chrisbednarski commented Aug 27, 2023

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
Copy link
Member

barnson commented Aug 27, 2023

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
Copy link
Author

chrisbednarski commented Aug 28, 2023

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
Copy link
Member

barnson commented Aug 28, 2023

Cool (x2)!

@chrisbednarski
Copy link
Author

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

  • Edge traversal property is turned on for application exceptions in V4. I would suggest keeping this off, unless explicitly added in. This is the default behavior of netsh advfirewall firewall add rule

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

  • All firewall rule properties are updated at runtime (based on data resolved from formatted columns). In V4, rules are re-enabled only when updated.

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
Copy link
Member

barnson commented Aug 30, 2023

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
Copy link
Member

barnson commented Sep 1, 2023

TRIAGE, please opine on the following:

  • If there are no behavior changes, can schema namespace stay the same between WiX v4 and v5?
  • If there are behavior changes, must schema namespace change between WiX v4 and v5? (i.e., require a trip through WixCop to adjust authoring?)
  • When custom table prefix must change (e.g., Wix4 to Wix5), generally the custom action prefix would change to match. Currently, there is a global extension CA prefix; should we change the global prefix for all CAs even though table names won't change (i.e., mix prefixes of tables and CAs) or change CA prefixes locally only? rename firewall extension table name to Wix5FirewallException wix#450 (comment)

@barnson
Copy link
Member

barnson commented Sep 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants