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

Changes to Common Controls Baseline #315

Merged
merged 22 commits into from
Aug 5, 2024
Merged

Conversation

jkaufman-mitre
Copy link
Collaborator

@jkaufman-mitre jkaufman-mitre commented Jun 11, 2024

🗣 Description

The following changes were made within the common controls baseline:

  • Policy 2.2 was removed
  • Policy 10.1 was removed
  • Policy Group 16, Additional Google Services, was created
  • Policy group 17, Multi-Party Approval, was created
  • Policy 14.1 implementation was updated.
  • Change 11.1 to SHALL

💭 Motivation and context

Fixes #240
Fixes #252
Fixes #274
Fixes #276

🧪 Testing

✅ 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.

Copy link
Collaborator

@adhilto adhilto left a comment

Choose a reason for hiding this comment

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

GWS.COMMONCONTROLS.2.1v0.2 still needs more work.

The note, "granular controls may be used if the agency needs it," is misleading because it kind of implies that specifying what controls you want context aware access to apply is optional. It's not. Simply doing what is currently the implementation for 2.1 doesn't do anything. The full instructions need to include the following elements:

  1. Turn on context aware access (as instructed in the current implementation, see screenshot)
    image

  2. Create one or more access levels, as needed
    image
    image

  3. Determine the conditions of the rule, per agency discretion.
    image

  4. Assign the access levels
    image

Finally, there are still a lot of open questions left.

  • What should the conditions of the access levels be? Entirely up to agency discretion?
  • Which apps should be assigned to those levels? All? Just the admin center? Agency discretion?
  • Which users should be assigned to those levels? They can be assigned at the OU and or group level. All? Or agency discretion?
  • What mode should those assignments be in? Just monitor? Or active? Left to agency discretion?

Recommendations:

  • Clarify that "Note:" for 2.1. Maybe: "Note: the implementation details of context-aware access use cases will vary per agency. Refer to Google's documentation on implementing context-aware access for your specific use cases."
  • Augment the implementation steps to include all that I detailed above.
  • Delete the implementation steps for 2.2 (they're still there).
  • Address the open questions listed above.

Copy link
Collaborator

@adhilto adhilto left a comment

Choose a reason for hiding this comment

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

Feedback for GWS.COMMONCONTROLS.16.1

@jkaufman-mitre
Copy link
Collaborator Author

jkaufman-mitre commented Jun 12, 2024

Removed Policy 10.1 because there is no implementation for the policy. Agencies will be evaluated on their in-house procedures for items such as this as part of their NIST 800-53 control assessments. The technical implementation steps are already covered within this policy group.

jkaufman-mitre and others added 5 commits June 13, 2024 09:17
…Baseline v0.2.md

Co-authored-by: Alden Hilton <106177711+adhilto@users.noreply.github.com>
…Baseline v0.2.md

Co-authored-by: Alden Hilton <106177711+adhilto@users.noreply.github.com>
Co-authored-by: Alden Hilton <106177711+adhilto@users.noreply.github.com>
@adhilto adhilto added this to the Coast milestone Jun 19, 2024
@jkaufman-mitre
Copy link
Collaborator Author

jkaufman-mitre commented Jun 20, 2024

Moved 11.2 to Policy Group 10 (#318)

@adhilto
Copy link
Collaborator

adhilto commented Jun 20, 2024

Moved 11.2 to Policy Group 10 (#318)

Looks like this change might not have been pushed yet?

@jkaufman-mitre
Copy link
Collaborator Author

Made Common Controls 11.1 a SHALL.

@jkaufman-mitre
Copy link
Collaborator Author

TTP Mappings have been added.

@jkaufman-mitre
Copy link
Collaborator Author

@adhilto @buidav Can you review this. This one is ready for review again.

@mdueltgen
Copy link
Collaborator

As discussed, removed Issue 290 for 2.2. Context Aware Access revamp will happen in the next release after Coast.

@adhilto Please review the 2.1 section for Coast release including changes to the implementation steps.

@adhilto adhilto dismissed their stale review August 1, 2024 18:49

OOO and don't want this PR to block on me

@snarve snarve mentioned this pull request Aug 1, 2024
@snarve snarve self-requested a review August 1, 2024 22:02
@snarve
Copy link
Collaborator

snarve commented Aug 1, 2024

11.2 is unchanged as @adhilto mentioned in the email thread. Since there is a separate issue for this( #318 ), recommend to create a separate branch for this and commit separately as this PR tackles multiple issues already. Having a separate branch and PR (per issue) would ease the tracking and updates.
@jkaufman-mitre @mdueltgen @adhilto @buidav any thoughts on this

@snarve snarve self-assigned this Aug 1, 2024
@adhilto
Copy link
Collaborator

adhilto commented Aug 2, 2024

11.2 is unchanged as @adhilto mentioned in the email thread. Since there is a separate issue for this( #318 ), recommend to create a separate branch for this and commit separately as this PR tackles multiple issues already. Having a separate branch and PR (per issue) would ease the tracking and updates. @jkaufman-mitre @mdueltgen @adhilto @buidav any thoughts on this

I agree, that's the right call (with the caveat that branch be made after this one is merged in to ease merge conflicts). I just edited the description of this PR to remove that issue so it's accurate and so that that issue won't auto-close once this PR is merged.

@mdueltgen
Copy link
Collaborator

11.2 is unchanged as @adhilto mentioned in the email thread. Since there is a separate issue for this( #318 ), recommend to create a separate branch for this and commit separately as this PR tackles multiple issues already. Having a separate branch and PR (per issue) would ease the tracking and updates. @jkaufman-mitre @mdueltgen @adhilto @buidav any thoughts on this

I agree, that's the right call (with the caveat that branch be made after this one is merged in to ease merge conflicts). I just edited the description of this PR to remove that issue so it's accurate and so that that issue won't auto-close once this PR is merged.

Sounds good I will make sure #318 and #290 are in different branches for the next release. I think now that those two have been removed from the description of this PR, I think we should be good to do review of PR as is and merge for Coast.

Copy link
Collaborator

@adhilto adhilto left a comment

Choose a reason for hiding this comment

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

Baseline looks good, and Rego changes are complete. @snarve did half yesterday, I did the last half today.

@adhilto adhilto changed the title Changes to Common Controls 2.1, 2.2, 14.1, and Policy Group 16 Changes to Common Controls Baseline Aug 2, 2024
Copy link
Collaborator

@mdueltgen mdueltgen left a comment

Choose a reason for hiding this comment

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

Rev

Copy link
Collaborator

@snarve snarve 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

@snarve snarve merged commit c823249 into main Aug 5, 2024
7 checks passed
@snarve snarve deleted the common-controls-changes-post-0.2-v2 branch August 5, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants