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

Addresses bugs in 800-53 catalog (issues 224 226 227 for OSCAL content patch 1.2.1 #228

Merged
merged 10 commits into from
Dec 15, 2023

Conversation

iMichaela
Copy link
Contributor

@iMichaela iMichaela commented Dec 8, 2023

Committer Notes

The latest oscal-content release is 1.2.0, so this patch should be oscal-content v 1.2.1 and not 1.1.3.
(NOTE: OSCAL models latest release is OSCAL 1.1.2 and the patch was wrongly marked as 1.1.3)

Current PR addresses the following issues flagged as bugs in the 800-53 catalog:

Resolutions for those issues are documented in the issue's comments tab.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you squashed any non-relevant commits and commit messages? [instructions]
  • Do all automated CI/CD checks pass?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?

@iMichaela
Copy link
Contributor Author

iMichaela commented Dec 8, 2023

During the triage meeting on 12/07/2023, the team discussed issue #224 and expressed concerns that changing the OSCAL's control IDs to include leading 0s could break the implementation of current adopters, in particular FedRAMP that provides SSP templates to its users.

Since the RMF team (the owners of the 800-53 catalog of controls) announced officially that the 800-53 v5.1.1 'introduces “leading 0s” to the control identifiers' NIST is proposing to replace the labels of structure:

<prop name="label" value="IA-9"/>
<prop name="label" class="sp800-53a" value="IA-09"/>

with

<prop name="alt-identifier" class="sp800-53a" value="IA-09"/>
<prop name="alt-identifier" class="sp800-53a" value="IA-09"/>

Please provide feedback on the proposed approach no later than 12/08, EOB since the patch release with the bug fixed and official control identifier captured correctly is urgent. In the meantime we are proceeding with the proposal above.

@iMichaela iMichaela marked this pull request as draft December 8, 2023 00:22
@iMichaela iMichaela self-assigned this Dec 8, 2023
@iMichaela iMichaela requested a review from a team December 8, 2023 00:23
@Compton-US
Copy link

Just want to make sure I am not misinterpreting -- is the intention to update to this? (the alt-identifier tag appears to be duplicated above)

<prop name="label" value="IA-9"/>
<prop name="alt-identifier" class="sp800-53a" value="IA-09"/>

If this is the case, I think this fits with the allowed values, and retains the original element identifier found in CPRT.

@iMichaela
Copy link
Contributor Author

Just want to make sure I am not misinterpreting -- is the intention to update to this? (the alt-identifier tag appears to be duplicated above)

<prop name="label" value="IA-9"/>
<prop name="alt-identifier" class="sp800-53a" value="IA-09"/>

If this is the case, I think this fits with the allowed values, and retains the original element identifier found in CPRT.

I am working towards replacing constructs like (currently in the catalog):

<prop name="label" value="IA-9"/>

with

<prop name="alt-identifier" class="sp800-53a" value="IA-09"/>

to include the control identifier as CPRT 800-53 lists it,
AND the existing

<prop name="label" class="sp800-53a" value="IA-09"/>

will have the label replaced with alt-identifier for the same reason.

<prop name="alt-identifier" class="sp800-53a" value="IA-09"/>

@Compton-NIST -- I am working towards completing it today as well and push it as a separate commit so if changes are needed, we can address them.

@iMichaela iMichaela changed the title [WIP] Addresses bugs in 800-53 catalog (issues 224 226 227 for OSCAL content patch 1.1.3 Addresses bugs in 800-53 catalog (issues 224 226 227 for OSCAL content patch 1.1.3 Dec 11, 2023
@iMichaela iMichaela marked this pull request as ready for review December 11, 2023 03:49
@iMichaela
Copy link
Contributor Author

@wendellpiez - since you are the expert and author of the original OSCAL SP 800-53 catalog, please review for correctness beyond validation against the schema and constraints the updates addressing the reported bugs. Thank you.

@wendellpiez
Copy link
Contributor

wendellpiez commented Dec 12, 2023

I am proceeding to review the changes responding to bug reports, but clarification is needed with respect to the change from label to alt-identifier. Please provide the rationale for this as a comment to this ticket.

The explanation needs to be succinct and suitable for pasting directly into the change log, so that users can find it.

I wonder why more alternatives were not considered? (I can think of a couple.) Indeed I don't much like this change since it muddies the distinction between labels and identifiers (albeit an OSCAL distinction not an SP800-53 distinction), but doesn't actually add any information.

But more importantly, why community was not included in this conversation?

If you feel this risks derailing the PR, and if bug fixes need to be expedited, I suggest making those changes only in a separate PR.

Suggestion: use class='v5-0' or class='v5-1-1' to distinguish between "label" props in the data. Don't use alt-identifier for this:

<prop name="label" class="v5-0" value="IA-9"/>
<prop name="label" class="v5-1-1" value="IA-09"/>

Copy link
Contributor

@JustKuzya JustKuzya left a comment

Choose a reason for hiding this comment

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

After leading-zero changes one to PM 7 to 9, I have no objections

@iMichaela iMichaela changed the title Addresses bugs in 800-53 catalog (issues 224 226 227 for OSCAL content patch 1.1.3 Addresses bugs in 800-53 catalog (issues 224 226 227 for OSCAL content patch 1.2.1 Dec 12, 2023
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.

I believe the proposed change to control properties has not been sufficiently discussed with users, both whether a change is warranted and what the change should be.

Accordingly, I suggest this PR be split into two: 1 patching for actual bug fixes, and 2 implementing proposed alterations in tagging patterns, with a clear and stated rationale based on actual experience (from users), not simply hearsay.

The alternative is to impose the costs of implementing changes on the users, with no clear benefits.

@iMichaela
Copy link
Contributor Author

iMichaela commented Dec 12, 2023

I believe the proposed change to control properties has not been sufficiently discussed with users, both whether a change is warranted and what the change should be.

Accordingly, I suggest this PR be split into two: 1 patching for actual bug fixes, and 2 implementing proposed alterations in tagging patterns, with a clear and stated rationale based on actual experience (from users), not simply hearsay.

The alternative is to impose the costs of implementing changes on the users, with no clear benefits.

@Wendell - Please note:

  1. All issues addressed were reported as bugs by the community.
  2. The community requested alignment of the control IDs with the CPRT data. So did the RMF team lead: requested accuracy of OSCAL representation with the CPRT data. CPRT data and announcement are available here: https://csrc.nist.gov/projects/cprt/catalog#/cprt/framework/version/SP_800_53_5_1_1/home. This PR aligns the OSCAL SP800-53 with the official CPRT data, and with RMF team announcement that lists the inclusion of control identifiers with leading zeros.
  3. The approach employed is identical to the sp800-53a approach.
  4. A request for comments on the proposed control id solution was posted 5 days ego with a Friday EOB deadline for submission of comments.
  5. The solution implemented is backwards compatible (does not change the OSCAL IDs, while provide alt-identfiers to represent the sp800-53 v.5.1.1 control IDs properly. This is not an arbitrary change.

We will not withhold the updates since FedRAMP needs an accurate catalog, free of errors. Any issue related to the new CPRT SP800-53 control IDs needs to be raised with the RMF team. Not representing the data accurately would be an error at our (OSCAL) end.

@wendellpiez
Copy link
Contributor

wendellpiez commented Dec 12, 2023

In the above, the definition of "control ID" is not specified. Is this a data point, and if so which one? Or is it only a rule for how to represent a control in short form (its 'code' visible on a screen)?

In preparation for last week's PR @aj-stein-nist (with my help) demonstrated that we could produce PDFs in "new style" with leading zeroes, without altering anything in the data and only minor changes to styling code (XSLT in the oscal-xslt repository ). RMF team lead helped us validate this functionality worked as wanted.

For both these reasons, I dispute that (a) all 'corrections' in this PR address reported bugs, and (b) that the 'solution' provided is backward compatible. (While still formally valid to schemas, it breaks code designed to represent the old data. QED.)

At best, it makes it necessary to make an adjustment in processing that had been optional and a matter of preference.

More fundamentally, I now see an increasing difficulty in validating the correctness of this representation against its upstream source. That was bad enough when we knew that this data set was at least internally consistent. Pending control mechanisms such as Schematron reflecting design specifications (unwritten), now we are hand patching, we lose that assurance as well.

I reiterate that a separate PR could be made with actual corrections, and without disturbing 'label' semantics until community has expressed preferences. Having those diffs separate would also aid greatly with transparency.

@iMichaela
Copy link
Contributor Author

In the above, the definition of "control ID" is not specified. Is this a data point, and if so which one? Or is it only a rule for how to represent a control in short form (its 'code' visible on a screen)?

Please check CPRT data which is the only format supported by the CPRT team.

In preparation for last week's PR @aj-stein-nist (with my help) demonstrated that we could produce PDFs in "new style" with leading zeroes, without anything in the data, only minor changes to styling code (XSLT in the oscal-xslt repository)[https://github.com/usnistgov/oscal-xslt/blob/25566d59bdff70b8c5b78866f2d92652da08e75f/publish/nist-emulation/sp800-53A-catalog_html.xsl#L96]. RMF team lead helped us validate this.

OSCAL team is not responsible for the generation of any PDF version of sp800-53. I was informed that the CPRT data is the official source, and OSCAL team is responsible for generating an accurate representation. As an OSCAL team member, I hope you align with this direction. Any other representation of the sp800-53 is not OSCAL team's responsibility.

For both these reasons, I dispute that (a) all 'corrections' in this PR address to reported bugs, and (b) that the 'solution' provided is backward compatible. (While still formally valid to schemas, it breaks code designed to represent the old data. QED.)

"Code designed to represent the old data" will have to be updated, this is not a good, ethical reason for presenting the OSCAL sp800-53 representation for being accurate.

More fundamentally, I now see an increasing difficulty in validating the correctness of this representation against its upstream source. That was bad enough when we knew that this data set was at least internally consistent. Pending control mechanisms such as Schematron reflecting design specifications (unwritten), now we are hand patching, we lose that assurance as well.

Why is that? It has been thoroughly reviewed by @JustKuzya. He found 3 places where leading zeros were missing.

I reiterate that a separate PR could be made with actual corrections, and without disturbing 'label' semantics until community has expressed preferences. Having those diffs separate would also aid greatly with transparency.

If you want to review the commits separately - please do so today, because the oscal-content patch release is being prepared.

I re-iterate, the community marked it as a bug, and the RMF team lead expects us (OSCAL team) to accurately represent the CPRT sp800-53. The issue has been discussed in OSCAL internal meetings and the approach was first reviewed internally by the participants in the Thursday meeting. As a result, including the CPRT sp800-83 IDs accurately was the resolution proposed, and the above comment describing the proposed approach and the call for feedback from the community was submitted immediately after the meeting on Th last week.

All your concerns of users and community members not having a say were addressed prior to raising them, through even the fact that they requested it..

@wendellpiez
Copy link
Contributor

wendellpiez commented Dec 12, 2023

I am seeing prop[@name='label'] in four places where it has not been updated, SA-22(1), SC-2, SC-44, SI-3(9) (using old-style labels).

Also, there is an insert (parameter reference) with a broken pointer, in the IA-13(3) statement (line 32407). It is pointing to ia-13_prm_1 which is now ia-13_odp.01 (above at line 32272).

On line 76109 appears a prop inside a part (statement) which has been erroneously changed from label (like its neighbors) to alt-identifier (but this is a part not a control). Unless the intent was to change them all (which has not been done), this is also wrong. (I do not have code to validate this fully yet.)

Additionally: wherever controls are cross-linked in the data, the link (anchor element in running content) has the 'old form' in display - which is what most renderers will show. (Look for any a element anywhere.)

<part name="guidance" id="ac-2.4_gdn">
   <p>Account management audit records are defined in accordance with <a href="#au-2">AU-2</a>
   and reviewed, analyzed, and reported in accordance with <a href="#au-6">AU-6</a>.</p>
</part>

Finally, I am seeing the 'old' label is nowhere found in any form (that I can see). The idea that backward compatibility is being maintained rather pales in view of this, since the only way now to match old to new is via an appropriate zero-padding or unpadding algorithm.

Except here:

<prop name="alt-identifier" class="sp800-53" value="SC-39(2)"/>

and here

<prop name="label" value="SI-3(9)"/>

(Will write again if I find more issues or questions.)

@wendellpiez
Copy link
Contributor

Another reason I am uneasy occurred to me. I am not sure the term 'alt-identifier' has any meaning when no label is offered.

Aren't they now just (all) identifier properties? What is "alt" about them?

If you are changing the meaning, maybe also change the term.

(Another set of inconsistencies we have to watch for is in the usage within parameters ... still looking.)

@wendellpiez
Copy link
Contributor

wendellpiez commented Dec 12, 2023

I have been able to confirm that only one prop[@name='label'] has been changed to alt-identifier when appearing directly inside part[@name='item'], in the part with id si-3.6_smt.a. The other 1111 are still marked 'label' (which is better for these IMV).

@wendellpiez
Copy link
Contributor

wendellpiez commented Dec 12, 2023

Were this subject to open discussion I might also suggest the prop elements with the sp800-53a values could now simply be dropped - or another way to put it is, what is the rationale now for retaining them? They merely echo the sp800-53 values as updated.

In view of what you have stated above, a simple way to put it might be:

<prop name="identifier" class="cprt" value="SC-39(02)"/>

Potential problem if identifier is not yet on the list of identifier identifiers (or an allowed-values constraint). But without 'label', "alternate identifier" also begs questions.

@iMichaela
Copy link
Contributor Author

This comment was under one commit. Moving it here.

These controls no longer carry the 'old' labels at all? Do other controls no longer have the old notation (without the zero-padding)?

More largely, what is the expected pattern for control labeling? (as it used to be called, but now "alt-identifier assigning"?) How is it being ensured that all controls follow that pattern?

I can work with team to show how to write, use and maintain a Schematron for this - again, in support of a regular process. Or you can start with Schematron examples already in the repo.
I validated the content with oscal-cli and oscal makefile. Please contribute with the Schematron validation. The team is interested and I'll schedule an internal meeting.
See below the info you asked for:

OSCAL-vs-CPRT 800-53

@iMichaela
Copy link
Contributor Author

iMichaela commented Dec 13, 2023

Were this subject to open discussion I might also suggest the prop elements with the sp800-53a values could now simply be dropped - or another way to put it is, what is the rationale now for retaining them? They merely echo the sp800-53 values as updated.

I agree but props with class=800-53a existed and have been used by the OSCAL 800-53 catalog users. Since SP800-53a is treated in CPRT as a separate data set, and uses identifiers too. It is easy to remove, much harder to add.

In view of what you have stated above, a simple way to put it might be:

<prop name="identifier" class="cprt" value="SC-39(02)"/>

Potential problem if identifier is not yet on the list of identifier identifiers (or an allowed-values constraint). But without 'label', "alternate identifier" also begs questions.

alt-identifier is an alternate identifier to the OSCAL id which has not been changed.
"identifier" for the name is not permitted. Why use class="cprt" when CPRT is a tool that represents sp800-53 in a simple JSON (aka another representation of 800-53 considered by RMF the original data). For 800-53a props the class="sp800-53a" . Consistency is better.
The capitalized, zero-leading identifiers are 800-53 identifiers from now on , not just for the v5.1.1, so class=sp800-53 is more appropriate.

@iMichaela
Copy link
Contributor Author

iMichaela commented Dec 13, 2023

I will work on addressing your finding. Here is the list for tracking and checking them when completed:

  • I am seeing prop[@name='label'] in four places where it has not been updated, SA-22(1), SC-2, SC-44, SI-3(9) (using old-style labels).

  • Also, there is an insert (parameter reference) with a broken pointer, in the IA-13(3) statement (line 32407). It is pointing to ia-13_prm_1 which is now ia-13_odp.01 (above at line 32272).

  • On line 76109 appears a prop inside a part (statement) which has been erroneously changed from label (like its neighbors) to alt-identifier (but this is a part not a control). Unless the intent was to change them all (which has not been done), this is also wrong. (I do not have code to validate this fully yet.)

Additionally: wherever controls are cross-linked in the data, the link (anchor element in running content) has the 'old form' in display - which is what most renderers will show. (Look for any an element anywhere.)

RESPONSE: The PR #220 submitted by AJ and reviewed by you (and me) was done to implement the list of changes AJ received from the RMF team (all small or big changes). I did not see the list of changes, nor could I review all are implemented, so I only focused on OSCAL format. However, I did observe other errors in the CPRT data where referenced controls were not updated (no leading zeros and broken links). When I raised the issue, and suggest to fix them in OSCAL, I was clearly asked (as it happened in the past too) to represent the information with existing error until they will be fixed first in the future in the CPRT .

  • Update:
<part name="guidance" id="ac-2.4_gdn">
   <p>Account management audit records are defined in accordance with <a href="#au-2">AU-2</a>
   and reviewed, analyzed, and reported in accordance with <a href="#au-6">AU-6</a>.</p>
</part>
  • Update
<prop name="alt-identifier" class="sp800-53" value="SC-39(2)"/>
  • Update
<prop name="label" value="SI-3(9)"/>

@iMichaela
Copy link
Contributor Author

@wendellpiez - all finding were fixed- please see the list above.

@wendellpiez
Copy link
Contributor

wendellpiez commented Dec 13, 2023

The semantics of 'alt-identifier' are wiggling, say what you like. Also, it bothers me that now we have three prop elements in every control, all of which say the exact same thing or nearly (including the sort-id).

But maybe I am wrong and there will be no complaints or grumbling about it. (I only wish I thought that meant no pain either.)

Please refresh the top-level @uuid one more time, and the metadata/last-modified timestamp. (A utility in the repo can be used to do this if wanted.)

Also please consider accepting PR #230, which updates the Schematrons held in the repo, so they do not report false errors on the changed data. It is made to be accepted into your branch before merging - and ideally should be tested by someone other than its developer.

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.

Please refresh the top-level @uuid one more time, and the metadata/last-modified timestamp. (A utility in the repo can be used to do this if wanted.)

Also please consider accepting PR #230, which updates the Schematrons held in the repo, so they do not report false errors on the changed data. It is made to be accepted into your branch before merging - and ideally should be tested by someone other than its developer.

@iMichaela
Copy link
Contributor Author

Please refresh the top-level @uuid one more time, and the metadata/last-modified timestamp. (A utility in the repo can be used to do this if wanted.)

Also please consider accepting PR #230, which updates the Schematrons held in the repo, so they do not report false errors on the changed data. It is made to be accepted into your branch before merging - and ideally should be tested by someone other than its developer.

Update done. I asked the team to review and approve your PR. If they are not doing it I will. Thank you.

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.

Thanks for adding

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.

4 participants