[Bug] Version Comparison Bug in Related Integrations Field at Build Time

[terrancedejesus]

Summary

We recently merged a PR that adjusted how the related_integrations build-time field determined which integrations are valid. After checks passed and the PR was merged, we noticed a backport failure for the 8.3 branch.

During debugging, we noticed that the build time field for related_integrations did not have a version value set in the dictionary object, thus failing schema validation. The reason it did not have a version value was because of the comparison logic in integrations.find_least_compatible_version.compare_versions. L83 will compare the versions to ensure the integration version is less than or equal to the current stack version which is derived from packages.yml.

Therefore it was doing this comparison logic with 8.3.0 (integration package) <= 8.3 (package version) and ultimately returning False which then assigned the version key a value of None which does not comply with the schema.

A closer inspection shows that the package version sources from L268 of misc.py.

Solution

To solve this, we updated misc.load_current_package_version to have an argument patch which is set to False by default. If True, the package version will be given a default patch number of 0 when returned.

We then needed to update the call to this function from _add_related_integrations method in rule.py so it calls the same function but passes the argument patch=True.

I then tested this by running python -m view-rule /Users/tdejesus/code/src/detection-rules/rules/integrations/gcp/collection_gcp_pub_sub_subscription_creation.toml where it failed before and no errors occurred as shown below.

{
  "author": [
    "Elastic"
  ],
  "description": "Identifies the creation of a subscription in Google Cloud Platform (GCP). In GCP, the publisher-subscriber relationship (Pub/Sub) is an asynchronous messaging service that decouples event-producing and event-processing services. A subscription is a named resource representing the stream of messages to be delivered to the subscribing application.",
  "false_positives": [
    "Subscription creations may be done by a system or network administrator. Verify whether the user email, resource name, and/or hostname should be making changes in your environment. Subscription creations by unfamiliar users or hosts should be investigated. If known behavior is causing false positives, it can be exempted from the rule."
  ],
  "index": [
    "filebeat-*",
    "logs-gcp*"
  ],
  "language": "kuery",
  "license": "Elastic License v2",
  "name": "GCP Pub/Sub Subscription Creation",
  "note": "",
  "query": "event.dataset:gcp.audit and event.action:google.pubsub.v*.Subscriber.CreateSubscription and event.outcome:success\n",
  "references": [
    "https://cloud.google.com/pubsub/docs/overview"
  ],
  "related_integrations": [
    {
      "integration": "audit",
      "package": "gcp",
      "version": "^2.2.1"
    }
  ],
  "required_fields": [
    {
      "ecs": true,
      "name": "event.action",
      "type": "keyword"
    },
    {
      "ecs": true,
      "name": "event.dataset",
      "type": "keyword"
    },
    {
      "ecs": true,
      "name": "event.outcome",
      "type": "keyword"
    }
  ],
  "risk_score": 21,
  "rule_id": "d62b64a8-a7c9-43e5-aee3-15a725a794e7",
  "setup": "The GCP Fleet integration, Filebeat module, or similarly structured data is required to be compatible with this rule.",
  "severity": "low",
  "tags": [
    "Elastic",
    "Cloud",
    "GCP",
    "Continuous Monitoring",
    "SecOps",
    "Log Auditing"
  ],
  "threat": [
    {
      "framework": "MITRE ATT&CK",
      "tactic": {
        "id": "TA0009",
        "name": "Collection",
        "reference": "https://attack.mitre.org/tactics/TA0009/"
      },
      "technique": [
        {
          "id": "T1530",
          "name": "Data from Cloud Storage Object",
          "reference": "https://attack.mitre.org/techniques/T1530/"
        }
      ]
    }
  ],
  "timestamp_override": "event.ingested",
  "type": "query",
  "version": 101
}

I also ran unit tests to ensure they passed as this is where the errors and build originally failed during backporting for the 8.3 branch and these were successful. Note, I did this from the 8.3 branch with the last commit of the PR merged prior to ensure the same error occurred before and after the solution implementation.

[terrancedejesus]

I also questioned why this did not fail for 8.4 or 8.5 branches and that is because the comparison returns True since the minor is larger. For example - (8,3,0) <= (8,4).

Since the 8.3 branch is in a failed state, we will need to manually push changes to this branch after the code review is done and approved. We should take the commit of the changes approved, apply these to the 8.3 branch locally and then push them manually to the branch. When finished, we can re-run the failed jobs for this PR which should then pass.

[Mikaayenson]

I also questioned why this did not fail for 8.4 or 8.5 branches and that is because the comparison returns True since the minor is larger. For example - (8,3,0) <= (8,4).

>>> Version("8.3") <= Version("8.3.0")
True
>>> Version("8.3.0") <= Version("8.3")
False
>>> Version("8.3.0") == Version("8.3")
False

[Mikaayenson]

Minor suggestions. Otherwise LGTM

[brokensound77]

The bug was outside of the changes here, so I would roll most back, except L78. I added a comment for the actual bug.

Separate from this was another bug, where the manifests were not properly sorted before iterating.

rename:

-    latest_major_integration_manifests = \
        {k: v for k, v in integration_manifests.items() if Version(k)[0] == max_major}
+        integration_version = Version(int_ver)

Then sort before the loop:

-    for version, manifest in latest_major_integration_manifests.items():
+    for version, manifest in sorted(integration_manifests_versions.items(), key=lambda x: tuple(x[0]), reverse=True):

Finally, I think it may be better to raise an error on the function rather than return None

On L93:

-    print(f"no compatible version for integration {package}:{integration}")
-    return None
+    raise ValueError(f"no compatible version for integration {package}:{integration} "

I may not understand fully how None would be used, so could be wrong on the last one, but if it is always a validation error, it would be better to raise it here, closer to the issue

[terrancedejesus]

Separate from this was another bug, where the manifests were not properly sorted before iterating.

Thanks for the find! Yes the sorting was not being done on this but both implementations needed adjusted to properly sort them after testing. The following was implemented and tested.

integration_manifests = {k: v for k, v in sorted(packages_manifest[package].items(), key=lambda x: Version(str(x[0])))}

[Mikaayenson]

Small suggestions if it helps readability

[brokensound77]

LGTM if it passes tests (I would iterate through a few branches to make sure)

[terrancedejesus]

Manual push to 8.3 -> https://github.com/elastic/detection-rules/actions/runs/3151885290