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

Fix by-component implemented requirement checks #796

Conversation

aj-stein-gsa
Copy link
Contributor

@aj-stein-gsa aj-stein-gsa commented Oct 18, 2024

Committer Notes

Fixes #770.

  • Add new constraint to report against bad practice FedRAMP will not accept for requirements outside of statement.
  • Fix current constraint and correct the test, target, and wording.

All Submissions:

By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.

@aj-stein-gsa aj-stein-gsa self-assigned this Oct 18, 2024
@aj-stein-gsa aj-stein-gsa linked an issue Oct 18, 2024 that may be closed by this pull request
1 task
@aj-stein-gsa aj-stein-gsa force-pushed the 770-fix-by-component-statement-checks branch from ef26bb6 to cefbf1b Compare October 19, 2024 02:42
@@ -212,31 +212,31 @@
<prop name="control-origination" value="sp-system" ns="https://fedramp.gov/ns/oscal"/>
<prop name="implementation-status" value="partial" ns="https://fedramp.gov/ns/oscal"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rene2mt, I was working on this later last week and I was surprised this bug fix is a little more confusing than I had once thought. I have a question about how we came to have the prop[@name="implementation-status"] and not use the dedicated implementation-status assembly? I have been gone a while, but this the prop is also thoroughly documented on our site and the primary implementation-status (not the prop) is not documented at all.

Do you know if there was/is a reason for this? It is not immediately related to the PR but tests kept failing as I moved that around and this led me to scratch my head.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the prop[@name="implementation-status"] prop was created because FedRAMP needed a way to specify and/or determine the overall status for a control at the control-implementation/implemented-requirement level. This prop was also to support scenarios where control-implementation/implemented-requirement a control has needs to have more than one status specified (e.g., "partial" and "planned"). There is some documentation around this, but it needs to be expanded to clarify the use of the "implementation-status" prop at the implemented-requirement level versus the use of the implementation-status assembly at the by-component level.

New issue #802 is going to explore the scenarios and options for implementation status in FedRAMP OSCAL SSPs. We'll probably draft an ADR for that.

Copy link
Member

@Rene2mt Rene2mt left a comment

Choose a reason for hiding this comment

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

I know this is draft. Just adding suggestions per the discussion earlier.

@@ -212,31 +212,31 @@
<prop name="control-origination" value="sp-system" ns="https://fedramp.gov/ns/oscal"/>
<prop name="implementation-status" value="partial" ns="https://fedramp.gov/ns/oscal"/>
Copy link
Member

Choose a reason for hiding this comment

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

I believe the prop[@name="implementation-status"] prop was created because FedRAMP needed a way to specify and/or determine the overall status for a control at the control-implementation/implemented-requirement level. This prop was also to support scenarios where control-implementation/implemented-requirement a control has needs to have more than one status specified (e.g., "partial" and "planned"). There is some documentation around this, but it needs to be expanded to clarify the use of the "implementation-status" prop at the implemented-requirement level versus the use of the implementation-status assembly at the by-component level.

New issue #802 is going to explore the scenarios and options for implementation status in FedRAMP OSCAL SSPs. We'll probably draft an ADR for that.

<description>
<p>Information System Component Inventory (CM-8) is partially implemented.</p>
</description>
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="partial"/>
Copy link
Member

Choose a reason for hiding this comment

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

Use the implementations-status assembly inside the by-component, however, it is optional.

Suggested change
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="partial"/>

<responsible-role role-id="system-admin">
<party-uuid>11111111-0000-4000-9000-000000000001</party-uuid>
</responsible-role>
</by-component>
</implemented-requirement>

<implemented-requirement uuid="bbbbbbbb-0000-4000-9000-00000000000b" control-id="cm-8">
<prop name="control-origination" value="sp-system" ns="https://fedramp.gov/ns/oscal"/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<prop name="control-origination" value="sp-system" ns="https://fedramp.gov/ns/oscal"/>
<prop name="control-origination" value="sp-system" ns="https://fedramp.gov/ns/oscal"/>
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="partial"/>

<description>
<p>Access Control Policy and Procedures (AC-1) is fully implemented in our system.</p>
</description>
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="implemented"/>
Copy link
Member

Choose a reason for hiding this comment

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

Use the implementations-status assembly inside the by-component, however, it is optional.

Suggested change
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="implemented"/>

Comment on lines +172 to +198
<expect id="by-component-requirement-not-inside-statement" target="implemented-requirement" test="not(exists(by-component))">
<message>A FedRAMP SSP MUST document only a component-based implemented requirement within a specific statement, not at the control level.</message>
</expect>
<expect id="missing-response-components" target="implemented-requirement/statement" test="count(./by-component) gt 0">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<expect id="by-component-requirement-not-inside-statement" target="implemented-requirement" test="not(exists(by-component))">
<message>A FedRAMP SSP MUST document only a component-based implemented requirement within a specific statement, not at the control level.</message>
</expect>
<expect id="missing-response-components" target="implemented-requirement/statement" test="count(./by-component) gt 0">
<expect id="by-component-requirement-not-inside-statement" target="implemented-requirement" test="not(exists(by-component))" level="ERROR">
<message>A FedRAMP SSP MUST document only a component-based implemented requirement within a specific statement, not at the control level.</message>
</expect>
<expect id="missing-response-components" target="implemented-requirement/statement" test="count(./by-component) gt 0" level="ERROR">

<description>
<p>Access Control Policy and Procedures (AC-1) is fully implemented in our system.</p>
</description>
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="implemented"/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="implemented"/>

<description>
<p>Information System Component Inventory (CM-8) is partially implemented.</p>
</description>
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="partial"/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<prop ns="https://fedramp.gov/ns/oscal" name="implementation-status" value="partial"/>

Defining them outside of a statement is syntatically valid, but outside
of FedRAMP best practices and is not accepted. We must add an additional
constraint to indicate this should be removed.

Co-Authored-By: Kylie Hunter <[email protected]>
@aj-stein-gsa aj-stein-gsa force-pushed the 770-fix-by-component-statement-checks branch from cefbf1b to 108feca Compare October 24, 2024 13:59
@aj-stein-gsa
Copy link
Contributor Author

aj-stein-gsa commented Oct 24, 2024

I am working on integrating Rene's feedback and reviewing the documentation to fix the test. His feedback was very helpful, but we found an unrelated bug when looking into errors with ssp-all-VALID.xml that were not making a lot of sense.

Thanks to Kylie for opening that issue in #818.

@aj-stein-gsa
Copy link
Contributor Author

Closing this, will reopen on same PR when I am back into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FedRAMP external constraints validating by-component at wrong layer
2 participants