Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config file support for control omission #431

Merged
merged 20 commits into from
Oct 9, 2024

Conversation

adhilto
Copy link
Collaborator

@adhilto adhilto commented Sep 24, 2024

🗣 Description

Added config file support for omitting controls. See documentation here: https://github.com/cisagov/ScubaGoggles/blob/351-add-config-file-support-for-control-omission/docs/usage/Config.md#omit-policies.

💭 Motivation and context

Closes #351.

📷 Screenshots (if appropriate)

image
image

🧪 Testing

Rationale left blank:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale:

Rationale empty:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale: ""

Neither rationale nor expiration date provided:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:

Rationale provided:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale: "example rationale"

omitpolicies left blank:

baselines: [drive]
omitpolicy:

expiration date in past:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale: "example rationale"
    expiration: "2023-12-31"

expiration date in future:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale: "example rationale"
    expiration: "2024-12-31"

expiration date empty:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale: "example rationale"
    expiration: ""

expiration date blank:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale: "example rationale"
    expiration:

expiration date malformed:

baselines: [drive]
omitpolicy:
  GWS.DRIVEDOCS.7.1v0.3:
    rationale: "example rationale"
    expiration: "hello world"

also omit_controls.yaml as is.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • If applicable, All future TODOs are captured in issues, which are referenced in the PR description.
  • The relevant issues PR resolves are linked preferably via closing keywords.
  • All relevant type-of-change labels have been added.
  • I have read and agree to the CONTRIBUTING.md document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge Checklist

  • This PR has been smoke tested to ensure main is in a functional state when this PR is merged.
  • Squash all commits into one PR level commit using the Squash and merge button.

✅ Post-merge Checklist

  • Delete the branch to clean up.
  • Close issues resolved by this PR if the closing keywords did not activate.

@adhilto adhilto added this to the Coast milestone Sep 24, 2024
@adhilto adhilto self-assigned this Sep 24, 2024
@adhilto adhilto linked an issue Sep 24, 2024 that may be closed by this pull request
5 tasks
@adhilto adhilto marked this pull request as draft September 24, 2024 00:27
@adhilto adhilto marked this pull request as ready for review September 24, 2024 00:43
Copy link
Collaborator

@mitchelbaker-cisa mitchelbaker-cisa left a comment

Choose a reason for hiding this comment

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

Looks good so far, below are the testing results:

  1. rationale left blank: warning on config file omitting drivedocs.7.1 but no rationale provided; report reflects behavior
  2. rationale empty: no warning; report details "Test omitted by user. Rationale not provided."
  3. neither rationale or expiration date provided: warning on config file omitting drivedocs.7.1 but no rationale provided; report reflects behavior
  4. rationale provided: functions as expected
  5. omitpolicies left blank: no error, report generated normally without omissions
  6. expiration date in past: warning on provided exp. date already passed and control not omitted; setting exp. date in mm-dd-yyyy format throws warning, expected format is yyyy-mm-dd
  7. expiration date in future: functions as expected
  8. expiration date empty: functions as expected with valid exp. date or without the field
  9. expiration date blank: functions as expected with valid exp. date or without the field
  10. expiration date malformed: warning on malformed expiration date, policy not omitted

I've referenced this PR in the python unit testing epic so we can get config file coverage on the above cases.

docs/usage/Config.md Outdated Show resolved Hide resolved
sample-config-files/omit_policies.yaml Show resolved Hide resolved
@adhilto
Copy link
Collaborator Author

adhilto commented Sep 26, 2024

Looks good so far, below are the testing results:

  1. rationale left blank: warning on config file omitting drivedocs.7.1 but no rationale provided; report reflects behavior
  2. rationale empty: no warning; report details "Test omitted by user. Rationale not provided."

For number 2, can you confirm that no warning is printed? There should be a warning, I just checked again and for me it printed.

@mitchelbaker-cisa
Copy link
Collaborator

Looks good so far, below are the testing results:

  1. rationale left blank: warning on config file omitting drivedocs.7.1 but no rationale provided; report reflects behavior
  2. rationale empty: no warning; report details "Test omitted by user. Rationale not provided."

For number 2, can you confirm that no warning is printed? There should be a warning, I just checked again and for me it printed.

Reran and the warning is displayed. "Test omitted by user. Rationale not provided." is shown in the report details.

Copy link
Collaborator

@mitchelbaker-cisa mitchelbaker-cisa left a comment

Choose a reason for hiding this comment

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

Ran against all test cases and overall looks good. A couple final considerations:

No warning is thrown if a policy ID is not formatted correctly (v0.3 not included):

omitpolicy:
  # the omission is excluded from the report but no indication of this action
  GWS.COMMONCONTROLS.13.1:
    rationale: "exclude this policy"

When I excluded the GWS.COMMONCONTROLS.13.1v0.3 (policy group for system defined rules) ScubaResults.json/BaselineReports.html indicates 1 omission for the Rules baseline. Should we update this to indicate "39 omissions" as to accurately reflect all of the rules?

Screenshot (238)

@adhilto
Copy link
Collaborator Author

adhilto commented Sep 30, 2024

Ran against all test cases and overall looks good. A couple final considerations:

No warning is thrown if a policy ID is not formatted correctly (v0.3 not included):

omitpolicy:
  # the omission is excluded from the report but no indication of this action
  GWS.COMMONCONTROLS.13.1:
    rationale: "exclude this policy"

When I excluded the GWS.COMMONCONTROLS.13.1v0.3 (policy group for system defined rules) ScubaResults.json/BaselineReports.html indicates 1 omission for the Rules baseline. Should we update this to indicate "39 omissions" as to accurately reflect all of the rules?

Good call about warning for bad IDs, I'll add that. As for Rules, interesting question. My first thought was that it should be counted as 1 omission for the common controls report but 39 for rules. That's what it currently does for common controls, but not rules. This is what it currently does:
image

Which...isn't exactly wrong per se. But you're probably right that this rules should have specially handling. I'll work on that too.

scubagoggles/orchestrator.py Outdated Show resolved Hide resolved
scubagoggles/scuba_argument_parser.py Outdated Show resolved Hide resolved
scubagoggles/scuba_argument_parser.py Show resolved Hide resolved
scubagoggles/scuba_argument_parser.py Outdated Show resolved Hide resolved
Co-authored-by: mitchelbaker-cisa <[email protected]>
@buidav buidav self-requested a review October 3, 2024 01:32
Copy link
Collaborator

@buidav buidav left a comment

Choose a reason for hiding this comment

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

@buidav buidav self-requested a review October 9, 2024 23:42
@buidav buidav merged commit 8cc7ff3 into main Oct 9, 2024
6 checks passed
@buidav buidav deleted the 351-add-config-file-support-for-control-omission branch October 9, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add config file support for control omission
3 participants