Skip to content

Conversation

@brokensound77
Copy link
Contributor

Issues

related to #2251

Summary

If a forked rule (a rule that was previously locked and a subsequent min_stack_version was added - this can be tracked by any rule with a previous entry in the lock file) gets changes, there are no checks to ensure that the change will result in a version collision with a future state of the rule.

This adds a command to check for version space, which will be executed in the incrementally in the lock-multiple.sh script as part of the lock-versions workflow.

@botelastic botelastic bot added the python Internal python for the repository label Aug 26, 2022
else:
raise RuntimeError("Unreachable code")

if 'previous' in lock_from_file:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this code worked, the flaw was that it relied on the root (current) version as a comparison. This meant that when the root version incremented, it would quietly allow previous entries to creep as well.

L290-295 instead use the newly pinned max_allowable_version to keep a static check

@dev_group.command('test-version-lock')
@click.argument('branches', nargs=-1, required=True)
@click.option('--remote', '-r', default='origin', help='Override the remote from "origin"')
def test_version_lock(branches: tuple, remote: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We potentially could run this in all unit tests where a rule file is changed to be really thorough, but I think for now we should leave it as a manual command

@brokensound77
Copy link
Contributor Author

brokensound77 commented Sep 15, 2022

While running the new test-version-lock command to simulate the full iteration of the lock. It worked great as expected and actually errored out as expected. The issue appears to be an unrelated bug where the previous version of 17 rules are incrementing but instead of incrementing 1 version, they are taking the root version (100) (this is the bug, which was reproduced in the lock workflow and can be seen here).

So when it eventually gets to the root version, it detects that a previous entry is higher (as expected). So the 2 things to figure out:

  • what is special about those 17 rules that they are different in 7.16 vs 8.3+ (not necessarily tied to the bug)
  • why is a previous entry (forked rule) with changes inheriting the root version vs incrementing by 1 (as it should)

Again, these bugs are separate from this PR, but before merging, we should investigate a bit more to fully understand the resolution, since this modifies the version.lock (with the maxes)

@terrancedejesus
Copy link
Contributor

terrancedejesus commented Sep 15, 2022

Checked out 7.16 and ran the build-release --update-version-lock command to identify which rules versions were jumping all buffer space.

Outlier chosen: rule.name == Potential Credential Access via Windows Utilities
Issue: 7.16 forked version goes from 9 -> 101

The issue appears to stem from having multiple forked rules and route C specifically on line 281.

info_from_rule - The latest forked content, 8.3 and version 100
info_from_file - The current stack, 7.16, and version 9

If these do not match, as expected, it will set the 7.16 forked entry to the 8.3 entry and update that rule version as well.

We do have a TODO note in here to determine supporting old locked versions so maybe this is the right time to discuss and update this route.

I am not sure why these 17 specific rules are hitting this bug as every rule should have a previous 7.16 forked entry and forked again at 8.3 as they don't have rule content changes based on the SHA256 hash. This may also point to the bug as well, for some reason when lock_from_rule is defined, rules that are not included in this list of 17 have the previous forked entry version whereas the 17 has the latest forked version.

@brokensound77
Copy link
Contributor Author

unnecessary bump resolution.

Still need to fix the massive version jump for legitimate cases though.

Copy link
Contributor

@Mikaayenson Mikaayenson left a comment

Choose a reason for hiding this comment

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

LGTM for this split merge

@brokensound77
Copy link
Contributor Author

I am not sure why these 17 specific rules are hitting this bug as every rule should have a previous 7.16 forked entry and forked again at 8.3 as they don't have rule content changes based on the SHA256 hash. This may also point to the bug as well, for some reason when lock_from_rule is defined, rules that are not included in this list of 17 have the previous forked entry version whereas the 17 has the latest forked version.

There was just misalignment between the rule min stack and corresponding lock. Fix normalizes them to be major.minor to match

@brokensound77 brokensound77 merged commit 2ee5a18 into elastic:main Sep 19, 2022
@brokensound77 brokensound77 deleted the version-collision-test branch September 19, 2022 15:53
protectionsmachine pushed a commit that referenced this pull request Sep 19, 2022
* Add test command to verify version collisions do not occur
* add max_allowable_version to schema and lock flow
* add max_allowable_version to all entries in version.lock
* add test-version-lock command
* use min supported stack if > locked min stack
* share lock conversion code with rule and lock to fix M.m bug

(cherry picked from commit 2ee5a18)
protectionsmachine pushed a commit that referenced this pull request Sep 19, 2022
* Add test command to verify version collisions do not occur
* add max_allowable_version to schema and lock flow
* add max_allowable_version to all entries in version.lock
* add test-version-lock command
* use min supported stack if > locked min stack
* share lock conversion code with rule and lock to fix M.m bug

(cherry picked from commit 2ee5a18)
protectionsmachine pushed a commit that referenced this pull request Sep 19, 2022
* Add test command to verify version collisions do not occur
* add max_allowable_version to schema and lock flow
* add max_allowable_version to all entries in version.lock
* add test-version-lock command
* use min supported stack if > locked min stack
* share lock conversion code with rule and lock to fix M.m bug

(cherry picked from commit 2ee5a18)
protectionsmachine pushed a commit that referenced this pull request Sep 19, 2022
* Add test command to verify version collisions do not occur
* add max_allowable_version to schema and lock flow
* add max_allowable_version to all entries in version.lock
* add test-version-lock command
* use min supported stack if > locked min stack
* share lock conversion code with rule and lock to fix M.m bug

(cherry picked from commit 2ee5a18)
protectionsmachine pushed a commit that referenced this pull request Sep 19, 2022
* Add test command to verify version collisions do not occur
* add max_allowable_version to schema and lock flow
* add max_allowable_version to all entries in version.lock
* add test-version-lock command
* use min supported stack if > locked min stack
* share lock conversion code with rule and lock to fix M.m bug

(cherry picked from commit 2ee5a18)
protectionsmachine pushed a commit that referenced this pull request Sep 19, 2022
* Add test command to verify version collisions do not occur
* add max_allowable_version to schema and lock flow
* add max_allowable_version to all entries in version.lock
* add test-version-lock command
* use min supported stack if > locked min stack
* share lock conversion code with rule and lock to fix M.m bug

(cherry picked from commit 2ee5a18)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport: auto python Internal python for the repository v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants