Resolve CredScan issues with Core, KeyVault #23653
Conversation
Resolves Azure#23505 and resolves Azure#23502
heaths
requested review from
chidozieononiwu,
danieljurek,
KrzysztofCwalina and
schaabs
as code owners
ghost
added
|
|
||
| namespace Azure.Security.KeyVault.Certificates.Tests | ||
| { | ||
| public partial class PemReaderTests |
There was a problem hiding this comment.
Should there be a similar comment about excluding this file from credscan?
There was a problem hiding this comment.
You're right. I clearly forgot that here. I'll submit a new PR since I don't think I have anything else KV-related planned today.
| @@ -50,11 +51,13 @@ | |||
| }, | |||
| { | |||
| "file":[ | |||
| "sdk/core/Azure.Core/tests/PemReaderTests.Keys.cs", | |||
There was a problem hiding this comment.
I'm a bit worried it would serve as a bad example for people to add their entire cs files to ignore list.
There was a problem hiding this comment.
They could've before.
I do include a comment that only keys should be in there (there is one test but because it needs a variable for line endings). Making these separate files and having to templatize some of them is an unnecessary burden.
There was a problem hiding this comment.
I definitely don't want entire cs files blindly added but if the cs file only contains test secrets and it is clear that is what the file is for I'm OK with this approach.
There was a problem hiding this comment.
I remember seeing an inline ignore syntax in CredScan similar to what cspell has. Should we use it instead?
There was a problem hiding this comment.
There is some support for inline suppressions however I've been trying to avoid them because they are CredScan specific and there are other secret scanning tools we are also depending on (i.e. github secret scanning) which don't support them and having secrets being isolated to individual files or using a set of placeholders is a little easier to reason about in those contexts.
Resolves #23505 and resolves #23502