search

usnistgov / oscal-content

NIST SP 800-53 content and other OSCAL content examples
Other
301 stars 123 forks source link

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

Closed iMichaela closed 10 months ago

iMichaela commented 10 months ago

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:

Changes to Core Features:

iMichaela commented 10 months ago

During the triage meeting on 12/07/2023, the team discussed issue https://github.com/usnistgov/oscal-content/issues/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.

Compton-US commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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"/>
iMichaela commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

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 .

iMichaela commented 10 months ago

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

wendellpiez commented 10 months ago

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.

iMichaela commented 10 months ago

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.