-
Notifications
You must be signed in to change notification settings - Fork 222
Fix validation constraints to properly deference and key on uri-reference values #2107
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
base: develop
Are you sure you want to change the base?
Fix validation constraints to properly deference and key on uri-reference values #2107
Conversation
|
nice work! this is true to the intention of the constraints and in line with previous examples self referential links prefixed by a hash. |
|
@iMichaela have you had any time to review this proposed change? It does fix an overly generalized constraint and fixing it will help others, not just FedRAMP. We can work around this issue in the core models, but it means we have to make confusing duplicate requirements with special FedRAMP namespacing because upstream bugfixes if it cannot be accepted. Thanks. |
| </index> | ||
| <index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by']"> | ||
| <key-field target="@href"/> | ||
| <index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by' and starts-with(@href,'#')]"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: This constraint is applied to validate-by which has been changed /replaced 4+ years ago with validate . Unfortunately the change was not propagated everywhere. See src/metaschema/shared-constraints/allowed-values-component_component_link-rel.ent file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entity file can define values for a target of allowed-values, but not the target itself, which is what your comment describes. Can you explain what this means?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to explain again what I see happening here:
In the file: src/metaschema/shared-constraints/allowed-values-component_component_link-rel.ent
the following shared constraints are listed:
<enum xmlns="http://csrc.nist.gov/ns/oscal/metaschema/1.0" value="depends-on">A reference to another component that this component has a dependency on.</enum>
<!-- CHANGED (BJR): validated-by to validation per https://github.com/usnistgov/OSCAL/blob/metaschema-m4-integration/docs/content/documentation/schema/implementation-layer/validation-modeling.md -->
<enum xmlns="http://csrc.nist.gov/ns/oscal/metaschema/1.0" value="validation">A reference to another component of component-type=validation, that is a validation (e.g., FIPS 140-2) for this component</enum>
<enum xmlns="http://csrc.nist.gov/ns/oscal/metaschema/1.0" value="proof-of-compliance">A pointer to a validation record (e.g., FIPS 140-2) or other compliance information.</enum>
<!-- rel values to also be allowed in implemented-component -->
<enum xmlns="http://csrc.nist.gov/ns/oscal/metaschema/1.0" value="baseline-template">A reference to the baseline template used to configure the asset.</enum>
<enum xmlns="http://csrc.nist.gov/ns/oscal/metaschema/1.0" value="uses-service">This service is used by the referenced component identifier.</enum>
<enum xmlns="http://csrc.nist.gov/ns/oscal/metaschema/1.0" value="system-security-plan">A link to the system security plan of the external system.</enum>Please note the comment about replacing validated-by with validation for the link/@rel allowed values.
See the reference:
We have a "validation" naming clash between the component/@type="validation" and link/@rel="validation". It is confusing but not a bug.
The bug is in the constraint definition because it is using validated-by which does not exist. In the image of the reference above, there is NO allowed value validated-by
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what @iMichaela is saying here and agree. The changes @aj-stein-gsa proposed are essentially correct in that the constraints needed to be updated to look for the leading #; however, an additional change is required on new line number 621.
At minimum, this:
<index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by' and starts-with(@href,'#')]">
Must be changed to this:
<index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated' and starts-with(@href,'#')]">
We should also consider changing the key ID from oscal-system-implementation-validated-by-index to oscal-system-implementation-validated-index
|
@aj-stein-gsa - I agree with the fact that the faulty constraints are indicating the Can you please help us understand:
Please note that the snippet of OSCAL SSP provided in the issue #2106 is taken from the validation test tutorial and it si valid per <component uuid="11111111-0000-4000-a000-000000000001" type="hardware">
<title>Product Name</title>
<description>
<p>Describe the product's function.</p>
</description>
<link rel="validation" href="#22222222-0000-4000-a000-000000000002" />
<status state="operational" />
</component>
<component uuid="22222222-0000-4000-a000-000000000002" type="validation">
<title>Validation Name</title>
<description>
<p>Describe the validation.</p>
</description>
<prop name="validation-type" value="fips-140-2" />
<prop name="validation-reference" value="xxxx" />
<link rel="validation-details" href="https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/xxxx" />
<status state="operational" />
</component>The faulty constraint definitions are not exercised since it is not using Errors are returned when validating an SSP with the following information: <component uuid="11111111-0000-4000-a000-000000000001" type="hardware">
<title>Product Name</title>
<description>
<p>Describe the product's function.</p>
</description>
<link rel="validation" href="#22222222-0000-4000-a000-000000000002" />
<status state="operational" />
</component>
<component uuid="22222222-0000-4000-a000-000000000002" type="validation">
<title>Validation Name</title>
<description>
<p>Describe the validation.</p>
</description>
<prop name="validation-type" value="fips-140-2" />
<prop name="validation-reference" value="xxxx" />
<link rel="proof-of-concept" href="https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/xxxx" />
<status state="operational" />
</component>
I also realized that the |
I am not sure if we want to expand scope to 2 in this conversation, but I guess that is an alternative approach, but further strays from the tutorial you reference on test validation modeling. Re this PR and the whole issue at hand, the reason for the changes to indices values with the lookup is they, per the specification, read into a caching index. Those should only occur intra-document, as external URI values (which are acceptable, but not indexable) are not looked up intra-document, those values don't make sense. The proposed change ignores non-uri-reference values and formats the appropriate ones, implementing the specific intent of the constraint. There are other constraints like this in the models, and I adjust to follow the pattern missed here extant in several others. Essentially you can throw away the If you would like me to go over the relevant Metaschema specification and examples outside of this context, I am happy to do so.
The referenced snippets look identical, is there something I am missing or misunderstood here?
Can you confirm this proposal is what you intend or use the GitHub suggestion feature to precisely identify the change you want? Your request does not align with the documentation, there is no |
My understanding is that you identified the snippet you provided in the #2106 as not being valid and you want it to be valid. The snippet appears to be identical with the tutorial's example and I got no validation error with oscal-cli 1.0.3 . The tutorial sample is not using @link/rel="proof-of-compliance" or |
Putting aside the |
That was a typo due to 2:00AM time. It should be `component[@type="validation"'.
|
|
@aj-stein-gsa (sorry I forgot to ping you) -- I reviewed the documentation we currently have, the issues opened and the PR comments, and created the example below that is valid per oscal-cli v1.0.3. I think the example demonstrates what you want to accomplish. <component uuid="11111111-0000-4000-a000-000000000001" type="hardware">
<title>Product Name</title>
<description>
<p>Describe the product's function.</p>
</description>
<link rel="validation" href="#22222222-0000-4000-a000-000000000002" />
<status state="operational" />
</component>
<component uuid="22222222-0000-4000-a000-000000000002" type="validation">
<title>Validation Name</title>
<description>
<p>Describe the validation.</p>
</description>
<prop name="validation-type" value="fips-140-2" />
<prop name="validation-reference" value="xxxx" />
<link rel="proof-of-compliance" href="#33333333-0000-4000-a000-000000000003" />
<status state="operational" />
</component>
...
<back-matter>
<resource uuid="33333333-0000-4000-a000-000000000003">
<description>
<p>FIPS 140-2 certificate.</p>
</description>
<rlink href="https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/xxxx"/>
</resource>
</back-matter>If the example illustrates what you want to accomplish, maybe all we/I need to do is to correct the bug and replace <index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by' and starts-with(@href,'#')]">
<key-field target="@href" pattern="#(.*)"/>
</index-has-key>with: <index-has-key id="oscal-system-implementation-validation-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validation' ]">
<key-field target="@href" />
</index-has-key>@aj-stein-gsa -- Based on the example above, I have a question I would like to resurface: If FedRAMP wants to enforce url-references, can FedRAMP include the constraint locally? We can then keep core OSCAL without additional constraints that might cause backwards compatibility issues for the community. |
I apologize for the delayed reply. I was hoping if I did not distract from the highest priority PR for which I have asked for expedited review (2111), it would keep us focused on that. I should have followed up on this PR sooner. To wit: if you do not focus this constraint and not correct it as I propose, anyone who uses the props for which I amended constraints will receive this error for a specification conformant tool when they use any |
|
SUM:
If the community members are OK with enforcing the @herf to be a relative URI reference which points to the This is where the OSCAL Foundation can help provide feedback that captures the view of ALL OSCAL constituents, hence bringing this PR for discussion there. NOTE: NIST will proceed In the meantime with correcting the bug revealed while creating the example above, and replace <index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by' and starts-with(@href,'#')]">
<key-field target="@href" pattern="#(.*)"/>
</index-has-key>with: <index-has-key id="oscal-system-implementation-validation-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validation' ]">
<key-field target="@href" />
</index-has-key> |
Changed `validated-by` to `validated` in both the `@id` and `@target`
brian-ruf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
| </index> | ||
| <index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by']"> | ||
| <key-field target="@href"/> | ||
| <index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by' and starts-with(@href,'#')]"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what @iMichaela is saying here and agree. The changes @aj-stein-gsa proposed are essentially correct in that the constraints needed to be updated to look for the leading #; however, an additional change is required on new line number 621.
At minimum, this:
<index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated-by' and starts-with(@href,'#')]">
Must be changed to this:
<index-has-key id="oscal-system-implementation-validated-by-index" name="index-system-implementation-component-uuid-validation" target="component/link[@rel='validated' and starts-with(@href,'#')]">
We should also consider changing the key ID from oscal-system-implementation-validated-by-index to oscal-system-implementation-validated-index

Committer Notes
Closes #2102 and closes #2106.
All Submissions:
By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.
(For reviewers: The wiki has guidance on code review and overall issue review for completeness.)
Changes to Core Features:
Have you written new tests for your core changes, as applicable?Have you included examples of how to use your new feature(s)?Have you updated the OSCAL website and readme documentation affected by the changes you made? Changes to the OSCAL website can be made in the OSCAL-Pages and OSCAL_Reference repositories.