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

Add firewall extension decompiler, make firewall modifications work for msi changes #431

Merged
merged 2 commits into from
Nov 19, 2023

Conversation

chrisbednarski
Copy link
Contributor

@chrisbednarski chrisbednarski commented Jul 20, 2023

Add firewall extension decompiler.

MSI database schema changes:

  • made all wix table columns except for Direction nullable
  • Changed wix table columns from numbers to strings to support using properties.

Compiler changes:

  • the firewall element can be nested under ServiceInstall or util:ServiceConfig

  • Added OnUpdate property to the Wix firewall language which affect what happens during MSI modifications and repairs:
    EnableOnly - enable existing firewall rules, do not change any other properties. This is similar to how wix V4 firewall CA works
    DoNothing - do not update any properties of existing firewall rules
    default: re-assign all properties on existing firewall rules

  • Added columns for all firewall properties supported by Windows Vista/7/8+
    INetFwRule Windows Vista / Server 2008
    INetFwRule2 Windows 7 / Server 2008 R2
    INetFwRule3 Windows 8/ Server 2012
    The related COM interfaces are only queried when at least one of the properties of the firewall interface has been set by the wix compiler.

IE. If a firewall rule is created and none of the INetFwRule2 properties are set, the firewall rule can be deployed on Vista/7/8+ . Same with INetFwRule3 properties. If none are used, the firewall rule can be deployed on Vista/7/8+

When a INetFwRule2 property is added to the firewall rule, it will no longer work on Vista, only on 7/8+
This is handled by the feaAddINetFwRule2 and feaAddINetFwRule3 flags in the Attributes column.

@chrisbednarski
Copy link
Contributor Author

chrisbednarski commented Jul 20, 2023

Is it possible to unit test this ?

I added some changes to enable nesting firewall exceptions under ServiceInstall and util:ServiceConfig, but am struggling to find a way to test the second one.

Looks like the built in testing framework only allows testing a single extension at a time.

Would this have to be covered by an end to end test?

@robmen
Copy link
Member

robmen commented Jul 20, 2023

Would this have to be covered by an end to end test?

Unit tests can only validate that the correct data gets into the MSI. Integration (end-to-end) tests are required to execution of the CustomAction. There is such a test for some user scenarios.

@barnson
Copy link
Member

barnson commented Jul 20, 2023

Or just add support for more than one extension. I'm sure this isn't the only place it could be used.

@chrisbednarski
Copy link
Contributor Author

Or just add support for more than one extension. I'm sure this isn't the only place it could be used.

image

The firewall extension is referenced as a project in the WixToolsetTest.Firewall.
Making and using that change would imply adding WiXExtension.Util project to the Firewall.wixext solution

I don't know if that makes sense, or would be ok.

@barnson
Copy link
Member

barnson commented Jul 20, 2023

It wouldn't be the only one; NetFx depends on both Bal and Util.

@chrisbednarski
Copy link
Contributor Author

It wouldn't be the only one; NetFx depends on both Bal and Util.

??

I couldn't find any test projects referencing more than a single project they're testing.

image

@barnson
Copy link
Member

barnson commented Jul 23, 2023

Ah, the production extension itself has the dependencies.

@barnson
Copy link
Member

barnson commented Jul 23, 2023

It does use PackageReference, not ProjectReference, as I recall.

@chrisbednarski chrisbednarski force-pushed the feat/firewall-squashed branch from 9fb8816 to 3565918 Compare July 30, 2023 09:42
@chrisbednarski
Copy link
Contributor Author

Would this have to be covered by an end to end test?

Unit tests can only validate that the correct data gets into the MSI. Integration (end-to-end) tests are required to execution of the CustomAction. There is such a test for some user scenarios.

Where would be the best place to add integration tests for the firewall extension?

@barnson
Copy link
Member

barnson commented Jul 30, 2023

src\test\msi\WixToolsetTest.MsiE2E. The Util.wixext tests live there so Firewall.wixext tests should be neighbors.

@chrisbednarski
Copy link
Contributor Author

I will review this, add integration tests and rebase with the develop branch after Windows XP support has been removed from the firewall extension. Might be able to split this into multiple PRs so they're easier to review as well

@barnson
Copy link
Member

barnson commented Aug 25, 2023

Because you're adding columns to the custom table, this will need a bit of triage discussion: I think we need to change the Wix4 prefix to Wix5. Can you open a feature request issue about the features you want to add? That'll give me the opportunity to bring up the prefix (and mention for the historical record that XP support is dead-dead).

@chrisbednarski
Copy link
Contributor Author

No problem.

@chrisbednarski
Copy link
Contributor Author

Added more firewall attributes. inetfwrule1 and inetfwrule2 interface is all covered, but needs more E2E tests
inetfwrule3 still to come

@chrisbednarski chrisbednarski force-pushed the feat/firewall-squashed branch 3 times, most recently from 39820ef to c8641c6 Compare September 24, 2023 09:16
@chrisbednarski
Copy link
Contributor Author

Would you be able to approve the build workflow on this?
I'd like to see if all the tests pass in your test environment.

Specifically, I'm not sure how many network interfaces there will be on the build image.

@chrisbednarski
Copy link
Contributor Author

Woops! Let's try that again.

@chrisbednarski chrisbednarski force-pushed the feat/firewall-squashed branch 2 times, most recently from 3f12d6d to 51808f8 Compare September 29, 2023 04:56
@chrisbednarski
Copy link
Contributor Author

Added more compiler/decompiler tests. I think I'm pretty much done code wise.
The PR description is way out of date for the moment though.

What's the next step to help get this merged ?

@barnson
Copy link
Member

barnson commented Sep 29, 2023

At the moment, it's #451. I don't expect an immediate resolution, so if it's otherwise ready, you should look at building the v4 packages and checking them in.

@chrisbednarski
Copy link
Contributor Author

Is this what you mean?

@barnson
Copy link
Member

barnson commented Sep 29, 2023

Looks right (probably don't need the .wixpdb). If we end up supporting v4 and v5 together, we could go back to building them individually, but it's likely we'll have Data or Extensibility changes that will prevent it.

@chrisbednarski
Copy link
Contributor Author

chrisbednarski commented Oct 15, 2023

rebase + added E2E tests covering msi properties and OnChange flag.

I think there is enough test coverage in there now.
Let me know if I missed anything.

@barnson
Copy link
Member

barnson commented Oct 15, 2023

Ready for review? Any other PRs need updating?

@chrisbednarski
Copy link
Contributor Author

Yes, ready for review. I've closed off some redundant PRs and rebased #451, which might still be useful

@barnson
Copy link
Member

barnson commented Oct 23, 2023

Cool, thanks. I will gird my loins and jump in, probably next weekend.

@barnson
Copy link
Member

barnson commented Nov 19, 2023

@chrisbednarski, can you apply this patch? I don't have permission to push to your branch. Checks out locally but I want the PR CI build, which I can't trigger for some reason. I'm hoping another commit will work.

0001-Fix-typo-and-compiler-complaints.patch

@chrisbednarski
Copy link
Contributor Author

applied the patch

@chrisbednarski
Copy link
Contributor Author

and rebased with develop branch

@barnson barnson merged commit dfb7512 into wixtoolset:develop Nov 19, 2023
3 checks passed
@barnson
Copy link
Member

barnson commented Nov 19, 2023

Very cool! Thanks for contributing (and working through everything).

@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2023
@chrisbednarski chrisbednarski deleted the feat/firewall-squashed branch November 19, 2023 21:22
@barnson
Copy link
Member

barnson commented Nov 22, 2023

If you get very bored, the schema doc at https://github.com/wixtoolset/web/blob/master/src/xsd4/firewall.xsd could use doc on the new features.

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

Successfully merging this pull request may close these issues.

3 participants