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

Add security_control and host profiles to base_event.json #1270

Closed
wants to merge 2 commits into from

Conversation

mlmitch
Copy link
Contributor

@mlmitch mlmitch commented Nov 28, 2024

security_control and host are universally applicable in OCSF

Both were applied in an ad-hoc manner almost everywhere.
Also, sometimes the profile wasn't correctly applied (e.g. cloud_resources_inventory_info.json)

This change enables providing a consistent interface with these profiles to downstream data consumers.

Also removed redundant profile declarations in event hierarchy (e.g. cloud in dhcp_activity.json)

@mlmitch mlmitch marked this pull request as draft November 28, 2024 20:43
@mlmitch mlmitch marked this pull request as ready for review November 28, 2024 20:44
@@ -48,8 +48,5 @@
"description": "The version of the resource. For example <code>1.2.3</code>.",
"requirement": "optional"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in there to allow the server to add it to "Referenced By" list of the profile. Take a look here - https://schema.ocsf.io/profiles/cloud

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every other case was in the event hierarchy so this was the only profile declaration removal I reverted

@query-jeremy
Copy link
Contributor

The compatibility check is failing because action_id will now be a required attribute on a number of events when the security_control profile is in play.

The validator assumes that profiles are enabled globally instead of per-record, essentially not trusting producers to assign the correct values to metadata.profiles. From the validator's perspective, a security_finding event produced using OCSF 1.3 with the security_control profile enabled will not be compatible with OCSF 1.4. This perspective may be flawed.

Adding new required attributes is not allowed, unless the attribute was introduced in a profile that is new to the schema. It could be that the check should be updated to allow attributes added by way of any profile newly assigned to the event in question. I'm curious to learn what @rmouritzen-splunk thinks.

Source for the relevant check

@mlmitch
Copy link
Contributor Author

mlmitch commented Dec 3, 2024

Thanks for diagnosing so quickly Jeremy.

It could be that the check should be updated to allow attributes added by way of any profile newly assigned to the event in question.

I think this makes sense.

@rmouritzen-splunk
Copy link
Contributor

rmouritzen-splunk commented Dec 6, 2024

The compatibility check is failing because action_id will now be a required attribute on a number of events when the security_control profile is in play.

The validator assumes that profiles are enabled globally instead of per-record, essentially not trusting producers to assign the correct values to metadata.profiles. From the validator's perspective, a security_finding event produced using OCSF 1.3 with the security_control profile enabled will not be compatible with OCSF 1.4. This perspective may be flawed.

Adding new required attributes is not allowed, unless the attribute was introduced in a profile that is new to the schema. It could be that the check should be updated to allow attributes added by way of any profile newly assigned to the event in question. I'm curious to learn what @rmouritzen-splunk thinks.

Source for the relevant check

This looks like it assumes attributes from profiles are always active. A profile is only active in an OCSF event with the profile in its metadata.profiles array.

Adding profiles to event classes and objects should always be backwards compatible since OCSF events without the added profiles are unaffected.

Here are checks possible, though. These are the profile backwards compatibility checks I can think of:

  1. Check that profiles are not removed.
  2. Check that profile attributes are not removed.
  3. Check that profile attributes do not have backwards incompatible changes, (e.g., ensure attributes are not removed, ensure attribute types and attribute constraints are not changed or narrowed).
  4. Check that profile constraints are backwards compatible (in the same way as any constraint).
  5. Check for colliding attributes and warn (or fail?).

The checks other than the first are done in isolation, outside of the context of an event class or object. Other than removing profiles, I don't think there are any context-sensitive checks.

Let me know this makes sense and that these arguments hold up.

@davemcatcisco
Copy link
Contributor

Assuming I'm understanding the action_id issue correctly, another way of addressing it is to make action_id optional. This was proposed in a previous PR from @mlmitch. The motivation for that PR was that some event producers want to use the security_control profile in order to include TTPs in an activity (in the attacks attribute) but are not actually a "control point" from a security perspective and therefore don't perform any action. That PR was closed in favour of an alternate approach which was to introduce an Observed value to the action_id enumeration. But perhaps this needs to be revisited. Note that all of this happened in 1.4 and therefore hasn't yet been released. Just food for thought.

@mlmitch
Copy link
Contributor Author

mlmitch commented Dec 6, 2024

I like the addition of the new action_id values so I think that is worth holding onto. However, you're right that making action_id optional or recommended would make validation pass here as is. It is also a decent change on its own.

Can y'all thumbs up here if you think I should change this optionality in the PR?

`security_control` and `host` are universally applicable in OCSF

Both were applied in an ad-hoc manner _almost_ everywhere
Also, sometimes the profile wasn't correctly applied (e.g. cloud_resources_inventory_info.json)

This change enables providing a consistent interface with these profiles
to downstream data consumers.

Change optionality of `action_id` in `security_control` to `recommended`

Also removed redundant profile declarations in event hierarchy (e.g. cloud in dhcp_activity.json)
Profile declarations in objects left alone to facilitate "Referenced By" feature

Signed-off-by: Mitchell Wasson <miwasson@cisco.com>
@mlmitch
Copy link
Contributor Author

mlmitch commented Dec 9, 2024

Changing the optionality of action_id in the security_control profile from required to recommended resolved the validation issue. I'll bring this iteration up on the community call.

@mikeradka
Copy link
Contributor

Changing the optionality of action_id in the security_control profile from required to recommended resolved the validation issue. I'll bring this iteration up on the community call.

Thanks @mlmitch . I certainly think it is worth raising in the next community call just to ensure we are all on the same page with this one.

@mlmitch
Copy link
Contributor Author

mlmitch commented Dec 10, 2024

Closing in favour of:

Separate PR for TTP profile: #1279
Separate PR for Host profile: #1280

@mlmitch mlmitch closed this Dec 10, 2024
@mlmitch
Copy link
Contributor Author

mlmitch commented Dec 12, 2024

TTP profile didnt work out.

#1281 revisits applying security_control to the base event.

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.

7 participants