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

feat: add local-definitions and findings (#34) #35

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

yana1205
Copy link
Collaborator

@yana1205 yana1205 commented Nov 5, 2024

Signed-off-by: Takumi Yanagawa [email protected]

This PR is about #34.

What's the changes:

  • enable to add local-definitions and findings to PVP Results
  • minor enhancement (Makefile): Changed to "pip install -e" to allow immediate reflection of code changes during development.

@yana1205 yana1205 requested a review from jpower432 November 5, 2024 14:35
Signed-off-by: Takumi Yanagawa <[email protected]>
@yana1205
Copy link
Collaborator Author

yana1205 commented Nov 6, 2024

Hi @jpower432,
Could you have a chance to take a look at the PR and approve if all looks good?

Copy link
Member

@jpower432 jpower432 left a comment

Choose a reason for hiding this comment

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

I think it would be helpful to talk through the logic a bit more on this one and get some examples of how this exchange would happen with the plugins since local-definitions and finding have reference back to controls.

@@ -118,6 +127,16 @@ class ObservationByCheck(C2PBaseModel):

class PVPResult(C2PBaseModel):
Copy link
Member

@jpower432 jpower432 Nov 7, 2024

Choose a reason for hiding this comment

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

If the PVPResult is returned by a PVP plugin per the PluginSpec, I am wondering how a plugin would be able to determine what should go in these two new fields. Would it make more sense for C2P to handle this information during the result generation after it has received the required information from the plugin? Specifically for local definitions, I think it might be easier to implement the Assessment Plan input first since that information would be pulled directly from that corresponding field. For findings, I would be curious if C2P should determine this information based on aggregations of the observations, their passing status, and how they related to specific controls. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two fields will just passed to the OSCAL Assessment Results (I hope my other comment may help).

Would it make more sense for C2P to handle this information during the result generation after it has received the required information from the plugin?

Yes, that makes sense. The C2P core could generate basic findings in OSCAL format based on the required information received from the plugin with using Component Definitions (OSCAL-COMPASS opinionated format). This approach would centralize the generation process, reducing the burden on individual plugins and ensuring consistency for "findings". Users can choose whether to use or discard these findings.

As for the local-definition, in this PR, I was assuming that the local-definition refers to a locally defined one that is not necessarily defined in the Assessment Plan (AP). There are use cases where stuff such as inventory and components are defined directly in results returned from PVP. There are also still use cases aligned with a partial OSCAL framework where the AP is not taken into account. This PR is for these use cases.

Copy link
Member

@jpower432 jpower432 Nov 8, 2024

Choose a reason for hiding this comment

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

Thanks for clarifying @yana1205 ! I understand the local-definitions use case better now and how PVPs might need to pass back system information. I see now that I was looking at the top level local definitions and this addresses local definitions under results.

It sounds like we agree on how to process findings. With the additional context you have provided, the only other thought I have is about the direct use of OSCAL objects in the PVPResult. I find that a core benefit of C2P is the simplified interface provided to plugin authors. I think including the direct manipulation of OSCAL objects for plugin authoring could add some complexity. I'm curious to hear your thoughts on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jpower432
That's good point. I also see the value of C2P plugins allowing users to work without deep knowledge of OSCAL. For now, though, comparing with "observation", I found it challenging to consolidate local-definition into a common structure since I haven’t encountered many use cases (this was actually my first one with the recent requirement.)
On the other hand, allowing PVPResult to support OSCAL local-definition natively as an optional feature provides added flexibility. Once I’ve gathered more use cases, I am thinking to consolidate this into a common structure and define a model to generate local-definition effectively. At that stage, C2P would generate OSCAL AR with local-definition from minimal information inputs. Additionally, plugin developers who are familiar with OSCAL could still write local-definition directly using Trestle if desired.
Similarly, for findings, C2P would support generating findings from minimal inputs, as well as providing flexibility for developers to directly write in OSCAL, in addition to the basic findings we discussed earlier.

@jpower432
Copy link
Member

Also wondering if we could identify helpful SDK transformation logic here. Specifically transforming an Assessment Plan into an partially populated Assessment Result. Might be related to oscal-compass/compliance-trestle#1727.

@yana1205
Copy link
Collaborator Author

yana1205 commented Nov 8, 2024

Thanks for your feedback!

I think it would be helpful to talk through the logic a bit more on this one and get some examples of how this exchange would happen with the plugins since local-definitions and finding have reference back to controls.

C2P simply forwards these two fields to the OSCAL Assessment Results.

For example of the following plugin (please also refer to the actual code),

class PluginSampleChecker(PluginSpec):

    def generate_pvp_result(self, raw_result: RawResult) -> PVPResult:

        pvp_result: PVPResult = PVPResult()
        pvp_result.observations_by_check = []

        # Define a finding
        finding = Finding(
            uuid=uuid(),
            title='Findings of the control cm-2.1',
            description='The control is not satisfied since there is no observation.',
            target=FindingTarget(
                target_id='cm-2.1',
                type=FindingTargetTypeValidValues.objective_id,
                status=ObjectiveStatus(state=ObjectiveStatusStateValidValues.not_satisfied),
            ),
        )
        pvp_result.findings = [finding]

        # Define local definitions
        pvp_result.local_definitions = LocalDefinitions1(
            inventory_items=[
                InventoryItem(
                    uuid=uuid(),
                    description='An inventory',
                )
            ]
        )

        return pvp_result

the generated OSCAL Assessment Results are as follows, where the local-definitions and the findings are just forwarded to the results. (Please also refer to the actual results)

{
  "assessment-results": {
    ...
    "results": [
      {
        "uuid": "95e0d766-2088-49d2-bb4c-dde80a94841d",
        "title": "Assessment Result",
        "local-definitions": {
          "inventory-items": [
            {
              "uuid": "4d6fd4e6-2d8b-4239-ae35-e931a10016c8",
              "description": "An inventory"
            }
          ]
        },
        "observations": [],
        "findings": [
          {
            "uuid": "97ad9512-96f3-409f-a452-88e41cd23b92",
            "title": "Findings of the control cm-2.1",
            "description": "The control is not satisfied since there is no observation.",
            "target": {
              "type": "objective-id",
              "target-id": "cm-2.1",
              "status": {
                "state": "not-satisfied"
              }
            }
          }
        ]
      }
    ]
  }
}

Copy link
Member

@jpower432 jpower432 left a comment

Choose a reason for hiding this comment

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

LGTM

@yana1205 yana1205 merged commit 389dc8f into oscal-compass:main Nov 13, 2024
4 checks passed
@yana1205 yana1205 deleted the feat34 branch November 13, 2024 14:05
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.

2 participants