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

Initial set of Azure secrets for #539 #1079

Merged
merged 9 commits into from Jun 15, 2023
Merged

Conversation

dvasdekis
Copy link
Contributor

@dvasdekis dvasdekis commented Jan 18, 2023

Description:

Please find in this PR an almost-complete set of Azure secrets as sourced from here, plus some new ones from my own experience.

Checklist:

  • Does your PR pass tests?
  • Have you written new tests for your changes?
  • Have you lint your code locally prior to submission?

@jit-ci
Copy link

jit-ci bot commented Jan 18, 2023

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

All security workflows are defined in a centralized repository named .jit.
In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

@dvasdekis
Copy link
Contributor Author

dvasdekis commented Jan 19, 2023

I've commented out the secret types I couldn't get to work (mostly the XML ones) but the rest pass go run main.go now. Hopefully you can approve and pull in 👍 and some future genius can fix my missing searches.

.gitignore Outdated Show resolved Hide resolved
Co-authored-by: Jesse Houwing <jesse.houwing@gmail.com>
@dvasdekis
Copy link
Contributor Author

Sure, I've accepted the changes

@dvasdekis
Copy link
Contributor Author

Hi, can I get these changed merged please?

@dvasdekis dvasdekis requested review from jessehouwing and nikolamalesevic and removed request for jessehouwing and nikolamalesevic February 12, 2023 23:19
@JoostVoskuil
Copy link
Contributor

Hi @zricethezav , can this be merged? This is a big addition for the Microsoft techstack. Or is there an additional step required?

Description: "CSCAN0100 - Found Azure storage credential in source code file.",
RuleID: "azure-storage-credential-xstore",
SecretGroup: 1,
Regex: generateUniqueTokenRegex(`[<XstoreAccountInfo].*accountSharedKey\s*=\s*['"].*['"]`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, what's the reason for changing some of the patterns?

-<XstoreAccountInfo[ -~"\s\S\n\r\t]+accountSharedKey\s*=\s*"[^"]{30}[ -~"\s\S\n\r\t]+/>
+[<XstoreAccountInfo].*accountSharedKey\s*=\s*['"].*['"]

In particular, this won't work as expected because [<XstoreAccountInfo] will only match a single character from inside the square brackets, not <XstoreAccountInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably because there were like 100 rules and I blew through them with all the enthusiasm that I had, now exhausted. When I get some free time I'll try it again, with the above change

@tgrnwd
Copy link

tgrnwd commented May 22, 2023

Definitely interested in seeing these rules included. Anything I can do to help @dvasdekis ?

@dvasdekis
Copy link
Contributor Author

These rules are complete. Not sure why they're not merged

@JoostVoskuil
Copy link
Contributor

@zricethezav how can we complete this?

@zricethezav zricethezav merged commit 7911ac6 into gitleaks:master Jun 15, 2023
@zricethezav
Copy link
Collaborator

zricethezav commented Jun 15, 2023

Actually, jk, there are some issues here. Most of these rules do not have keywords, which are necessary if gitleaks is to maintain reasonable performance. Opening up a follow up commit.

FWIW, after adding this PR the scan time more than doubled. Gotta have them keywords

zricethezav added a commit that referenced this pull request Jun 15, 2023
@dvasdekis
Copy link
Contributor Author

Okay, I can amend the original code with some keywords, when they're available from Azure. But for those in the spec that don't have keywords, are you planning to never merge then?

@nikolamalesevic
Copy link

@zricethezav This is not a way to handle this. Many of us have been waiting for this PR for a long time, with inexplicable months of silence from your side, and now jokes and fun and let's throw everything in the fire.

There must be a better way to communicate a way forward.

@JoostVoskuil
Copy link
Contributor

@nikolamalesevic be kind please

@zricethezav
Copy link
Collaborator

zricethezav commented Jun 16, 2023

@nikolamalesevic lol I'm really really sorry for not providing good enough free labor. get real dude

Anyways, this PR adds rules to the default configuration. If you want to use these rules for your own scans then you can, you just need to specify it using --config or one of the other methods.

I admit, I was trigger happy and merged this PR too quickly. Something that should be added is a check that makes sure all new rules have at least one keyword and that the delta in scan time does not exceed some %.

@zricethezav
Copy link
Collaborator

@dvasdekis

Okay, I can amend the original code with some keywords, when they're available from Azure. But for those in the spec that don't have keywords, are you planning to never merge then?

I'd be happy to merge if the rules can be updated with keywords. The reason why keywords are so important is because gitleaks does a string compare on a chunk of text before attempting to match a regex. So if no keyword is specified, then that rule would be running regex on every chunk of text, which slows things down dramatically. You can see how this is implemented here:

gitleaks/detect/detect.go

Lines 578 to 596 in 7804d65

for _, rule := range d.Config.Rules {
if len(rule.Keywords) == 0 {
// if not keywords are associated with the rule always scan the
// fragment using the rule
findings = append(findings, d.detectRule(fragment, rule)...)
continue
}
fragmentContainsKeyword := false
// check if keywords are in the fragment
for _, k := range rule.Keywords {
if _, ok := fragment.keywords[strings.ToLower(k)]; ok {
fragmentContainsKeyword = true
}
}
if fragmentContainsKeyword {
findings = append(findings, d.detectRule(fragment, rule)...)
}
}
return filter(findings, d.Redact)

Feel free to reopen a PR when keywords have been added. Again, sorry for the merging then reverting.

@nikolamalesevic
Copy link

@nikolamalesevic be kind please

@nikolamalesevic lol I'm really really sorry for not providing good enough free labor. get real dude

The comment was meant to be an honest feedback specific to handling of this PR (however negative), and never did I say that I do not appreciate a hard work put into this. I do apologize if I came out as being rude.

@zricethezav
Copy link
Collaborator

The comment was meant to be an honest feedback specific to handling of this PR (however negative), and never did I say that I do not appreciate a hard work put into this. I do apologize if I came out as being rude.

It's all good, I need a performance check to make sure new rules don't increase the scan time by some %. Again, I'm happy to review new rules if someone is up to the task.

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

Successfully merging this pull request may close these issues.

None yet

7 participants