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

SAP constraints conflicting in the local-definition/objects-and-methods #2059

Open
iMichaela opened this issue Nov 1, 2024 · 19 comments
Open
Labels

Comments

@iMichaela
Copy link
Contributor

Describe the bug

The constraints defined for the assessment-plan/local-definitions/object-and-methods/part/@name and assessment-plan/local-definitions/object-and-methods/part/prop/@name are conflicting.

The following sample:

<?xml version="1.0" encoding="UTF-8"?>
<assessment-plan 
    uuid="60077e84-e62f-4375-8c6c-b0e0d4560c5f"
    xmlns="http://csrc.nist.gov/ns/oscal/1.0">
    <metadata>
        <title>IFA Assessment Plan</title>
        <last-modified>2023-05-18T13:57:28.355446-04:00</last-modified>
        <version>1.0</version>
        <oscal-version>1.1.2</oscal-version>
        <role id="assessor">
            <title>IFA Security Control Assessor</title>
        </role>
        <party uuid="e7730080-71ce-4b20-bec4-84f33136fd58" type="person">
            <name>Amy Assessor</name>
            <member-of-organization>3a675986-b4ff-4030-b178-e953c2e55d64</member-of-organization>
        </party>
        <party uuid="3a675986-b4ff-4030-b178-e953c2e55d64" type="organization">
            <name>Important Federal Agency</name>
            <short-name>IFA</short-name>
            <link href="https://www.ifa.gov" rel="website"/>
        </party>
        <responsible-party role-id="assessor">
            <party-uuid>e7730080-71ce-4b20-bec4-84f33136fd58</party-uuid>
        </responsible-party>
    </metadata>
    <import-ssp href="../3-implementation/ssp.oscal.xml"/>
    <local-definitions>
        <objectives-and-methods control-id="ac-6.1">
            <description>
                <p>documenting AC-06(01) assessment objectives and methods</p>
            </description>
            <prop name="marking" value="ac-6.1_om"/>
            <part id="ac-6.1_obj" name="assessment-objective">
                <prop name="method-id" value="some-objective-value">
                    <remarks>
                        <p>some obj text</p>
                    </remarks>
                </prop>
                <p>some more obj info</p>
            </part>
            <part id="ac-6.1_mtd" name="assessment-method">
                <prop name="method" value="some-value">
                    <remarks>
                        <p>some text</p>
                    </remarks>
                </prop>
                <p>some more info</p>
                <part id="ac-6.1_obj" name="assessment-objects"> 
                </part>
            </part>
            
        </objectives-and-methods>
        <activity uuid="52277182-1ba3-4cb6-8d96-b1b97aaf9d6b">
            <title>Examine System Elements for Least Privilege Design and Implementation</title>
            <description>
                <p>The activity and it steps will be performed by the assessor and facilitated by owner, ISSO, and product team for the IFA GoodRead system with necessary information and access about least privilege design and implementation of the system's elements: the application, web framework, server, and cloud account infrastructure.</p>
            </description>
            <prop name="method" value="EXAMINE"/>
            <step uuid="733e3cbf-e398-46b6-9c02-a2cb534c341e">
                <title>Obtain Network Access via VPN to IFA GoodRead Environment</title>
                <description>
                    <p>The assessor will obtain network access with appropriately configured VPN account to see admin frontend to the application for PAO staff, which is only accessible via VPN with an appropriately configured role for PAO staff accounts.</p>
                </description>
            </step>
            <step uuid="4ce7e0b4-d69e-4b80-a700-8600b4d4d933">
                <title>Obtain Credentials and Access to AwesomeCloud Account for IFA GoodRead System</title>
                <description>
                    <p>The assessor will obtain access to the GoodRead Product Team's AwesomeCloud account with their single sign-on credentials to a read-only assessor role.</p>
                </description>
            </step>
            <step uuid="3d0297de-e47b-4360-b9c3-cf5c425f86cd">
                <title>Obtain Applcation Access Provided by Product Team</title>
                <description>
                    <p>The assessor will obtain non-privileged account credentials with the PAO staff role to test this role in the application does not permit excessive administrative operations.</p>
                </description>
            </step>
            <step uuid="64ca1ef6-3ad4-4747-97c6-40890222463f">
                <title>Confirm Load Balancer Blocks Access to Admin Frontend from Internet</title>
                <description>
                    <p>The assessor will confirm that the load balancer for public access does not allow access to Admin Frontend of the application from the Internet.</p>
                </description>
            </step>
            <step uuid="715f0592-166f-44f6-bb66-d99623e035dc">
                <title>Confirm GoodRead's PAO Role Cannot Manage Users</title>
                <description>
                    <p>The assessor will confirm that user's logged into the GoodRead Application with the PAO staff role cannot add, modify, or disable users from the system.</p>
                </description>
            </step>
            <step uuid="4641957b-a0fa-4c61-af1a-d3e9101efe40">
                <title>Confirm Django Admin Panel Not Available</title>
                <description>
                    <p>The assessor will confirm with web-based interface and API methods users with the PAO Staff role cannot access the Django admin panel functions and interactively change application's database records.</p>
                </description>
            </step>
            <related-controls>
                <control-selection>
                    <include-control control-id="ac-6.1"/>
                </control-selection>
            </related-controls>
            <responsible-role role-id="assessor">
                <party-uuid>e7730080-71ce-4b20-bec4-84f33136fd58</party-uuid>
            </responsible-role>
        </activity>
    </local-definitions>
    <reviewed-controls>
        <control-selection>
            <include-control control-id="ac-6.1"/>
        </control-selection>
        <control-objective-selection>
            <include-all/>
        </control-objective-selection>
    </reviewed-controls>
    <assessment-subject type="component">
        <description>
            <p>The assessor for the IFA GoodRead Project, including the application and infrastructure for this information system, are within scope of this assessment.</p>
        </description>
        <include-all/>
    </assessment-subject>
    <task uuid="b3504d22-0e75-4dd7-9247-618661beba4e" type="action">
        <title>Examine Least Privilege Design and Implementation</title>
        <associated-activity activity-uuid="0d243b23-a889-478f-9716-6d4870e56209">
            <subject type="component">
                <include-all/>
            </subject>
        </associated-activity>
        <responsible-role role-id="assessor"/>
        <remarks>
            <p>Per IFA's use of NIST SP-800 53A, the assessor, with the support of the owner, information system security officer, and product team for the IFA GoodRead project, will examine least privilege design and implementation with the following:</p>
            <ul>
                <li>list of security functions (deployed in hardware, software, and firmware) and security-relevant information for which access must be explicitly authorized;</li>
                <li>system configuration settings and associated documentation;</li>
            </ul>
        </remarks>
    </task>
</assessment-plan>

RETURNS:

[ERROR] [/assessment-plan/local-definitions[1]/objectives-and-methods[1]/part[1]/prop[1]/@name] Value 'method-id' doesn't match one of 'alt-identifier, label, marking, or sort-id' at path '/assessment-plan/local-definitions[1]/objectives-and-methods[1]/part[1]/prop[1]/@name'
[ERROR] [/assessment-plan/local-definitions[1]/objectives-and-methods[1]/part[2]/prop[1]/@name] Value 'method' doesn't match one of 'alt-identifier, label, marking, or sort-id' at path '/assessment-plan/local-definitions[1]/objectives-and-methods[1]/part[2]/prop[1]/@name'

When an attempt is made to fix the errors as shown below (even if it makes no sense from the data type perspective):

           <part id="ac-6.1_obj" name="assessment-objective">
                <!-- prop name="method-id" value="some-objective-value" -->
                <prop name="marking" value="some-objective-value">
                    <remarks>
                        <p>some obj text</p>
                    </remarks>
                </prop>
                <p>some more obj info</p>
            </part>
            <part id="ac-6.1_mtd" name="assessment-method">
                <!-- prop name="method" value="some-value" -->
                <prop name="marking" value="some-value">
                    <remarks>
                        <p>some text</p>
                    </remarks>
                </prop>
                <p>some more info</p>
                <part id="ac-6.1_obj" name="assessment-objects"> 
                </part>
            </part>

the following errors are returned:

[ERROR] [/assessment-plan/local-definitions[1]/objectives-and-methods[1]] The cardinality '0' is below the required minimum '1' for items matching the expression 'part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('assessment','assessment-method')]/prop[has-oscal-namespace(('http://csrc.nist.gov/ns/oscal','http://csrc.nist.gov/ns/rmf')) and @name='method']'.
[ERROR] [/assessment-plan/local-definitions[1]/objectives-and-methods[1]] The cardinality '0' is below the required minimum '1' for items matching the expression 'part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]/prop[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name='method-id']'.

A discussion on this topic exists here:
#1968

Who is the bug affecting

All SAP developers

What is affected by this bug

OSCAL Content, Modeling

How do we replicate this issue

Run validation on the provided SAP sample .

Expected behavior (i.e. solution)

To provide correct constraints

Other comments

n/a

Revisions

No response

@iMichaela
Copy link
Contributor Author

I asked the community for input on this issue and no preference on how to address it was provided from the community. Since a patch v1.1.3 is being prepared and this bug needs to be addressed, I'll remove both conflicting constraints and who needs them can enforce them as external constraints.

@aj-stein-gsa
Copy link
Contributor

OK so to confirm, you still want feedback on this issue and no change has been made in a developer repository? (That could be with or without a pull request and I could not find anything, that is why I am asking to make sure I clearly understand the ask here.)

@brian-ruf
Copy link
Contributor

@iMichaela it is unclear to me what prop[@name='method-id'] is intended to reference.

I find no documentation on its purpose, although I see where it has a cardinality of exactly 1. It does not show in any allowed-values list where it would normally have some type of description.

There is a 'method' property in the catalog syntax which has an optional uuid flag, and while this property appears many times in the NIST 800-53 catalog, it never has an ID/UUID.

So I believe it is under-defined/under-documented. At a minimum, the cardinality enforcement of the property should be removed until it is better documented and exercised.

@brian-ruf
Copy link
Contributor

brian-ruf commented Nov 14, 2024

The allowed values listed in the error ('alt-identifier, label, marking, sort-id') are the generic allowed values made available to every OSCAL prop.

This portion of the content should have the same structure and constraints as /catalog//control/part[@name=('objective' or 'assessment-objective')], arguably with some additional properties or constraints as may be appropriate in the context of AP/local-definitions.

            <part id="ac-6.1_obj" name="assessment-objective">
                <prop name="method-id" value="some-objective-value">
                    <remarks>
                        <p>some obj text</p>
                    </remarks>
                </prop>
                <p>some more obj info</p>
            </part>
            <part id="ac-6.1_mtd" name="assessment-method">
                <prop name="method" value="some-value">
                    <remarks>
                        <p>some text</p>
                    </remarks>
                </prop>
                <p>some more info</p>
                <part id="ac-6.1_obj" name="assessment-objects"> 
                </part>
            </part>

I do not see any indication that the catalog

@brian-ruf
Copy link
Contributor

THIS is incorrect:

      <constraint>
         <allowed-values target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal')]/@name">
            <enum value="objective" deprecated="1.0.1">**(deprecated)** Use 'assessment-objective' instead.</enum>
            <enum value="assessment" deprecated="1.0.1">**(deprecated)** Use 'assessment-method' instead.</enum>
            <enum value="assessment-objective">The part defines an assessment objective.</enum>
            <enum value="assessment-method">The part defines an assessment method.</enum>
         </allowed-values>
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('assessment','assessment-method')]/prop[has-oscal-namespace(('http://csrc.nist.gov/ns/oscal','http://csrc.nist.gov/ns/rmf')) and @name='method']" min-occurs="1" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('assessment','assessment-method')]/part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objects','assessment-objects')]" min-occurs="1" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]/prop[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name='method-id']" min-occurs="1" />
      </constraint>

"assessment" is not the depreciated allowed value. "method" is.
The 2nd enum and the 2nd and 3rd has-cardinality need to be updated to replace "assessment" with "method" (or 'assessment' with 'method' - single quotes instead of double quotes.)

@brian-ruf
Copy link
Contributor

brian-ruf commented Nov 14, 2024

It appears that many of the rules for control syntax are defined in the catalog metaschema instead of the controls-common metaschema.

Most of these constraints also apply to profiles (//modify/alter/add) as well as AP/AR local-definitions, but are only applied to catalogs.

I believe the correct resolution is to move these constraints from the control assembly definition in catalog metaschema into the oscal_control-common_metaschema, and then apply them appropriately in catalog, profile, AP/LD and AR/LD.

@david-waltermire
Copy link
Contributor

david-waltermire commented Nov 15, 2024

The problem with applying these to alter in profiles is that alter is cumulative, meaning a statement added by a previous alter can be altered again. This creates a case where it may be difficult to write a constraint that checks this based on the source content, as the intermediate state is not exposed to the constraint. It's easier and probably better to apply the constraints when validating the resulting catalog.

@iMichaela
Copy link
Contributor Author

THIS is incorrect:

      <constraint>
         <allowed-values target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal')]/@name">
            <enum value="objective" deprecated="1.0.1">**(deprecated)** Use 'assessment-objective' instead.</enum>
            <enum value="assessment" deprecated="1.0.1">**(deprecated)** Use 'assessment-method' instead.</enum>
            <enum value="assessment-objective">The part defines an assessment objective.</enum>
            <enum value="assessment-method">The part defines an assessment method.</enum>
         </allowed-values>
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('assessment','assessment-method')]/prop[has-oscal-namespace(('http://csrc.nist.gov/ns/oscal','http://csrc.nist.gov/ns/rmf')) and @name='method']" min-occurs="1" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('assessment','assessment-method')]/part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objects','assessment-objects')]" min-occurs="1" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]/prop[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name='method-id']" min-occurs="1" />
      </constraint>

"assessment" is not the depreciated allowed value. "method" is. The 2nd enum and the 2nd and 3rd has-cardinality need to be updated to replace "assessment" with "method" (or 'assessment' with 'method' - single quotes instead of double quotes.)

I am summarizing here the recommended direction to fix the bug:

  1. constraints will be corrected as follows:
      <constraint>
         <allowed-values target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal')]/@name">
            <enum value="objective" deprecated="1.0.1">**(deprecated)** Use 'assessment-objective' instead.</enum>
            <enum value="method" deprecated="1.0.1">**(deprecated)** Use 'assessment-method' instead.</enum>
            <enum value="assessment-objective">The part defines an assessment objective.</enum>
            <enum value="assessment-method">The part defines an assessment method.</enum>
         </allowed-values>
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('method','assessment-method')]/prop[has-oscal-namespace(('http://csrc.nist.gov/ns/oscal','http://csrc.nist.gov/ns/rmf')) and @name='method']" min-occurs="1" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('method','assessment-method')]/part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objects','assessment-objects')]" min-occurs="1" max-occurs="1" />
         <has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]/prop[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name='method-id']" min-occurs="1" />
      </constraint>
  1. the constraints above will be moved from catalog metaschema to the controls-common metaschema, so profiles are importing them too.

@david-waltermire
Copy link
Contributor

I would prefer to comment on a PR that actually shows the changes. As I mentioned before, I believe that introducing these constraints in the profile, will cause problems. If I have a PR with these changes, I can make a test case that illustrates this. When will a PR be available to comment on?

@wandmagic
Copy link
Collaborator

if it helps,

<has-cardinality target="part[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name=('objective','assessment-objective')]/prop[has-oscal-namespace('http://csrc.nist.gov/ns/oscal') and @name='method-id']" min-occurs="1" />

i believe this constraint is conflicting with another constraint

@david-waltermire
Copy link
Contributor

@wandmagic Which other constraint is it conflicting with?

@wandmagic wandmagic mentioned this issue Nov 15, 2024
9 tasks
@aj-stein-gsa
Copy link
Contributor

@david-waltermire I was pairing with @wandmagic and have some questions, but this solution resolves constraint violations but could use some review, even from our side of the house.

#2076

@wandmagic
Copy link
Collaborator

wandmagic commented Nov 15, 2024

it conflicts with

<!ENTITY allowed-values-control-group-property-name SYSTEM "./shared-constraints/allowed-values-control-group-property-name.ent">

@brian-ruf
Copy link
Contributor

The caution from @david-waltermire is fair, and I hadn't considered that when I was looking at the application of constraints.

While I stand by my recommendation of moving the constraint rules into the controls-common, I also agree that any application of those constraints must be surgical, and not equally applied to everyplace that might create or manipulate control content.

@david-waltermire
Copy link
Contributor

The caution from @david-waltermire is fair, and I hadn't considered that when I was looking at the application of constraints.

While I stand by my recommendation of moving the constraint rules into the controls-common, I also agree that any application of those constraints must be surgical, and not equally applied to everyplace that might create or manipulate control content.

Just to be clear about my prior concerns, the surgical thing to do is fix them in the catalog model where they are. If they are moved to control common they will also apply to the profile model, which will likely cause breakage due to how alter statements work.

In my view, the best course of action here is to hold this issue back from the patch release to allow for more exploration and consensus on the approach. This can be released in a later patch or minor release. I believe this is what @aj-stein-gsa was pushing for by leaving this out of his change proposal for 1.1.3.

@wandmagic
Copy link
Collaborator

The caution from @david-waltermire is fair, and I hadn't considered that when I was looking at the application of constraints.
While I stand by my recommendation of moving the constraint rules into the controls-common, I also agree that any application of those constraints must be surgical, and not equally applied to everyplace that might create or manipulate control content.

Just to be clear about my prior concerns, the surgical thing to do is fix them in the catalog model where they are. If they are moved to control common they will also apply to the profile model, which will likely cause breakage due to how alter statements work.

In my view, the best course of action here is to hold this issue back from the patch release to allow for more exploration and consensus on the approach. This can be released in a later patch or minor release. I believe this is what @aj-stein-gsa was pushing for by leaving this out of his change proposal for 1.1.3.

sounds good to me, i'm happy to implement the change once we decide what we want

@iMichaela
Copy link
Contributor Author

I agree with @david-waltermire, and as long as FedRAMP is aware of the bug, I can release 1.1.3 without it, BUT this bug has been raised to us (NIST) several times and we tried to get community's attention to agreeing to a direction for the fix. Since external constraints is a desired direction, we can look into it after 1.1.3, with high priority. The solution will have to be backwards compatible (like simply removing them from the models and relocating and address the conflicting constraints). Community's agreement, examples and testing are a must.

@david-waltermire
Copy link
Contributor

We have developed a solid test harness for testing external constraints at FedRAMP. We would be glad to offer tuning up the OSCAL repo CI/CD with some of these improvements, including integrating the test harness for external OSCAL constraints. If NIST is amenable, we could make some PRs to do this.

@iMichaela
Copy link
Contributor Author

We have developed a solid test harness for testing external constraints at FedRAMP. We would be glad to offer tuning up the OSCAL repo CI/CD with some of these improvements, including integrating the test harness for external OSCAL constraints. If NIST is amenable, we could make some PRs to do this.

Thank you, We appreciate the support and we would like to work with the FedRAMP team as long as the harness is aligned with and is pushed to usnistgov OSCA-related repositories. There is also a timing concern - when to move the existing constraints (general ones, RMF, FedRAMP) outside the OSCAL models. The community impact will have to be analyzed first, and we will determine, in collaboration with the community members, when and how we will proceed. I envision huge impact on the entities that are not using the OSCAL schemas but rather parse the metaschema definitions to implement the constraints too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Needs Triage
Development

No branches or pull requests

5 participants