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

Consider sanitizing all known secret values in text when serializing recordings #2378

Closed
heaths opened this issue Dec 2, 2021 · 4 comments
Closed
Assignees
Labels
Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.

Comments

@heaths
Copy link
Member

heaths commented Dec 2, 2021

Currently we rely on known secret variable names or JSONPath to denote secret values in test recordings. For example, anywhere a variable named "MySecret" is used would be sanitized, or - after spending the compute to deserialize a JSON payload (if JSON; XML not supported currently) - a property matching some JSONPath should be sanitized.

But where secret values exist in text, relying only on JSONPath still requires a developer to input this and is prone to mistakes. While JSONPath also solves problems where secrets are output from services and not known at dev time (i.e. we need to keep support), I think it's worth considering something similar to what I did in Search: https://github.com/Azure/azure-sdk-for-net/blob/71054d50e35c7ebf5eee0a185c48dab702f6fb82/sdk/search/Azure.Search.Documents/tests/Utilities/SearchRecordedTestSanitizer.cs#L93-L125

Effectively, using both an as-is copy and JSON-encoded copy of the secret value search all text matching this value when saving a recording and sanitizing it. Because this could be expensive by default, we could still have developers opt into it, but wouldn't need to know all possible JSONPaths up front (and mitigates the case where the service may add some different patterns).

@heaths
Copy link
Member Author

heaths commented Dec 2, 2021

On a related note, @JoshLove-msft also suggested a great idea that could benefit any language by somehow hooking up CredScan to run on recordings using the test proxy.

/cc @pakrym

@pakrym pakrym transferred this issue from Azure/azure-sdk-for-net Dec 2, 2021
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 2, 2021
@pakrym pakrym added the EngSys This issue is impacting the engineering system. label Dec 2, 2021
@weshaggard
Copy link
Member

I pointed @scbedd to the CredScan as a library and I believe he has an issue tracking that for investigation.

@kurtzeborn kurtzeborn added Central-EngSys This issue is owned by the Engineering System team. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Dec 6, 2021
@kurtzeborn
Copy link
Member

@scbedd, pretty sure you already have an issue tracking this. If I'm right, go ahead and link to it here and close this one. We only need one issue to track work.

@scbedd
Copy link
Member

scbedd commented Dec 7, 2021

Yeah, duplicate of #1950. Closing this one.

@scbedd scbedd closed this as completed Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.
Projects
None yet
Development

No branches or pull requests

5 participants