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

Change --b64-entropy-score and --hex-entropy-score to --entropy-sensitivity #265

Closed
tarkatronic opened this issue Nov 4, 2021 · 2 comments · Fixed by #272
Closed

Change --b64-entropy-score and --hex-entropy-score to --entropy-sensitivity #265

tarkatronic opened this issue Nov 4, 2021 · 2 comments · Fixed by #272
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@tarkatronic
Copy link
Contributor

Feature Request

Is your feature request related to a problem? Please describe.

The --b64-entropy-score and --hex-entropy-score arguments, while useful, can be very confusing. Even knowing the internals of tartufo like I do, I can't say with complete confidence what the default values represent, nor how they were arrived at.

Describe the solution you'd like

As described by @rbailey-godaddy in #223 (comment), it would be far more useful and intuitive to have a --entropy-sensitivity argument, ranging from 1-100. It would require a bit more calculation on the part of tartufo, but would then be much more understandable for users.

The one thing I'm unsure of is, should this be split out into --b64-entropy-sensitivity and --hex-entropy-sensitivity, or keep them combined as one?

@tarkatronic tarkatronic added the enhancement New feature or request label Nov 4, 2021
@rbailey-godaddy
Copy link
Contributor

The one thing I'm unsure of is, should this be split out into --b64-entropy-sensitivity and --hex-entropy-sensitivity, or keep them combined as one?

I strongly advocate keeping them combined as one thing. They are effectively linked by the underlying math and the difference between them is due solely to the size of the representation character set. To my mind, wanting to use different settings for each representation is like standing in the surf at the beach and thinking "this water is too cold for my left foot and too warm for my right foot". ;-)

@rbailey-godaddy rbailey-godaddy self-assigned this Nov 8, 2021
@rbailey-godaddy rbailey-godaddy added this to the Version 3.0 milestone Nov 8, 2021
rbailey-godaddy pushed a commit that referenced this issue Nov 9, 2021
Fixes #265

Provide more comprehensible alternative for tuning entropy checking.
This is applied consistently across all target character sets, and stated
in a way that is slightly easier to understand ("higher means more
likely to flag a given string").

The older `--b64-entropy-score` and `--hex-entropy-score` options are
marked as deprecated but retained for backwards compatibility (and they
override `--sensitivity` if used together with it).
rbailey-godaddy pushed a commit that referenced this issue Nov 10, 2021
Fixes #265

Provide more comprehensible alternative for tuning entropy checking.
This is applied consistently across all target character sets, and stated
in a way that is slightly easier to understand ("higher means more
likely to flag a given string").

The older `--b64-entropy-score` and `--hex-entropy-score` options are
marked as deprecated but retained for backwards compatibility (and they
override `--sensitivity` if used together with it).
rbailey-godaddy pushed a commit that referenced this issue Nov 12, 2021
Fixes #265

Provide more comprehensible alternative for tuning entropy checking.
This is applied consistently across all target character sets, and stated
in a way that is slightly easier to understand ("higher means more
likely to flag a given string").

The older `--b64-entropy-score` and `--hex-entropy-score` options are
marked as deprecated but retained for backwards compatibility (and they
override `--sensitivity` if used together with it).
rbailey-godaddy pushed a commit that referenced this issue Nov 12, 2021
Fixes #265

Provide more comprehensible alternative for tuning entropy checking.
This is applied consistently across all target character sets, and stated
in a way that is slightly easier to understand ("higher means more
likely to flag a given string").

The older `--b64-entropy-score` and `--hex-entropy-score` options are
marked as deprecated but retained for backwards compatibility (and they
override `--sensitivity` if used together with it).
tarkatronic pushed a commit that referenced this issue Nov 15, 2021
* Add --sensitivity option

Fixes #265

Provide more comprehensible alternative for tuning entropy checking.
This is applied consistently across all target character sets, and stated
in a way that is slightly easier to understand ("higher means more
likely to flag a given string").

The older `--b64-entropy-score` and `--hex-entropy-score` options are
marked as deprecated but retained for backwards compatibility (and they
override `--sensitivity` if used together with it).

* Change option name

Use `--entropy-sensitivity` instead of `--sensitivity`

* Wordsmithing

* Wordsmithing again

* Rebase to eliminate merge conflicts

* linter fixups

* Documentation fixups

...in response to review comments

* Expose magic numbers as properties

...and use them instead of private members

* Fix managed merge handling

* Remove entropy scoring members

Do everything from scratch instead of storing explicitly. This is a PITA
because you can't combine `@property` and `@lru_cache()` and skipping
the caching would be a killer.

* Review feedback tuneups

* Consolidate common code for entropy limit back into a single method,
  and rework properties related to it so they are cleaner.
* Invert sensitivity scale; adjust math and doc to match. It's still
  weird but aligns more closely to the underlying entropy metric.

* Fix change log

Co-authored-by: Scott Bailey <scott.bailey@godaddy.com>
@tarkatronic
Copy link
Contributor Author

Fixed in #272

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants