Skip to content

NM-278: skip autoupdate evaluation on join#3922

Merged
abhishek9686 merged 1 commit intodevelopfrom
NM-278
Mar 19, 2026
Merged

NM-278: skip autoupdate evaluation on join#3922
abhishek9686 merged 1 commit intodevelopfrom
NM-278

Conversation

@abhishek9686
Copy link
Member

Describe your changes

Provide Issue ticket number if applicable/not in title

Provide testing steps

Checklist before requesting a review

  • My changes affect only 10 files or less.
  • I have performed a self-review of my code and tested it.
  • If it is a new feature, I have added thorough tests, my code is <= 1450 lines.
  • If it is a bugfix, my code is <= 200 lines.
  • My functions are <= 80 lines.
  • I have had my code reviewed by a peer.
  • My unit tests pass locally.
  • Netmaker is awesome.

@tenki-reviewer
Copy link

tenki-reviewer bot commented Mar 18, 2026

Tenki Code Review - Complete

Files Reviewed: 4
Findings: 1

By Severity:

  • 🟠 Medium: 1

The PR introduces a SkipAutoUpdate flag to bypass auto-update posture checks for non-user device paths, but the current implementation unconditionally hard-codes SkipAutoUpdate: true at both the enrollment-key registration and add-host-to-network call sites, silently disabling auto-update posture enforcement for all hosts regardless of administrator configuration.

Files Reviewed (4 files)
controllers/enrollmentkeys.go
controllers/hosts.go
models/structs.go
pro/logic/posture_check.go

Copy link

@tenki-reviewer tenki-reviewer bot left a comment

Choose a reason for hiding this comment

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

Overview

This PR adds a SkipAutoUpdate bool field to PostureCheckDeviceInfo and updates GetPostureCheckViolations so that the AutoUpdate posture check is skipped when either d.IsUser or d.SkipAutoUpdate is true (previously only for users). The intent appears to be suppressing auto-update enforcement during host admission flows.

Design Concern: Hard-coded Bypass

Both admission-time call sites — controllers/enrollmentkeys.go (host self-registration via enrollment key) and controllers/hosts.go (addHostToNetwork) — now hard-code SkipAutoUpdate: true in the PostureCheckDeviceInfo literal. This means auto-update posture checks are unconditionally disabled for every device joining any network, regardless of the AutoUpdate value the host actually reports and regardless of whether a network administrator has deliberately configured an auto-update posture check for that network.

The periodic background runner (RunPostureChecks) still reads h.AutoUpdate correctly via GetPostureCheckDeviceInfoByNode (line 308 of pro/logic/posture_check.go), so the enforcement asymmetry is: checks pass at admission but may fail on periodic re-evaluation. This is confusing UX and a potential policy gap.

What Was Changed

  • models/structs.go: Added SkipAutoUpdate bool to PostureCheckDeviceInfo — struct change looks fine.
  • pro/logic/posture_check.go line 136: Updated skip condition from d.IsUser to d.IsUser || d.SkipAutoUpdate — correct implementation of the new flag.
  • controllers/enrollmentkeys.go line 385 and controllers/hosts.go line 698: Replaced AutoUpdate: <host>.AutoUpdate with SkipAutoUpdate: true — this is where the problem lies.

Recommendation

If the intent is that auto-update state cannot be reliably known at registration/join time (e.g., the client hasn't synced its config yet), then:

  • Document that rationale with a comment at the call sites.
  • Consider computing SkipAutoUpdate based on actual host state rather than a hard-coded true.

If the intent is to never enforce auto-update posture at admission, the change achieves that — but it silently removes a previously enforced security control with no visible indication to operators.

@abhishek9686 abhishek9686 merged commit 2645abd into develop Mar 19, 2026
6 checks passed
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.

2 participants