Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.7k
Experiment: allowlist expressions using CEL#1874
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| tps := utils.GenerateSampleSecrets("atlassian", secrets.NewSecret(utils.AlphaNumeric("24"))) | ||
| tps = append(tps, utils.GenerateSampleSecrets("confluence", secrets.NewSecret(utils.AlphaNumeric("24")))...) | ||
| tps = append(tps, utils.GenerateSampleSecrets("jira", secrets.NewSecret(utils.AlphaNumeric("24")))...) | ||
| v1Secret := func() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
v1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This rule is a compound of two distinct formats. I wasn't sure what to call the "old" one.
config/allowlist.go Outdated
| // Compile the expression | ||
| if useExpression && a.Expression != "" { | ||
| // Build the environment: variables and functions available to the users. | ||
| // TODO: Is it safe to reuse this across multiple expressions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would hope so 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i.e., canenv be declared as a top-level variable instead of being declared for each whitelist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yep, correctomundo
config/allowlist.go Outdated
| // ExpressionAllowed returns the result of the predicate expression. | ||
| func (a *Allowlist) ExpressionAllowed( | ||
| // Miserable workaround to import-cycles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmmm maybe it's timefinding got its own package. Outside of the scope for this PR though
| @@ -604,10 +607,18 @@ regexes = [ | |||
| '''^[a-zA-Z_.-]+$''', | |||
| ] | |||
| [[rules.allowlists]] | |||
| description = "Allowlist for Generic API Keys matching the `access` keyword" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If evaluating CEL is slower than regex, does this new allowlist actually benefit us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Albeit, it's a good demonstration on the power of introducing CEL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This occurred to me as well — though you're right, it was mostly for demonstration purposes. Rudimentary benchmarks show it to be faster.
Using expression
$ go generate ./...Checking finding: rA1wk0Y45YCufyfq10:58AM INF Evaluated expression duration="15.178µs"<<<Lazy initialization? The first call is much slower. 10:58AM INF Evaluated regex duration="1.633µs"Checking finding: e8e4df51-2054-49b0-ab1c-516ac95c691d10:58AM INF Evaluated expression duration="6.214µs"10:58AM INF Evaluated regex duration="1.663µs"Checking finding: 0736f5ef-7e88-499a-80cc-90c85d2a518010:58AM INF Evaluated expression duration="2.206µs"10:58AM INF Evaluated regex duration=746nsChecking finding: _LIBCPP_CONSTEXPR_AFTER_CXX1110:58AM INF Evaluated expression duration="2.54µs"10:58AM INF Evaluated regex duration="1.021µs"
Using combined regex
$ go generate ./...Checking finding: rA1wk0Y45YCufyfq11:01AM INF Evaluated regex duration="23.093µs"Checking finding: e8e4df51-2054-49b0-ab1c-516ac95c691d11:01AM INF Evaluated regex duration="27.992µs"Checking finding: 0736f5ef-7e88-499a-80cc-90c85d2a518011:01AM INF Evaluated regex duration="22.343µs"Checking finding: _LIBCPP_CONSTEXPR_AFTER_CXX1111:01AM INF Evaluated regex duration="23.505µs"
| Allowlists: githubAllowlist, | ||
| Allowlists: []*config.Allowlist{ | ||
| { | ||
| Expression: `secret.substring(34) != base62encode(crc32(secret.substring(4, 34)))`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@zricethezav@bplaxco I added areally sloppy expression to validate the checksum of GitHubghp_ tokens.
A 32 bit checksum in the last 6 digits of each token strikes the optimal balance between keeping the random token portion at a consistent entropy and enough confidence in the checksum. We start the implementation with a CRC32 algorithm, a standard checksum algorithm. We then encode the result with a Base62 implementation, using leading zeros for padding as needed.
https://github.blog/engineering/platform-security/behind-githubs-new-authentication-token-formats/
Uh oh!
There was an error while loading.Please reload this page.
Description:
This allows users to define custom expressions usinghttps://cel.dev/. I've provided two practical examples of this.
generic-api-keyallowlist only tests "access"-related regexes whenaccesswas detected in the fragment.Checklist: