Skip to content

fix(client/windows): put NRPT rules in a special spot if Group Policy is active#6472

Merged
ReactorScram merged 4 commits intomainfrom
reactorscram/fix-nrpt-gpo
Aug 28, 2024
Merged

fix(client/windows): put NRPT rules in a special spot if Group Policy is active#6472
ReactorScram merged 4 commits intomainfrom
reactorscram/fix-nrpt-gpo

Conversation

@ReactorScram
Copy link
Contributor

@ReactorScram ReactorScram commented Aug 28, 2024

Closes #6469

DNS deactivation now also uses the registry instead of PowerShell, but this may not be faster, since the latency would already be hidden from users most of the time.

@ReactorScram ReactorScram requested a review from jamilbk August 28, 2024 20:44
@ReactorScram ReactorScram self-assigned this Aug 28, 2024
@vercel
Copy link

vercel bot commented Aug 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 28, 2024 9:54pm

@ReactorScram ReactorScram changed the title fix(rust/gui-client/windows): put NRPT rules in a special spot if Group Policy is active fix(client/windows): put NRPT rules in a special spot if Group Policy is active Aug 28, 2024
@ReactorScram ReactorScram marked this pull request as ready for review August 28, 2024 20:48
Comment on lines 132 to 133
if group_policy_key_exists {
let (key, _) = hklm.create_subkey(group_nrpt_path().join(NRPT_REG_KEY))?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a TOCTOU gap here, if someone is taking the computer off of Group Policy right as we're activating DNS. Worth fixing before merging maybe, but not before testing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably such a low chance it might not be worth the added complexity. I think always having the local key set should mean that it will fall back to that if GPO is disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'll leave it alone for now then and just leave a TODO

Comment on lines 132 to 133
if group_policy_key_exists {
let (key, _) = hklm.create_subkey(group_nrpt_path().join(NRPT_REG_KEY))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably such a low chance it might not be worth the added complexity. I think always having the local key set should mean that it will fall back to that if GPO is disabled?

@ReactorScram ReactorScram enabled auto-merge August 28, 2024 21:53
@ReactorScram ReactorScram added this pull request to the merge queue Aug 28, 2024
Merged via the queue into main with commit ef75f0f Aug 28, 2024
@ReactorScram ReactorScram deleted the reactorscram/fix-nrpt-gpo branch August 28, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Domain-joined Windows hosts break Split DNS

2 participants