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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Nov 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
248e1ef
feat: add local-definitions and findings (#34)
yana1205 Oct 24, 2024
0d86b0c
fix: trestle version in unit test
yana1205 Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ build:

.PHONY: install
install:
python -m pip install .
python -m pip install -e .

.PHONY: install-dev
install-dev:
python -m pip install ".[dev]"
python -m pip install -e ".[dev]"

# Direct dependency is not allowed for Pypi packaging even if the dependant module is defined as extra dependencies.
# Workaround: Move to manual installation by make
Expand Down
9 changes: 7 additions & 2 deletions c2p/framework/c2p.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ def _get_result(self, pvp_result: PVPResult) -> Result:
start=oscal_utils.get_datetime_str(),
observations=self._get_observations(pvp_result),
reviewed_controls=oscal_utils.reviewed_controls(self._component_root.component_definition),
local_definitions=pvp_result.local_definitions,
findings=pvp_result.findings,
)
if pvp_result.links != None:
result.links = list(map(lambda x: Link(href=x.href, text=x.description), pvp_result.links))
Expand All @@ -183,7 +185,10 @@ def _get_observations(self, pvp_result: PVPResult) -> List[Observation]:
oscal_utils.add_prop(props, 'evaluated-on', subject, ['evaluated_on'])
oscal_utils.add_prop(props, 'reason', subject, ['reason'])
s = SubjectReference(
subject_uuid=oscal_utils.uuid(), title=subject.title, type=subject.type, props=props
subject_uuid=subject.subject_uuid if subject.subject_uuid else oscal_utils.uuid(),
title=subject.title,
type=subject.type,
props=props,
)
subjects.append(s)

Expand All @@ -197,7 +202,7 @@ def _get_observations(self, pvp_result: PVPResult) -> List[Observation]:
if observation.props != None:
props = props + observation.props
o = Observation(
uuid=oscal_utils.uuid(),
uuid=observation.uuid if observation.uuid else oscal_utils.uuid(),
title=observation.title,
description=observation.title,
methods=observation.methods,
Expand Down
19 changes: 19 additions & 0 deletions c2p/framework/models/pvp_result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@
from typing import List, Optional

from pydantic.v1 import Field
from trestle.oscal.assessment_results import LocalDefinitions1

from c2p.common.c2p_base_model import C2PBaseModel
from trestle.oscal.assessment_results import LocalDefinitions1
from trestle.oscal.common import Finding


class ResultEnum(str, Enum):
Expand Down Expand Up @@ -68,6 +71,11 @@ class Subject(C2PBaseModel):
A human-oriented identifier reference to a resource. Use type to indicate whether the identified resource is a component, inventory item, location, user, or something else.
"""

subject_uuid: Optional[str] = Field(
None,
description="A machine-oriented identifier reference to a component, inventory-item, location, party, user, or resource using it's UUID.",
title='Subject Universally Unique Identifier Reference',
)
title: str = Field(title='Name of the object')
type: str = Field(
...,
Expand All @@ -89,6 +97,7 @@ class ObservationByCheck(C2PBaseModel):
Describes an individual observation based on each Check_Id defined in Component Definition.
"""

uuid: Optional[str] = Field(None)
title: Optional[str] = Field(
None,
description='The title for this observation for the check item. If not given, check id is used.',
Expand Down Expand Up @@ -118,6 +127,16 @@ class ObservationByCheck(C2PBaseModel):

class PVPResult(C2PBaseModel):
Copy link
Member

@jpower432 jpower432 Nov 7, 2024 •
edited
Loading

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 •
edited
Loading

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.

observations_by_check: Optional[List[ObservationByCheck]] = Field(None)
findings: Optional[List[Finding]] = Field(
None,
description='Equivalent to the "findings" defined in the OSCAL Assessment Results.',
title='Describes an individual finding',
)
local_definitions: Optional[LocalDefinitions1] = Field(
None,
description='Equivalent to the "local-definitions" defined in the OSCAL Assessment Results.',
title='Local Definitions',
)
links: Optional[List[Link]] = Field(None)


Expand Down
2 changes: 1 addition & 1 deletion tests/data/framework/c2p/assessment-results.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"metadata": {
"title": "TEST Assessment Results",
"last_modified": "2024-03-22T08:28:11.000+00:00",
"version": "3.4.0",
"version": "3.5.0",
"oscal_version": "1.1.2"
},
"import_ap": {
Expand Down