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(valiation): account for controls not evaluated by Lula #847

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

nacx
Copy link
Contributor

@nacx nacx commented Dec 12, 2024

Description

Opening as a draft to discuss the more convenient way to provide feedback when some controls do not have any Lula validation defined:

Currently, if the implemented controls do not define a Lula validation, they are reported as not-satisfied. This is fine, but it would be great to indicate the reasons for that verdict in the assessment results, such as "no lula validation is defined for this control", so users can properly reason about the result.

This can be achieved in different ways I'd like to discuss:

  • In the first place, is this a valid use case? Should the project account for such implemented-controls that do not define a lula validation? (And if not, should there be some validation instead?)
  • If the answer to the above is that Lula should gracefully handle that:
    • Does it make sense to keep findings for those controls in the findings list and in the reviewed-controls?
      • If not, the current state of the PR addresses it by not including the findings in the result. Or should that be reported in a different way?
      • If yes, what would be the best way to communicate that no validation was performed to determine the control is not-satisfied? Would a note in the finding remarks be OK? Or should that information be provided somewhere else?

Related Issue

#776
#350

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@brandtkeller
Copy link
Member

Currently, if the implemented controls do not define a Lula validation, they are reported as not-satisfied. This is fine, but it would be great to indicate the reasons for that verdict in the assessment results, such as "no lula validation is defined for this control", so users can properly reason about the result.

Agreed!

This can be achieved in different ways I'd like to discuss:

  • In the first place, is this a valid use case? Should the project account for such implemented-controls that do not define a lula validation? (And if not, should there be some validation instead?)

I believe the project should still account for them - more context below.

  • If the answer to the above is that Lula should gracefully handle that:

    • Does it make sense to keep findings for those controls in the findings list and in the reviewed-controls?

Current line of thought is yes - Retaining these controls as Findings allows for a few core principles to Lula adoption:

  1. Lula augments OSCAL - People can bring their own OSCAL with zero Lula implementation and generate an Assessment Results file that they could then use manually or with other systems.
  2. It allows for historical traceability - A control that was not-satisfied previously is now satisfied -> Report this (as well as the opposite scenario)
* If not, the current state of the PR addresses it by not including the findings in the result. Or should that be reported in a different way?
* If yes, what would be the best way to communicate that no validation was performed to determine the control is not-satisfied? Would a note in the finding remarks be OK? Or should that information be provided somewhere else?

I believe finding.remarks we be a great place to quickly annotated that no evidence was provided or collected to support the control being satisfied. This allows us to explore more concepts around what evidence means to findings.

Open to other opinions for consideration though.

@nacx
Copy link
Contributor Author

nacx commented Dec 12, 2024

I believe finding.remarks we be a great place to quickly annotated that no evidence was provided or collected to support the control being satisfied. This allows us to explore more concepts around what evidence means to findings.

That sounds good to me and as a good starting point. I'll update the PR accordingly following this approach and add the relevant tests, etc. Thx!

@meganwolf0
Copy link
Collaborator

I'd possibly throw out the other field in status which is the reason one. I sort of interpreted that field as more "machine readable", with remarks being just additional context. The constraint is that the value "may be locally defined", and I'm not totally sure what that is, but I assume it's something we could choose to enumerate however we want?

So for example we could have an enumeration of options for reason (validations-failed, validations-passed, partial-validations, no-validations) then some additional more human friendly remark that's like "No validations are linked to the control" or "Some validations failed (this one, that one)"

This is also something we could instrument over time and for this initial thought I think remarks would suffice 😄

@brandtkeller
Copy link
Member

Ah great point - I think it would be great to start the implementation of reason. The token type and ambiguity around "may be locally defined" (I need to understand what this really means) makes this a little unclear.

That said - leaning into finding.status.reason = other as defined natively and finding.status.remarks being the remarks section to augment?

@meganwolf0
Copy link
Collaborator

That said - leaning into finding.status.reason = other as defined natively and finding.status.remarks being the remarks section to augment?

Yeah I think that would work well!

nacx added 2 commits December 12, 2024 22:36
Signed-off-by: Ignasi Barrera <[email protected]>
@nacx nacx marked this pull request as ready for review December 12, 2024 21:37
@nacx nacx requested a review from a team as a code owner December 12, 2024 21:37
Signed-off-by: Ignasi Barrera <[email protected]>
@nacx
Copy link
Contributor Author

nacx commented Dec 12, 2024

Ok, pushed a commit implementing the suggested approach

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

👍🏻 LGTM, thanks again for the submission! Keep 'em coming!!! 😂 ❤️

@brandtkeller brandtkeller merged commit 58b234b into defenseunicorns:main Dec 13, 2024
10 checks passed
mildwonkey pushed a commit that referenced this pull request Dec 16, 2024
* Account for controls not evaluated by Lula

Signed-off-by: Ignasi Barrera <[email protected]>

* review comments

Signed-off-by: Ignasi Barrera <[email protected]>

* imports

Signed-off-by: Ignasi Barrera <[email protected]>

---------

Signed-off-by: Ignasi Barrera <[email protected]>
Co-authored-by: Brandt Keller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants