Skip to content

Custom tags #420

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Custom tags #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 24 additions & 23 deletions contentctl/objects/abstract_security_content_objects/detection_abstract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,8 @@
from enum import StrEnum
from typing import TYPE_CHECKING, Annotated, Any, List, Optional, Union

from pydantic import (
Field,
FilePath,
HttpUrl,
ValidationInfo,
computed_field,
field_validator,
model_serializer,
model_validator,
)
from pydantic import (Field, FilePath, HttpUrl, ValidationInfo, computed_field,
field_validator, model_serializer, model_validator)

from contentctl.objects.lookup import FileBackedLookup, KVStoreLookup, Lookup
from contentctl.objects.macro import Macro
Expand All @@ -31,22 +23,16 @@
from contentctl.objects.base_test_result import TestResultStatus
from contentctl.objects.constants import (
CONTENTCTL_DETECTION_STANZA_NAME_FORMAT_TEMPLATE,
CONTENTCTL_MAX_SEARCH_NAME_LENGTH,
ES_MAX_STANZA_LENGTH,
ES_SEARCH_STANZA_NAME_FORMAT_AFTER_CLONING_IN_PRODUCT_TEMPLATE,
)
CONTENTCTL_MAX_SEARCH_NAME_LENGTH, ES_MAX_STANZA_LENGTH,
ES_SEARCH_STANZA_NAME_FORMAT_AFTER_CLONING_IN_PRODUCT_TEMPLATE)
from contentctl.objects.data_source import DataSource
from contentctl.objects.deployment import Deployment
from contentctl.objects.detection_tags import DetectionTags
from contentctl.objects.drilldown import DRILLDOWN_SEARCH_PLACEHOLDER, Drilldown
from contentctl.objects.enums import (
AnalyticsType,
ContentStatus,
DataModel,
NistCategory,
ProvidingTechnology,
RiskSeverity,
)
from contentctl.objects.drilldown import (DRILLDOWN_SEARCH_PLACEHOLDER,
Drilldown)
from contentctl.objects.enums import (AnalyticsType, ContentStatus, DataModel,
NistCategory, ProvidingTechnology,
RiskSeverity)
from contentctl.objects.integration_test import IntegrationTest
from contentctl.objects.manual_test import ManualTest
from contentctl.objects.rba import RBAObject, RiskScoreValue_Type
Expand Down Expand Up @@ -381,6 +367,21 @@ def annotations(self) -> dict[str, Union[List[str], int, str]]:
# So start with the mapping object.
annotations_dict.update(self.mappings)

##### Adding other tags in detection #####
# They should be referenced as a list in the yaml under tags>others
### Example:
## others:
## - tag1:
## - value1
## - tag2:
## - value21
## - value22
for temp_annotation in self.tags:
if temp_annotation[0] == "others":
other_tags = temp_annotation[1]
for other_tag in other_tags:
annotations_dict.update(other_tag)

# Make sure that the results are sorted for readability/easier diffs
return dict(sorted(annotations_dict.items(), key=lambda item: item[0]))

Expand Down
34 changes: 9 additions & 25 deletions contentctl/objects/detection_tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,9 @@
import uuid
from typing import TYPE_CHECKING, List, Optional, Union

from pydantic import (
UUID4,
BaseModel,
ConfigDict,
Field,
HttpUrl,
ValidationInfo,
computed_field,
field_validator,
model_serializer,
model_validator,
)
from pydantic import (UUID4, BaseModel, ConfigDict, Field, HttpUrl,
ValidationInfo, computed_field, field_validator,
model_serializer, model_validator)

from contentctl.objects.story import Story
from contentctl.objects.throttling import Throttling
Expand All @@ -25,24 +16,17 @@
from contentctl.objects.annotated_types import CVE_TYPE, MITRE_ATTACK_ID_TYPE
from contentctl.objects.atomic import AtomicEnrichment, AtomicTest
from contentctl.objects.constants import ATTACK_TACTICS_KILLCHAIN_MAPPING
from contentctl.objects.enums import (
AssetType,
Cis18Value,
KillChainPhase,
NistCategory,
SecurityContentProductName,
SecurityDomain,
)
from contentctl.objects.mitre_attack_enrichment import (
MitreAttackEnrichment,
MitreAttackGroup,
)
from contentctl.objects.enums import (AssetType, Cis18Value, KillChainPhase,
NistCategory, SecurityContentProductName,
SecurityDomain)
from contentctl.objects.mitre_attack_enrichment import (MitreAttackEnrichment,
MitreAttackGroup)


class DetectionTags(BaseModel):
# detection spec

model_config = ConfigDict(validate_default=False, extra="forbid")
model_config = ConfigDict(validate_default=False, extra="allow")
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 not a change I believe we can, or should, make in the core contentctl.
Before the release of contentctl 5.x, we did not include extra="forbid" in the definitions of our objects. What that led to was a SIGNIFICANT number of otherwise "optional" keys, such as manual_test, cve, etc being in the wrong place (for example nesting them under the detection object rather than tags or vice versa) or typo-ing a field and not having it caught by validation since it was parsed as an extra field. Being stricter with object definitions helps catch these at the validate phase and generate descriptive warnings.

Also, since these others are not typed in the PR (and do not have a use in ESCU) I would prefer not to add them to contentctl.

My recommendation would be that this field is instead added to your own copy of the detection.py file here:

contentctl/contentctl/objects/detection.py

Line 8 in 449128b

class Detection(Detection_Abstract):

Looking at the comment, you can see this is a file that future contentctl updates should not touch and is meant for user-specific implementations or modifications to the core Detection specification (which is actually Detection_Abstract).

I would recommend, at least, adding
others: dict = {}
to your local copy of the Detection Object or, even better, creating an Others object that looks like this or similar

class Others(BaseModel):
  tag1: list[str] = []
  tag2: list[str] = []

You should then be able to override the annotations method/property by calling the parent's annotations and updating it with the extra info you've included in your other field:

@computed_field
@property
def annotations(self) -> dict[str, Union[List[str], int, str]]:
   base_annotations = super().annotations
   #new_annotations = for loop logic for serializing whatever is in others```
   #Return combined annotations - note this can+will overwrite values in base_annotations with new_annotations if the keys match
   return base_annotations | new_annotations

I would agree that a strict definition like this does seem overly prescriptive, but with the complexity of our YML objects and possibility for error, I think that the prescriptiveness is actually a positive rather than a negative.

Please let me know your thoughts 😄

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your feedback! Maybe I did not code it the right way and did not use adequate variable names, I admit. But for me, the unmanaged annotations are a basic functionality in use in ES, so that would of great help to have this available through contentctl (see below). Let me know how we can move forward on this, really willing to help.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, okay this does make a lot of sense! Thank you for the clarification. Perhaps the core contentctl should include the following change, then, in detection_tags.py

unmanaged_annotations: dict[str,list[str]]

and that field, if present, should be serialized as part of the annotations function recommended above?
I think by clearly calling the field unmanaged_annotations we communicate pretty clearly to users:

  1. Where exactly these are going to show up in Enterprise Security (in........ Unmanaged Annotations!)
  2. That they don't get the same level of in-depth checking/validations as other fields.

This is actually much closer to what you originally contributed.
Let me tag in another contentctl contributor to see if they can provide their thoughts.

What do you think @ljstella ?

Copy link
Author

Choose a reason for hiding this comment

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

Hi all! Any news regarding this? Let me know if you need anything. Thanks!

analytic_story: list[Story] = Field(...)
asset_type: AssetType = Field(...)
group: list[str] = []
Expand Down