-
Notifications
You must be signed in to change notification settings - Fork 50
Add new rule RPC-v1-13 to mandate x-ms-secrets annotation for properties containing secrets #805
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
Changes from all commits
9dad461
822ae2f
4cf4c11
575cde4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| # XMSSecretInResponse | ||
|
|
||
| ## Category | ||
|
|
||
| ARM Error | ||
|
|
||
| ## Applies to | ||
|
|
||
| ARM OpenAPI(swagger) specs | ||
|
|
||
| ## Related ARM Guideline Code | ||
|
|
||
| - RPC-Put-V1-13 | ||
|
|
||
| ## Description | ||
|
|
||
| When defining the response model for an ARM PUT/GET/POST 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. | ||
|
|
||
| ## How to fix the violation | ||
|
|
||
| To fix this violation, review the response model for your ARM PUT/GET/POST operation and identify all properties that store secrets (such as `password`, `key`, `access`, `credentials`, `token`, `secret`, `auth`). For each of these properties, add the `"x-ms-secret": true` annotation to ensure they are properly marked as secrets. This helps protect sensitive information and complies with ARM guidelines. | ||
|
|
||
| Note: `The ARM reviewers have determined that the property is a false positive and have verified that it does not contain any secrets, they can suppress the error.` | ||
|
|
||
| ## Good example | ||
|
|
||
| ```json5 | ||
| "Resource": { | ||
| "description": "The resource", | ||
| "properties": { | ||
| "password": { | ||
| "type": "string", | ||
| "description": "secret", | ||
| "x-ms-secret": "true" | ||
| }, | ||
| "accessKey": { | ||
| "type": "string", | ||
| "description": "secret", | ||
| "x-ms-secret": "true" | ||
| }, | ||
| "key": { | ||
| "type": "string", | ||
| "description": "secret", | ||
| "x-ms-secret": "true" | ||
| }, | ||
| "token": { | ||
| "type": "string", | ||
| "description": "secret", | ||
| "x-ms-secret": "true" | ||
| }, | ||
| "credentials": { | ||
| "type": "string", | ||
| "description": "secret", | ||
| "x-ms-secret": "true" | ||
| }, | ||
| "secret": { | ||
| "type": "string", | ||
| "description": "secret", | ||
| "x-ms-secret": "true" | ||
| }, | ||
| "auth": { | ||
| "type": "string", | ||
| "description": "secret", | ||
| "x-ms-secret": "true" | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Bad example | ||
|
|
||
| ```json5 | ||
| "Resource": { | ||
| "description": "The resource", | ||
| "properties": { | ||
| "password": { | ||
| "type": "string", | ||
| "description": "secret" | ||
| // No x-ms-secret annotation | ||
| } | ||
| } | ||
| } | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| const sensitiveKeywords = ["access", "credential", "secret", "password", "key", "token", "auth", "connection"] | ||
|
|
||
| const excludeKeywords = ["publicKey"].map((keyword) => keyword.toLowerCase()) | ||
|
|
||
| function isPotentialSensitiveProperty(propertyName: string): boolean { | ||
| const lowerName = propertyName.toLowerCase() | ||
| return ( | ||
| sensitiveKeywords.some((keyword) => lowerName.endsWith(keyword) && !lowerName.startsWith(keyword)) && | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nvm
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, same here—the typespec team introduced some new checks to cut down on all those false positives.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so, passwordXYZ is fine to be defined? |
||
| !excludeKeywords.some((keyword) => lowerName.endsWith(keyword) && !lowerName.startsWith(keyword)) | ||
| ) | ||
| } | ||
|
|
||
| function isKeyValuePairKeyProp(propertiesKeys: any): boolean { | ||
| return propertiesKeys.includes("key") && propertiesKeys.includes("value") | ||
| } | ||
|
|
||
| export const XMSSecretInResponse = (properties: any, _opts: any, ctx: any) => { | ||
| if (properties === null || typeof properties !== "object") { | ||
| return [] | ||
| } | ||
|
|
||
| const path = ctx.path || [] | ||
| const errors: any[] = [] | ||
| const propertiesSize = Object.keys(properties).length | ||
| const propertiesKeys = Object.keys(properties) | ||
| const keyValuePairCheck = propertiesSize === 2 && isKeyValuePairKeyProp(propertiesKeys) | ||
|
|
||
| // Check top-level and deeply nested properties | ||
| for (const prpName of propertiesKeys) { | ||
| if (prpName === "properties" && typeof properties[prpName] === "object") { | ||
| errors.push(...XMSSecretInResponse(properties[prpName], _opts, { ...ctx, path: [...path, prpName] })) | ||
| } else { | ||
| // Add all conditions for secret detection | ||
| if ( | ||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)? |
||
| properties[prpName].type === "string" // property type is string | ||
| ) { | ||
| errors.push({ | ||
| message: `Property '${prpName}' contains secret keyword and does not have 'x-ms-secret' annotation. To ensure security, must add the 'x-ms-secret' annotation to this property.`, | ||
| path: [...path, prpName], | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return errors | ||
| } | ||
|
|
||
| export default XMSSecretInResponse | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
why map to convert to lowercase and not what you ahev for sensitive keywords? #Resolved
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 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.