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

Given all in-model constraints IDs #2090

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

aj-stein-gsa
Copy link
Contributor

@aj-stein-gsa aj-stein-gsa commented Dec 15, 2024

Committer Notes

This PR closes #2088 once reviewed, approved, and merged.

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 added an explanation of what your changes do and why you'd like us to include them?
  • 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. These changes will not be used in the document generation pipeline unless the current metaschema-xslt and oscal-xslt are adapted to generate references to the identifiers of constraints, but at this time the pipelines do not act on them and they are open flags for Metaschema definitions.

@aj-stein-gsa aj-stein-gsa force-pushed the 2088-constraint-ids branch 2 times, most recently from 27eb4d7 to 4848333 Compare January 21, 2025 01:31
@aj-stein-gsa aj-stein-gsa changed the title [WIP] Add common assessment constraint IDs for #2088 Given all in-model constraints IDs Jan 21, 2025
@aj-stein-gsa aj-stein-gsa marked this pull request as ready for review January 21, 2025 04:11
@aj-stein-gsa aj-stein-gsa requested a review from a team as a code owner January 21, 2025 04:11
@iMichaela iMichaela mentioned this pull request Jan 21, 2025
9 tasks
@iMichaela
Copy link
Contributor

@wendellpiez - can you please assist with this PR. Robust profile resolution testing must be part of this PR.

@aj-stein-gsa
Copy link
Contributor Author

aj-stein-gsa commented Jan 21, 2025

I saw your comment in the other PR, @iMichaela, which does not seem particularly related. What is the impact and how will we test adding constraint IDs break profile resolution? The constraints conceptually operate separate of constraints, no?

@wendellpiez
Copy link
Contributor

With @aj-stein-gsa I wonder why profile resolution is implicated here.

@aj-stein-gsa
Copy link
Contributor Author

aj-stein-gsa commented Jan 21, 2025

With @aj-stein-gsa I wonder why profile resolution is implicated here.

It came up in another comment in #2095 (comment) from @iMichaela, not I. It is why I wanted clarification.

Thank you. Will prioritize this work. Profile resolution testing is also a concern for #2090 . @wendellpiez - can you please assist with this PR?

I wanted to redirect that conversation here as it summarized possible concerns about this PR.

EDIT: Updated to include the specific context from #2095, not just the top-level PR reference.

@aj-stein-gsa
Copy link
Contributor Author

Sorry, I accidentally posted this to #2095 thinking it was this PR (#2090 if you are reading email).

We have a way of computing all the constraints and targets in liboscal-java, and weirdly just adding the constraint IDs caused some exceptions. That result is separate of the low-risk changes, but is interesting because it does mean I may have duplicated or missed something. So on that note, I will update will relevant information of that analysis.

If NIST staff or community members want more details about methodology or the use of a consisten prefix (id="oscal-...), I am more than willing to discuss that. Let me know where, when, and how you would like that information.

@iMichaela
Copy link
Contributor

iMichaela commented Jan 21, 2025

With @aj-stein-gsa I wonder why profile resolution is implicated here.

To make sure that when a profile is resolved, the IDs for constraints are carried on into the resolved profile correctly. I am not saying there is an issue, but rather wanted to make sure the profile resolver(s) oscal-cli and XSLT transformations work well, so the oscal-content can be updated and released.

@aj-stein-gsa
Copy link
Contributor Author

To make sure that when a profile is resolved, the IDs for constraints are carried on into the resolved profile correctly.

I have not touched the XSLT documentation pipeline in three to six months, but such a feature would be new and require additional development. There resulting constraint information is rendered into usnistgov/OSCAL-Reference, but not IDs.

You can evidence by the fact a small minority of constraints, such as data-flow/diagram have had their own @ids for some time (going back at least two releases) and are not generated in the extant documentation. I do not see any output generation for is-unique at all for the relevant assembly or children assemblies, fields, or flags.

https://github.com/usnistgov/OSCAL/blob/v1.1.3/src/metaschema/oscal_ssp_metaschema.xml#L517C18-L522

https://pages.nist.gov/OSCAL-Reference/models/v1.1.3/system-security-plan/xml-reference/#/system-security-plan/system-characteristics/data-flow/diagram

@wendellpiez
Copy link
Contributor

wendellpiez commented Jan 21, 2025

The affected elements (within constraint elements) are in the metaschemas, not in profiles or their sources (catalogs). Profile resolution should be unaffected.

@aj-stein-gsa
Copy link
Contributor Author

Sorry, I accidentally posted this to #2095 thinking it was this PR (#2090 if you are reading email).

We have a way of computing all the constraints and targets in liboscal-java, and weirdly just adding the constraint IDs caused some exceptions.

Re my commit message in c0eee8b, I have found the source of the error and it was accidental use of = versus - in a constraint ID. I am continuing with testing and will report further findings and if there is no further issues found on our end.

Again, @iMichaela or @wendellpiez, if you want a brief on the methodology or adhoc testing without sufficient automated testing in usnistgov/OSCAL, oscal-content, or related dependency repos, let us know. I am happy to summarize how I am doing this analysis for the benefit of you and others in the community, so others can follow suit.

@aj-stein-gsa
Copy link
Contributor Author

The affected elements (constraint elements) are in the metaschemas, not in profiles or their sources (catalogs). Profile resolution should be unaffected.

I have agreed several times, not sure how to make it more clear, but I did want to address a different but related issue I thought might be helpful to discuss. I can also test profile resolution but I am less concerned about that for that reason, @wendellpiez.

@aj-stein-gsa
Copy link
Contributor Author

Again, @iMichaela or @wendellpiez, if you want a brief on the methodology or adhoc testing without sufficient automated testing in usnistgov/OSCAL, oscal-content, or related dependency repos, let us know. I am happy to summarize how I am doing this analysis for the benefit of you and others in the community, so others can follow suit.

I was able to test constraint validation and profile resolution locally. I resolved profiles, both the NIST and FedRAMP profiles without issue. I cannot run the documentation generation pipeline in GitHub Actions from your org to do a full end-to-end test there, but this appears to be ready for review and possible merge.

Copy link
Contributor

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

With the qualification that I have not run the build or inspected outputs, I'm approving the PR. Having scanned it, it seems to me any problems would have been exposed and repaired, or will be exposed in CI/CD. TMK we have no resources that hardcode these ID values to point into the constraints, which would break. Almost by definition any other emergent problem would be a new bug or requirement discovery (good things). @aj-stein-gsa thanks for this!

@aj-stein-gsa
Copy link
Contributor Author

To circle back on this issue, is there anything we can do to assist with coordinating a review and possible merge?

@wandmagic wandmagic added the Developer Experience Issues around enhancing and optimizing work for development of NIST OSCAL artifacts label Jan 24, 2025
@wandmagic
Copy link
Collaborator

see the test harness against main (illustrating lack of constraint Id's)
https://github.com/metaschema-framework/oscal-test-harness/actions/runs/12951179612/job/36125648541

see test harness running against this PR (no style errors)
https://github.com/metaschema-framework/oscal-test-harness/actions/runs/12951464676/job/36126568337

@aj-stein-gsa
Copy link
Contributor Author

So given the PR and supporting testing from @wandmagic, can we get more precise reviews, @iMichaela or @wendellpiez?

Just a heads up, @iMichaela, when you use the assignee field as the person who made the PR, that person cannot review the same PR unless you set up admin overrides. I have been bitten by this mistake a few times and it will limit your ability to review, so FYI.

wendellpiez
wendellpiez previously approved these changes Jan 27, 2025
Copy link
Contributor

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

Looking over, I am seeing only changes to IDs plus one place (oscal-ssp-metaschema.xml) where a rule has been removed. If I'm missing anything important @aj-stein-gsa please let me know.

@iMichaela iMichaela removed their assignment Jan 27, 2025
@iMichaela
Copy link
Contributor

So given the PR and supporting testing from @wandmagic, can we get more precise reviews, @iMichaela or @wendellpiez?

Just a heads up, @iMichaela, when you use the assignee field as the person who made the PR, that person cannot review the same PR unless you set up admin overrides. I have been bitten by this mistake a few times and it will limit your ability to review, so FYI.

I never assign myself ... It must be an error. I did ask Wendell to review. I will be reviewing it today.

@aj-stein-gsa
Copy link
Contributor Author

aj-stein-gsa commented Jan 27, 2025

I never assign myself ... It must be an error. I did ask Wendell to review. I will be reviewing it today.

No worries, it has happened to me several times and then approval button went away and I got very confused. Thanks for moving this review forward, @iMichaela!

@aj-stein-gsa
Copy link
Contributor Author

aj-stein-gsa commented Feb 4, 2025

I will rebase this PR, but any updates on the review thus far, @iMichaela?

@iMichaela
Copy link
Contributor

@aj-stein-gsa I started the review... It is next in my queue ... Thank you

@iMichaela
Copy link
Contributor

Again, @iMichaela or @wendellpiez, if you want a brief on the methodology or adhoc testing without sufficient automated testing in usnistgov/OSCAL, oscal-content, or related dependency repos, let us know. I am happy to summarize how I am doing this analysis for the benefit of you and others in the community, so others can follow suit.

Sure, @aj-stein-gsa . How would you like to brief us and the community on the use of the constraints' IDs?
I can host a virtual meeting for us and the interested members of the community, if this is what you had in mind.

@aj-stein-gsa
Copy link
Contributor Author

aj-stein-gsa commented Feb 5, 2025

Again, @iMichaela or @wendellpiez, if you want a brief on the methodology or adhoc testing without sufficient automated testing in usnistgov/OSCAL, oscal-content, or related dependency repos, let us know. I am happy to summarize how I am doing this analysis for the benefit of you and others in the community, so others can follow suit.

Sure, @aj-stein-gsa . How would you like to brief us and the community on the use of the constraints' IDs? I can host a virtual meeting for us and the interested members of the community, if this is what you had in mind.

I would prefer to do things in writing and not consume a lot of time on virtual presentation. If it is a presentation, I will have to clear it and get approval. Is that ok for now?

During testing with metaschema-framework/{metaschema-java,liboscal-java,
oscal-cli} harness, I was able to detect that I had erroneously prefixed
a constraint ID with `oscal=`, not the intended `oscal-` prefix, and
constraint builder function utilities picked this mistake up.
The constraint with id="oscal-system-implementation-validated-by-index"
had its corresponding index name erased in error, making this
index-has-key constraint inoperable by tools conformant with the current
version of the Metaschema spec. This change restores it back.
@aj-stein-gsa
Copy link
Contributor Author

Also apologies for the delays, @iMichaela, I have rebased the PR and re-review and retest on my end in the morning.

@iMichaela
Copy link
Contributor

Again, @iMichaela or @wendellpiez, if you want a brief on the methodology or adhoc testing without sufficient automated testing in usnistgov/OSCAL, oscal-content, or related dependency repos, let us know. I am happy to summarize how I am doing this analysis for the benefit of you and others in the community, so others can follow suit.

Sure, @aj-stein-gsa . How would you like to brief us and the community on the use of the constraints' IDs? I can host a virtual meeting for us and the interested members of the community, if this is what you had in mind.

I would prefer to do things in writing and not consume a lot of time on virtual presentation. If it is a presentation, I will have to clear it and get approval. Is that ok for now?

Yes. I understand. Verba volant, scripta manent :) So in writing it is.

@aj-stein-gsa
Copy link
Contributor Author

So it seems GitHub Actions test failures were with my fork, but it was only Debian runner installation and on the upstream runners here it worked, so this is looking good.

@wandmagic
Copy link
Collaborator

this will improve output on the application level for oscal, looking forward to seeing this in a future release!

@iMichaela
Copy link
Contributor

So it seems GitHub Actions test failures were with my fork, but it was only Debian runner installation and on the upstream runners here it worked, so this is looking good.

Yes, one repo was responding late and was timing out. It worked this AM when I re-ran the job.
I will test locally with act and then ensure the models are able to be used for 800-53 and examples validation and will them merge the PR if no issues will surface. Thank you @aj-stein-gsa and @wandmagic

@iMichaela
Copy link
Contributor

@aj-stein-gsa, @wandmagic and @Telos- I tested the PR and I am ready to approve it, but I still need some clarification to ensure the purpose of the #2088 is addressed. While the IDs for constraints exist now in the OSCAL metaschema definition files, they are not propagated into any XSD or JSON schemas, so unless a user is implementing OSCAL based on the metaschema definitions, those IDs are useless to the community. Just extra maintenance ...

I will wait for your explanation of the vision fro those IDs before I'll approve and merge the PR.

In case the intension is to propagate the IDs to the XSD and JSON schemas, then the PR is not complete, because it is not accomplishing what and @telos use case described in the #2088 is not supported.

NOTE: If I move forward with other PRs towards develop before you provide some clarification, your branch will have to be rebased.

@wandmagic
Copy link
Collaborator

wandmagic commented Feb 6, 2025

The metaschema constraint processor we use for fedramp automation leverages the constraints directly and they are consumable via sarif validation artifacts. adjusting the official json and xsd schemas to consider constraint id information is not required as far as I understand it.

@iMichaela
Copy link
Contributor

The metaschema constraint processor we use for fedramp automation leverages the constraints directly and they are consumable via sarif validation artifacts. adjusting the official json and xsd schemas to consider constraint id information is not required as far as I understand it.

Thank you, @wandmagic . This means that, unless a GRC tool or platform is leveraging the OSCAL metaschema definitions, nobody else needs those IDs. And IF there are GRC tools/platforms using metaschema definitions, they will ignore the IDs today unless they implement the functionality to use them. Bottom line, this enhancement serves only FedRAMP process today. How can CSPs or agencies consume/benefit from the IDs? Just to be clear: I am not against including the IDs despite the extra maintenance work and the need to update also all prototype models since NIST is committed to support all our community members and FedRAMP has an important place, but all community members need to understand the positive, negative (if any) or neutral (zero) impact to their operationalization of OSCAL once I merge the PR.
@aj-stein-gsa promised a written an explanation of how he envisions the use of the IDs and I am keeping the PR ready to be approved until the community gets that explanation and reviews it.

@aj-stein-gsa
Copy link
Contributor Author

@aj-stein-gsa promised a written an explanation of how he envisions the use of the IDs and I am keeping the PR ready to be approved until the community gets that explanation and reviews it.

AJ was on sick leave the first half of the day and will get back to it shortly. I can write a longer explanation, but the people I worked on this for, who use both NIST content and FedRAMP content independently, and supported this work, are @Telos-sa and their colleagues.

If you would prefer to not receive this work, with or without explanation, let us know and I can publish it elsewhere. I can also help with other models in down time, but it may need to wait a day or two for those (before these core changes). I am open to whatever you request.

In the interim, I will wrap up other tasks and work on that explanation in a discussion post.

@wendellpiez
Copy link
Contributor

Hi - not speaking to the merits of including and improving IDs, as I think those are obvious and speak for themselves. (If needed for the record @aj-stein-gsa has offered to enumerate.) On the other side, all the arguments being made against giving good IDs to the constraints can also be made against including the constraints themselves. If the model includes constraint definitions at all, and wishes to encourage (not discourage) implementations, it will support them with strong, traceable IDs. (Just my 0.02.)

The one caution I will make is that while adding IDs is almost a no-brainer, changing IDs should be done as seldom as possible and never without care, since this can indeed cause problems downstream. (But I think this group understands that.)

@wendellpiez
Copy link
Contributor

Just adding that because you don't want to change them later, you do want to get them right before there are many implementations and downstream consumers ... --

@iMichaela
Copy link
Contributor

f you would prefer to not receive this work, with or without explanation, let us know and I can publish it elsewhere. I can also help with other models in down time, but it may need to wait a day or two for those (before these core changes). I am open to whatever you request.

@aj-stein-gsa I regret you have been seek. No pressure on providing your promised written explanation. Beyond that, I refuse to comment on your negative interpretation of my statement above: "Just to be clear: I am not against including the IDs despite the extra maintenance work and the need to update also all prototype models since NIST is committed to support all our community members and FedRAMP has an important place, but all community members need to understand the positive, negative (if any) or neutral (zero) impact to their operationalization of OSCAL once I merge the PR."
I called on @Telos-sa because they planned on using the IDs , but no review or comment were provided, to ensure those changes meet their expectations as well.

@aj-stein-gsa
Copy link
Contributor Author

OK, sorry for the delay. Explanation of background outside the scope of the PR is in #2101 to discuss further. I can also add demos of the software to make the points more clear. Let me know if that helps, @iMichaela.

@iMichaela
Copy link
Contributor

I consider this PR ready to be merged, and I will do so after 6PM ET today. All community members have a chance to review and comment on this PR only until 6PM Friday Feb 7. The PR has been opened for enough time now and AJ documented in the #2101 discussion all technical benefits envisioned by him as the author of this PR. Thank you AJ.

@aj-stein-gsa
Copy link
Contributor Author

The PR has been opened for enough time now and AJ documented in the #2101 discussion all technical benefits envisioned by him as the author of this PR. Thank you AJ.

Excellent, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Issues around enhancing and optimizing work for development of NIST OSCAL artifacts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants