Add new rule RPC-v1-13 to mandate x-ms-secrets annotation for properties containing secrets #805
Conversation
tejaswiMinnu
requested review from
AkhilaIlla,
mikeharder,
raosuhas and
weshaggard
as code owners
|
@tejaswiMinnu you should be able to follow- https://github.com/Azure/azure-openapi-validator/blob/main/.github/workflows/README-staging-lint-checks.md and test the new rule. |
AkhilaIlla
left a comment
AkhilaIlla
left a comment
There was a problem hiding this comment.
Test the new rule using staging linter workflow and validate if works as expected.
currently its a NoOp as the rule name isnt mentioned in the body or as label-https://github.com/Azure/azure-openapi-validator/actions/runs/19814210448/job/56762071187?pr=805 |
|
I reviewed the staging lintdiff pipeline results. Before adding the typespec logic, there were 180 errors, some of which were false positives. After implementing the typespec logic, the errors reduced to 72, and I checked, there are no false positives now.https://github.com/Azure/azure-openapi-validator/actions/runs/19903790744 |
| function isPotentialSensitiveProperty(propertyName: string): boolean { | ||
| const lowerName = propertyName.toLowerCase() | ||
| return ( | ||
| sensitiveKeywords.some((keyword) => lowerName.endsWith(keyword) && !lowerName.startsWith(keyword)) && |
| @@ -0,0 +1,52 @@ | |||
| const sensitiveKeywords = ["access", "credential", "secret", "password", "key", "token", "auth", "connection"] | |||
|
|
|||
| const excludeKeywords = ["publicKey"].map((keyword) => keyword.toLowerCase()) | |||
There was a problem hiding this comment.
This is to exclude "publicKey" since it is not a secret as suggested by typespec.
To explicitly keep it as "publicKey" for clarity, and this is how it is typically defined in .ts or .json files.
| isPotentialSensitiveProperty(prpName) && // property name matches sensitive keywords | ||
| properties[prpName] && // property exists | ||
| properties[prpName]["x-ms-secret"] !== true && // not explicitly marked as secret | ||
| !keyValuePairCheck && // not a key-value pair key |
There was a problem hiding this comment.
Suggestion from typespec team: "Exclude key when used in a model that only has 1 other property value(Get rid of most key instance as its just key value pair)"
Without this check, we were getting a bunch of false positives. That’s why the typespec team came up with all these new checks, and now they’ve added them to their code too to avoid false positives.
There was a problem hiding this comment.
and why is this- Exclude key when used in a model that only has 1 other property value(Get rid of most key instance as its just key value pair)?
| function isPotentialSensitiveProperty(propertyName: string): boolean { | ||
| const lowerName = propertyName.toLowerCase() | ||
| return ( | ||
| sensitiveKeywords.some((keyword) => lowerName.endsWith(keyword) && !lowerName.startsWith(keyword)) && |
There was a problem hiding this comment.
Yep, same here—the typespec team introduced some new checks to cut down on all those false positives.
There was a problem hiding this comment.
so, passwordXYZ is fine to be defined?
| }) | ||
| }) | ||
|
|
||
| test("XMSSecretInResponse should find no errors with newly added typespec conditions", () => { |
There was a problem hiding this comment.
In the last commit, all checks were sourced from the typespec team. I have added a few additional tests to verify if they align with the new checks.
here is the PR - Azure/typespec-azure#3411
| }, | ||
| token: { | ||
| type: "string", | ||
| }, |
There was a problem hiding this comment.
why is this not flagged?
There was a problem hiding this comment.
This should not be flagged according to the new checks from typespec. "somethingtoken" should fail, but just "token" should not. Some JSONs have fields like tokenName, tokenURL, and tokenValue; these should not be flagged since they are only names and URLs, not secrets.
However, fields such as Resourcetoken, resourceaccess, resourceauth, and resourceconnection are actual secrets and should be flagged.
This reverts commit a73dd47.
When defining the response model for an ARM PUT or GET operation, any property that contains sensitive information (such as passwords, keys, tokens, credentials, or other secrets) must include the "x-ms-secret": true annotation. This ensures that secrets are properly identified and handled according to ARM security guidelines.
The last commit changes were based on suggestions from the typespec team. You can review the typespec PR here: Azure/typespec-azure#3411.
rules: XMSSecretInResponse