-
Notifications
You must be signed in to change notification settings - Fork 206
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
Improve secret detection heuristics #4289
Conversation
1a057c9
to
2972d9b
Compare
@@ -2554,6 +2598,9 @@ objectModelConfiguration: | |||
Databricks: | |||
ResourceId: | |||
$armReference: true | |||
DatabricksProperties: | |||
DatabricksAccessToken: |
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.
Breaking change?
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.
Yup.
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.
Where are the associated code-generated changes for this?
I only see 2 files in this PR now?
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 isn't new - it's relocated from just below - we're supposed to have things in alphabetical order, so I moved it up into place. That's why there's no change to generated code.
v2/azure-arm.yaml
Outdated
@@ -2587,6 +2634,9 @@ objectModelConfiguration: | |||
DatabricksProperties: | |||
DatabricksAccessToken: | |||
$isSecret: true | |||
VirtualMachineSshCredentials_STATUS: | |||
PrivateKeyData: |
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 shouldn't be included in the STATUS
?
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.
Correct.
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.
Looks good except that I was expecting generated code changes which I don't see now?
Most of the changes are marking false-positives as not secrets but there is one new secret, right? Where is it in the generated code?
@@ -2554,6 +2598,9 @@ objectModelConfiguration: | |||
Databricks: | |||
ResourceId: | |||
$armReference: true | |||
DatabricksProperties: | |||
DatabricksAccessToken: |
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.
Where are the associated code-generated changes for this?
I only see 2 files in this PR now?
What this PR does / why we need it:
We've noticed a significant number of properties that should have been flagged as secrets, but which weren't noticed earlier. In this PR, we're improving our secret detection heuristics to try and prevent this from happening in the future.
Closes #4269
Special notes for your reviewer:
This detected a number of issues, which I think I've resolved correctly, but this needs a careful check.
Blocked on merge of #4237 and this will likely require a rebase after that one merges.
How does this PR make you feel: