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

Improve and fix enum declarations #1111

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

mlmitch
Copy link
Contributor

@mlmitch mlmitch commented Jun 12, 2024

Changes were tested on a local OCSF server and are error free.

Description of changes:

classification_ids now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/malware.json)

data_lifecycle_state_id mistakenly had the "Other" description applied to "0"/"Unknown"
A "99"/"Other" value was missing

flag_ids now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/dns_answer.json)

integrity mistakenly referenced the direction_id enum in its description

integrity_id now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/process.json)

load_type_id now declared in dictionary with default "0" and "99" values and decsriptions
"0" and "99" already existed without description through override in its only use (objects/module.json)

opcode_id now points to RFC5395 to explain why "0" does not correspond to "Unknown"
A "99"/"Unmapped" value is added to match other RFC based enums

protocol_ver_id now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/network_connection_info.json)

run_state_id now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/job.json)

@mlmitch
Copy link
Contributor Author

mlmitch commented Jun 12, 2024

I wasn't sure what to do with risk_level_id,
risk_level_id does not adhere to the enum convention with "0" being "Info" and no "Other" being defined in the dictionary.json, though "Other" values may exist in specific contexts.
A non-disruptive change could be to add "99"/"Other" to the dictionary and live with the inappropriate use of "0"

@mlmitch
Copy link
Contributor Author

mlmitch commented Jun 12, 2024

I talked over risk_level_id with @davemcatcisco a little.

risk_level description is fixed to reflect the absence of "99"/"Other" in risk_level_id.
risk_level_id does not have a "99" in any of its usages and "0" does not correspond to "Unknown".
Therefore the situation did not justify adding "99"/"Other"

irakledibm
irakledibm previously approved these changes Jun 18, 2024
Copy link
Contributor

@irakledibm irakledibm left a comment

Choose a reason for hiding this comment

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

lgfm

@zschmerber zschmerber added the bug Something isn't working label Jun 18, 2024
@mlmitch mlmitch force-pushed the enum-improvments branch 3 times, most recently from 3060d29 to da94142 Compare June 21, 2024 15:30
@mlmitch mlmitch requested a review from irakledibm June 21, 2024 15:55
zschmerber
zschmerber previously approved these changes Jun 21, 2024
irakledibm
irakledibm previously approved these changes Jun 24, 2024
Copy link
Contributor

@irakledibm irakledibm left a comment

Choose a reason for hiding this comment

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

Those changes make sense for current version of enum. We will need to consider introducing different types of enums in our dictionary as a data types in next release. An issue will be opened to start discussion.

@pagbabian-splunk
Copy link
Contributor

I have a minor suggestion: for the enums with only the nominal values (0/99 or 99) we should include the 'See specific usage` phrase in the descriptions, which the compiler will enforce to make sure a class or object defines the specific values. This usually is done for more generic attributes that can be overridden per class, but for specific attribute names, even though they are targeted for a single usage, the actual values must be defined in the class or object if they aren't defined in the dictionary.

@pagbabian-splunk pagbabian-splunk added description_updates Issues related to missing/incorrect/lacking descriptions of attributes v1.3.0 Changes marked for v1.3.0 of OCSF labels Jun 24, 2024
@mlmitch mlmitch dismissed stale reviews from irakledibm and zschmerber via 168cc51 June 25, 2024 21:44
@mlmitch
Copy link
Contributor Author

mlmitch commented Jun 26, 2024

Adding in the "See specific usage" phrase cracked open a new can of worms.
To start, the server produces a bunch of warnings:

2024-06-26 00:46:58.789 [warning] mfa=Schema.Cache.missing_desc_warning/4 line=1150  Please update the description for module.load_type_id: The normalized identifier of the load type. It identifies how the module was loaded in memory. See specific usage.
2024-06-26 00:46:58.791 [warning] mfa=Schema.Cache.missing_desc_warning/4 line=1150  Please update the description for malware.classification_ids: The list of normalized identifiers of the malware classifications. See specific usage. Reference: <a target='_blank' href='https://docs.oasis-open.org/cti/stix/v2.1/os/stix-v2.1-os.html#_oxlc4df65spl'>STIX Malware Types</a>
2024-06-26 00:46:58.792 [warning] mfa=Schema.Cache.missing_desc_warning/4 line=1150  Please update the description for process.integrity_id: The normalized identifier of the process integrity level (Windows only). See specific usage.
2024-06-26 00:46:58.793 [warning] mfa=Schema.Cache.missing_desc_warning/4 line=1150  Please update the description for affected_package.type_id: The normalized type identifier of an object. See specific usage.
2024-06-26 00:46:58.793 [warning] mfa=Schema.Cache.missing_desc_warning/4 line=1150  Please update the description for affected_package.vendor_name: The name of the vendor. See specific usage.
2024-06-26 00:46:58.793 [warning] mfa=Schema.Cache.missing_desc_warning/4 line=1150  Please update the description for package.type_id: The normalized type identifier of an object. See specific usage.
2024-06-26 00:46:58.793 [warning] mfa=Schema.Cache.missing_desc_warning/4 line=1150  Please update the description for package.vendor_name: The name of the vendor. See specific usage.

Some of these warnings existed previously and have nothing to do with this PR (type_id, vendor_name).
@pagbabian-splunk this is probably what you were thinking, and these things should be fixed too.
However, I think properly fixing these involves evaluating if a given enum is:

  1. a very specific enum name, and the definitions should be pulled into dictionary.json
  2. a general enum name, and the definitions should be pushed into their specific use case.

I started down this path and I suspect some of them will need conversation / debate.
The actual diff is probably about as big as this PR was originally.

I suggest that we hold off on the "See specific usage" change, and I'll start an additional PR to address those issues.

`classification_ids` now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/malware.json)

`data_lifecycle_state_id` mistakenly had the "Other" description applied to "0"/"Unknown"
A "99"/"Other" value was missing

`flag_ids` now declared in dictionary with default "0" and "99" values and descriptions
 "0" and "99" already existed without description through override in its only use (objects/dns_answer.json)

`integrity` mistakenly referenced the `direction_id` enum in its description

`integrity_id` now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/process.json)

`load_type_id` now declared in dictionary with default "0" and "99" values and decsriptions
"0" and "99" already existed without description through override in its only use (objects/module.json)

`opcode_id` now points to RFC5395 to explain why "0" does not correspond to "Unknown"
A "99"/"Unmapped" value is added to match other RFC based enums

`protocol_ver_id` now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/network_connection_info.json)

`run_state_id` now declared in dictionary with default "0" and "99" values and descriptions
"0" and "99" already existed without description through override in its only use (objects/job.json)

`risk_level` description is fixed to reflect the absence of "99"/"Other" in `risk_level_id`.
`risk_level_id` does not have a "99" in any of its usages and "0" does not correspond to "Unknown".
Therefore the situation did not justify adding "99"/"Other"

Analytic `type_id` mistakenly had "4"/"Learning (ML/DL)" removed in 1.2.0 with commit b44120e
This was a breaking change as "4" is now absent.
A description of this analytic type is added to match the other enum entries.

Signed-off-by: Mitchell Wasson <[email protected]>
@pagbabian-splunk pagbabian-splunk merged commit 3759e0d into ocsf:main Jul 2, 2024
2 checks passed
pagbabian-splunk pushed a commit that referenced this pull request Jul 25, 2024
…ee specific usage' in the description (#1146)

This change was a suggestion from @pagbabian-splunk in #1111 

After this PR, all enums that are defined in `dictionary.json` with only
the nominal values (0/99 or 99) also have 'See specific usage' in the
description.
The effect is that a warning will be generated by the OCSF server if
these attributes are used without overriding the description.

The process for these changes was:
- Decide if the attribute name is general enough to have other uses or
not.
- If the name is general enough, the definition in `dictionary.json`
only has 0/99 and 'See specific usage' is in description. The use of the
enum has the additional enum values and a description override.
- If the name is very specific, then the whole enum definition is now in
`dictionary.json` and the use of it only specifies optionality.

Signed-off-by: Mitchell Wasson <[email protected]>
Co-authored-by: Rajas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working description_updates Issues related to missing/incorrect/lacking descriptions of attributes v1.3.0 Changes marked for v1.3.0 of OCSF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants