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

Remove GWS.CHAT.5.1 #322

Merged
merged 6 commits into from
Jul 19, 2024
Merged

Remove GWS.CHAT.5.1 #322

merged 6 commits into from
Jul 19, 2024

Conversation

jkaufman-mitre
Copy link
Collaborator

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

πŸ—£ Description

Remove Chat.5.1 as Chat apps are now also controlled by the marketplace settings (see CommonControls.11.1).

πŸ’­ Motivation and context

Fixes #222
Google Update

πŸ§ͺ 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.

@jkaufman-mitre
Copy link
Collaborator Author

@adhilto This PR is still waiting to be reviewed.

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.

Going back to the original discussion of this policy. Chat 5.1 is currently contradictory to our current stance with Common Controls 11.1. If Common Controls 11.1 allowlisting is not enforced, then turning this setting on allows any user to install any chat app. Which is insecure.

It seems that we should either delete this baseline and enforce Common Controls as a SHALL but leave the decision to enable this setting up to organizations/agencies

OR

We should rewrite this policy to make it organizational unit specific.
example:

The ability to install Chat apps SHALL be disabled for the top-level organizational unit

_Note:_ The ability to install Chat apps MAY be enabled on a per OU basis.

This is to prevent the default of allowing anyone to install Chat apps if App allowlisting is not enforce but allow flexibility for specific organizational units to install chat applications.

@adhilto
Copy link
Collaborator

adhilto commented Jun 27, 2024

Going back to the original discussion of this policy. Chat 5.1 is currently contradictory to our current stance with Common Controls 11.1. If Common Controls 11.1 allowlisting is not enforced, then turning this setting on allows any user to install any chat app. Which is insecure.

It seems that we should either delete this baseline and enforce Common Controls as a SHALL but leave the decision to enable this setting up to organizations/agencies

OR

We should rewrite this policy to make it organizational unit specific. example:

The ability to install Chat apps SHALL be disabled for the top-level organizational unit

_Note:_ The ability to install Chat apps MAY be enabled on a per OU basis.

This is to prevent the default of allowing anyone to install Chat apps if App allowlisting is not enforce but allow flexibility for specific organizational units to install chat applications.

I agree with David on this one. I'll also add that per the research you documented here:

  • "If the new feature is turned on, but the app IS NOT allowlisted, then only admins are allowed to install the app for the org." Acceptable from a security standpoint.
  • "If the feature is turned on and the app IS allowlisted, then both admins and users can install." Acceptable from a security standpoint.
  • "If the new feature is off, then the app cannot be installed." Also acceptable from a security standpoint.

Therefore, my vote would be that we delete CHAT.5.1 entirely.

@jkaufman-mitre
Copy link
Collaborator Author

jkaufman-mitre commented Jun 27, 2024

@adhilto @buidav Turning this setting on does not override Common Controls 11.1. In the contrary, turning this off does not allow users to install any chat app. Turning this feature one allows users to install allowlisted chat apps.

Not allowing any user to install any app could be a hinderance to the user.

@jkaufman-mitre
Copy link
Collaborator Author

jkaufman-mitre commented Jun 27, 2024

My vote would be to make this policy a SHOULD because if there is no need for users to have chat apps, then it should be disabled to protect the org.

Removing this policy all together would allow for this setting to be on which is could allow for unauthorized apps to be installed if an admin account gets compromised. We should have a baseline stating that it SHOULD be disabled and add a not that if there is an organizational need for chat apps then it can be enabled.

My recommendation is for the policy to be: "User-level ability to install Chat apps SHOULD be disabled."

@jkaufman-mitre
Copy link
Collaborator Author

@adhilto, Also I realized that the PR changes did not reflect what my recommendation was, so went in ad commited the updated policy statement.

@adhilto
Copy link
Collaborator

adhilto commented Jun 27, 2024

@adhilto, Also I realized that the PR changes did not reflect what my recommendation was, so went in ad commited the updated policy statement.

Thanks for pointing that out, that completely changes the discussion.

@adhilto @buidav Turning this setting on does not override Common Controls 11.1.

David did not claim that it does. His point was that if Common Controls 11.1 was not implemented and this setting were on, users could install any Chat app. I haven't tested this myself but it certainly seems like that would be the case. Regardless, since you updated it from "SHOULD be enabled" to "SHOULD be disabled," this point is no longer important.

Not allowing any user to install any app could be a hinderance to the user.

Hindrance, yes. Security risk? No. Though it is a reason for why we wouldn't want to require this setting be disabled, which is now the current recommendation.

Weighing both sides of the issue, the reason for including 5.1 "SHOULD be disabled" is as follows:

  • Defense in depth. Yes, COMMON.CONTROLS.11.1 would prevent non-approved apps from being installed (per your research here ), but to David's point, what if COMMON.CONTROLS.11.1 is not implemented? Disabling the setting in question for CHAT.5.1 would ensure that even in that case, unapproved Chat apps could not be installed.

Reason for enabling "Allow users to install Chat apps":

  • The usability cost in setting that to OFF is quite high, because as you indicated here, this would completely prevent all Chat apps from being installed, even if the app is allow listed.

Weighing both those two points, I'm inclined to think that this is a setting that we need don't need to take a stance on. Since the security benefit is so minimal and the usability cost so high, each agency should be permitted to weigh the tradeoffs themselves and make their own decision. In other words, I'm still in favor of cutting this control.

That said, as the interactions between these two settings isn't immediately obvious, it could be useful to document and explain this somewhere, so that agencies are able to make an informed decision. We should not mandate that this setting be ON or OFF though.

@snarve snarve added this to the Coast milestone Jul 3, 2024
@jkaufman-mitre
Copy link
Collaborator Author

We will go ahead an remove CHAT.5.1v0.2 and make COMMONCONTROLS.11.1v0.2 a SHALL.

@jkaufman-mitre
Copy link
Collaborator Author

Policy Group 5 has been removed. @adhilto Ready for review again.

@adhilto
Copy link
Collaborator

adhilto commented Jul 11, 2024

Policy Group 5 has been removed. @adhilto Ready for review again.

Just need to update the table of contents. Other than that looks good.

@adhilto
Copy link
Collaborator

adhilto commented Jul 11, 2024

@buidav I just implemented the Rego changes, ready for your review.

@adhilto adhilto changed the title Changed 5.1 to a SHOULD and added a note Remove GWS.CHAT.5.1 Jul 11, 2024
@jkaufman-mitre
Copy link
Collaborator Author

@adhilto @buidav I have updated the Table of Content.

@mdueltgen
Copy link
Collaborator

@adhilto @buidav Any finals thoughts on this or are we good to merge? I think all changes requested were handled.

@buidav buidav merged commit 94657a0 into main Jul 19, 2024
6 checks passed
@buidav buidav deleted the chat-changes-5.1 branch July 19, 2024 02:14
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.

Pre-Requisite needs to be Added to Common Controls Policy Group 11
5 participants