Finalize Regular Backups strategy (ML1 & ML2)#97
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cd4fd1007
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if _kv_has(kv, "backup_location", "offsite") and _kv_has(kv, "immutability", "enabled"): | ||
| rows.append(_row("ML1", "ML1-RB-02", "Offsite or immutable backups", "PASS", "Medium", | ||
| "Maintain offsite backups with immutability enabled.", t)) |
There was a problem hiding this comment.
ML1 offsite/immutability pass requires both flags
The ML1-RB-02 detector currently passes only when the evidence contains both backup_location=offsite and immutability=enabled (lines 108-110), but the documented rule for ML1-RB-02 is “offsite or immutable” (see security/evidence/regular_backups/README.md lines 44-47). Any ML1 evidence that shows only one of those signals—e.g., backup_location=offsite without an immutability field—will no longer register a ML1-RB-02 PASS and will fall through to the generic failure path, under-reporting compliance.
Useful? React with 👍 / 👎.
| elif _kv_has(kv, "restore_test", "full") and _kv_has(kv, "status", "success"): | ||
| rows.append(_row("ML1", "ML1-RB-03", "Restore testing", "PASS", "Medium", | ||
| "Continue periodic restore testing.", t)) |
There was a problem hiding this comment.
Restore test pass ignores common_point key
ML1-RB-03 is marked as a PASS whenever restore_test=full and status=success are present (lines 121-123), but the README specifies that a PASS should also require common_point=true to prove the restore point (see security/evidence/regular_backups/README.md lines 48-53 and the test data table). Evidence such as restore_test=full plus status=success without any common_point= tag will be reported as ML1-RB-03 PASS, producing false positives relative to the documented detection criteria.
Useful? React with 👍 / 👎.
|
@Seshini2002 Those two automated comments are worth reviewing |
du-dhartley
left a comment
There was a problem hiding this comment.
@Seshini2002 in addition to the two comments that the bot has put in here already (that are relevant and should be addressed), the evidence screenshots don't need to be uploaded into this repository.
Taking the screenshots for evidence is fine, but they would belong in some sort of document that you can use to show the work, rather than being committed here.
romil-bijarnia
left a comment
There was a problem hiding this comment.
Overall this is a solid improvement. The Regular Backups strategy is clearer and more deterministic with the move to consistent key=value evidence, and the ML1/ML2 coverage + test harness makes the results easier to verify and less likely to regress. Documentation and evidence are thorough; just keep an eye on avoiding repo bloat and making sure docs/evidence stay aligned with the parsing rules as this evolves.
There was a problem hiding this comment.
We cannot have so many screenshots in our codebase, these will need to be removed and instead added someplace else
Please remove these screenshots from the repo. and store them externally (PR attachments / OneDrive / SharePoint / etc.) and link them instead. This was raised before as well for the previous PR
There was a problem hiding this comment.
This guide is out of sync with the actual evidence files + the strategy implementation.
- It says “keyword-based detection” but the strategy is now KV-driven.
- Several examples don’t match the actual evidence files (e.g., restore fail shows
status=failbut the evidence usesstatus=failure; ML2 verification examples don’t match the actual keys present; repo audit fail example referencesis_backup_admin=falsewhich isn’t inrepo_audit_fail.txt).
Please update the document so the examples and rules match the current evidence keys and the code.
There was a problem hiding this comment.
This README is also out of sync with the code/evidence. It claims “keyword-based” detection.
-
The PASS/FAIL rules described don’t match what security/strategies/regular_backups.py actually checks (notably the offsite/immutability logic and restore/common_point conditions).
-
The “exact contents from your test files” section is factually incorrect vs the current evidence files.
-
Please correct or simplify this README so it stays accurate.
There was a problem hiding this comment.
This file contains contradictory evidence: access=restricted and access=allowed in the same evidence sample.
That violates the guide’s own “one control outcome per file” rule and makes the evidence unrealistic.
Please fix by making the evidence consistent (single meaning), and if you need ML2 evidence to emit ML1 rows, do that in code instead of forcing contradictory keys in the file.
There was a problem hiding this comment.
This evidence mixes two different restore types in one file (restore_test=full and restore_test=item-level) and duplicates keys (status=success twice).
That violates the “one control outcome per file” guideline and makes the evidence unclear/unrealistic.
Please split or simplify so the file represents one outcome cleanly.
There was a problem hiding this comment.
Same issue as the pass file: mixes restore_test=full and restore_test=item-level, duplicates status keys.
Please make it represent one clear outcome.
There was a problem hiding this comment.
These look like ML2 audit/access evidence, but they don’t follow the project’s own “ML2 files must include _ml2 in filename” rule, so ML2 logic will never run for them.
Also the content doesn’t match what the docs claim for “unauthorised access” (missing is_backup_admin=false, etc.).
Please either:
-
rename + align these files to be real ML2 evidence (include _ml2 and the expected keys), or
-
treat them as ML1 evidence and update the docs accordingly.
|
• Updated ML1-RB-02 logic so a PASS is correctly triggered when either backup_location=offsite or immutability=enabled, aligning the implementation with the documented “offsite or immutable” requirement. |
• Finalised Regular Backups (RB) strategy for Essential Eight
• Implemented full support for ML1 (RB-01 to RB-06) and ML2 (RB-01 to RB-06)
• Fixed ML1 and ML2 detection logic so all evidence files are always triggered
• Ensured ML2 evidence produces both ML1 and ML2 results
• Corrected and standardised all Regular Backups evidence text files
• Added complete test coverage with verified PASS and FAIL scenarios
• Updated README.md and USER_INSTRUCTIONS.md to reflect final behaviour