feat(node-ensure/delete-untracked/metrics) Ensure there is no extra ARP/FDB configuration when ensuring node endpoints, build OTEL metrics out of control loop#330
Conversation
…RP/FDB configuration when ensuring node endpoints, build OTEL metrics out of control loop Related to #328
cdfa48c to
82e154b
Compare
| if err != nil { | ||
| // If the netns path no longer exists, deactivate the endpoint. Other | ||
| // activation errors should not prevent the remaining endpoints from being ensured. | ||
| if os.IsNotExist(errors.Cause(err)) { |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
A wrapped file-not-found error from erepo.Activate(...) may not be recognized here, so missing endpoint.TargetNetnsPath can skip erepo.Deactivate(...). An attacker who can cause stale netns paths during restore could leave orphaned endpoint network state behind.
More details about this
erepo.Activate(...) returns an error, and this code decides whether to clean up the endpoint by checking os.IsNotExist(errors.Cause(err)). That check only looks at the unwrapped cause, so a wrapped not exist error can be missed and the code will take the failed to ensure endpoint path instead of calling erepo.Deactivate(...).
A plausible abuse case is: 1) an attacker causes endpoint.TargetNetnsPath to point to a namespace path that has already been removed, 2) erepo.Activate(endpointCtx, network, endpoint, ...) returns a wrapped file-not-found error for that path, 3) this os.IsNotExist(errors.Cause(err)) check fails to recognize it, and 4) the stale endpoint is left active because erepo.Deactivate(endpointCtx, network, endpoint) is never called. In a system restoring many endpoints, repeatedly triggering this on selected endpoint.NetworkID / endpoint.TargetNetnsPath values can leave orphaned network state behind, which can keep unexpected connectivity or policy state attached to containers that should have been cleaned up.
To resolve this comment:
✨ Commit fix suggestion
| if os.IsNotExist(errors.Cause(err)) { | |
| if errors.Is(errors.Cause(err), fs.ErrNotExist) { |
View step-by-step instructions
-
Replace the
os.IsNotExist(...)check with the standard libraryerrors.Is(...)check against the not-exist sentinel error.
Changeos.IsNotExist(errors.Cause(err))tostderrors.Is(err, fs.ErrNotExist). -
Stop unwrapping the error with
errors.Cause(...)in this condition.
errors.Isalready walks wrapped errors, so it can match the original not-exist error througherrors.Wrap,fmt.Errorf(... %w ...), and similar wrappers. -
Add the standard library imports needed for the new check.
If this file already importsgithub.com/pkg/errorsaserrors, import the standard library package with an alias such asstderrors "errors", and addio/fsforfs.ErrNotExist. -
Keep the existing deactivate path the same, and only update the condition.
The resulting condition should look likeif stderrors.Is(err, fs.ErrNotExist) { ... }. -
Alternatively, if this file does not import
github.com/pkg/errorswith the nameerrors, use the standard library package directly aserrors.Is(err, fs.ErrNotExist).
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by os-error-is-not-exist.
You can view more details about this finding in the Semgrep AppSec Platform.
Related to #328