Repository navigation
fix(anta.tests): Detect SNMPv3 remote users and align SA178 with revision 1.3 - #1856
carl-baillargeon wants to merge 6 commits into
Conversation
…sion 1.3 SA178 read SNMPv3 users from 'show snmp user', which never lists remote users, so devices with only remote users were reported not affected. Read the authentication state from the running-config 'snmp-server user' lines instead, and parse the optional 'remote <host> [udp-port <port>]' prefix. Align SA178 with revision 1.3: new affected range and fixed releases, and the SWIX hotfix for 4.36.2F, 4.35.6M, 4.34.8M and 4.33.10M. Fixes aristanetworks#1761
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Parse SNMPv3 authentication from configuration anta/_advisory/facts/management.py, tests/units/_advisory/facts/test_management.py |
SnmpV3AuthenticationFact now reads running-configuration text and derives authentication state from credential syntax. Parsing handles optional remote hosts and UDP ports. Tests cover local, remote, malformed, and no-auth configurations. |
Integrate hotfix state and mitigated conditions anta/_advisory/facts/software.py, anta/tests/advisories/sa_178.py, anta/_advisory/findings/models.py, anta/_advisory/findings/projection.py, docs/security-advisory/tests-overview.md, tests/units/_advisory/facts/test_software.py |
SA178 adds a persistent-hotfix fact and uses its state for listed hotfix releases. The advisory’s affected and fixed release boundaries advance by one patch. AffectedResult now stores mitigated conditions, which the projection renders in affected results. Advisory metadata is updated. |
Validate detection and assessment outcomes tests/units/anta_tests/advisories/test_sa_178.py, tests/units/_advisory/findings/test_projection.py |
Tests cover revised release boundaries, remote and unauthenticated users, effective and ineffective hotfix states, unavailable facts, remediation, and mitigated-condition rendering. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant SA178
participant SnmpV3AuthenticationFact
participant SA178HotfixFact
participant EOS
participant _assess_sa178
SA178->>SnmpV3AuthenticationFact: collect authentication state
SnmpV3AuthenticationFact->>EOS: read running configuration
EOS-->>SnmpV3AuthenticationFact: return configuration text
SA178->>SA178HotfixFact: collect persistent hotfix state
SA178HotfixFact->>EOS: read hotfix command output
EOS-->>SA178HotfixFact: return hotfix state
SA178->>_assess_sa178: pass collected facts
Possibly related PRs
- aristanetworks/anta#1857: Changes SA178 expected-result documentation for encrypted syntax and adds advisory docstring layout checks.
Suggested reviewers: clausholbecharista, gmuloc
Merge Risk: 
5c7bf
-
Remote-user detection and hotfix assessment have no identified merge-blocking issue. Merge after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the primary changes: remote SNMPv3 user detection and SA178 revision 1.3 alignment. Description check The description follows the required template, explains the remote-user parsing and SA178 updates, references issue #1761, documents verification, and marks all checklist items complete.Linked Issues check Issue #1761requires SA178 to detect authenticated remote SNMPv3 users.SnmpV3AuthenticationFactnow parsesshow running-config | include ^snmp-server user, including the optional `remote […Out of Scope Changes check The changes remain within SA178. The release matrix, fixed releases, SA178HotfixFact, remediation logic, finding rendering, documentation, and tests implement the current PR intent for SA178 revisio…Full details: Linked Issues check
-
Explanation
Issue
#1761requires SA178 to detect authenticated remote SNMPv3 users.SnmpV3AuthenticationFactnow parsesshow running-config | include ^snmp-server user, including the optionalremote <host> [udp-port <port>]fields. The parser detects the issue reproduction layout and returns an unavailable fact for malformed layouts. The changed tests cover remote, UDP-port, IPv6, mixed, no-auth, and malformed configurations. No coding requirement from#1761remains unmet.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within SA178. The release matrix, fixed releases,
SA178HotfixFact, remediation logic, finding rendering, documentation, and tests implement the current PR intent for SA178 revision 1.3 and its hotfix behavior. The finding-model changes support the SA178 mitigation output. No unrelated subsystem or feature change is demonstrated.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 15 functions across 9 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
fix/sa178-snmp-remote-users
- Resolve merge conflict in branch
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1856 +/- ##
==========================================
+ Coverage 95.39% 95.50% +0.11%
==========================================
Files 172 172
Lines 17261 17263 +2
Branches 2644 2638 -6
==========================================
+ Hits 16466 16487 +21
+ Misses 480 472 -8
+ Partials 315 304 -11
🚀 New features to boost your workflow:
|
| return AffectedResult( | ||
| vulnerability_id=VULNERABILITY_ID, | ||
| context=(software_context,), | ||
| context=(EosReleaseAssessment(software_context.fact, VersionRelation.FIXED),), |
There was a problem hiding this comment.
for an affected version like 4.35.6 we are statng Version FIXED here (because the hotfix was applied).
Would it not make more sense to state VersionRelartion.AFFECTED and claum the mitigation is enabled?
| "eos_data": eos_data(LEGACY_USER, PERSISTENT_HOTFIX), | ||
| "expected": expected_result( | ||
| AntaTestStatus.FAILURE, | ||
| "The device is affected because EOS version '4.35.6M' is fixed, the SNMPv3 authentication key is enabled, and the SNMPv3 credential syntax is legacy", |
There was a problem hiding this comment.
This is wrong 4.35.6M is not FIXED it is AFFECTED but hotfix is present
ankudinov
left a comment
ankudinov
left a comment
There was a problem hiding this comment.
LGTM. SA178 lab test passed
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 1
tests/units/anta_tests/advisories/test_sa_178.py: 1
299-299:Trivial · Maintainability & Code Quality · Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Put the actual value first in this assertion.
The assertion puts the expected set on the left and
HOTFIX_RELEASESon the right. Every other assertion in this file puts the actual value first. SonarCloud flags the inconsistency. Swap the operands to match the file convention.♻️ Proposed fix
Source: Linters/SAST tools
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: defaults
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1ebe0cc0-9de0-46e8-beb6-2925772ddbcb
-
anta/_advisory/facts/management.pyanta/_advisory/findings/models.pyanta/_advisory/findings/projection.pyanta/tests/advisories/sa_178.pytests/units/_advisory/facts/test_software.pytests/units/_advisory/findings/test_projection.pytests/units/anta_tests/advisories/test_sa_178.py
-
- tests/units/_advisory/facts/test_software.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Description
SA178 read SNMPv3 users from
show snmp user, which never lists remote users. Devices with only remote users that have an authentication key were reported as not affected.show running-config | include ^snmp-server useroutput as the credential-syntax check. The parser handles the optionalremote <host> [udp-port <port>]prefix. An unexpected line layout now returns an error instead of being skipped.SA178HotfixFact. The SWIX only counts on those four releases, and only when it is installed and set to load at boot. Legacy keys still needconfigure convert new-syntax.Verified on vEOS-lab from 4.31.0F to 4.36.3M with local, remote, remote
udp-port, IPv6 remote and no-auth users. With remote-only users,mainreports not affected and this branch reports affected, with the correct remediation per release.Fixes #1761
Checklist
pre-commit run)tox -e testenv)Summary by CodeRabbit