[RBAC] az ad sp credential reset: modify credential generation to avoid troublesome special characters

[chef-davin]

Signed-off-by: Davin Taddeo [email protected]

Description

This is an update to the change made in #13357 in the hopes of fixing issue #13625. I updated the new _random_password() method to avoid special characters that can cause problems in a lot of scripting languages.

Testing Guide

az ad sp credential reset -n test

Expected Result: password/secret should be a string with at least one special character, but whose first character is alpha-numeric and does not contain dangerous special characters like !$#;/

History Notes

[RBAC] az ad sp credential reset: modify credential generation to avoid troublesome special characters

---

This checklist is used to make sure that common guidelines for a pull request are followed.

• The PR title and description has followed the guideline in Submitting Pull Requests.

• I adhere to the Command Guidelines.

[msftclas]

All CLA requirements met.

[yonzhan]

add to S170

[jiasli]

Thank you very much for submitting the PR.

I don't think <>() are safe when they are not quoted. (Some users new to bash may forget to do that.) <> can be parsed as stdin stdout redirection, and () can be parsed as subshell execution:

$ echo a>b
# Output is redirected

$ echo (ab)
-bash: syntax error near unexpected token `ab'

% is also not safe in a Windows Command Prompt: https://ss64.com/nt/syntax-percent.html

We will sync internally with the security team first and try to find out the best solution.

The possible solution is either of:

1. The first character must be a letter or number, then the rest characters can only use punctuation in -_.~
2. Show a warning when generating the password, something like "when using the password, it must be quoted to avoid shell interpretation"

[jiasli]

Microsoft Graph API generates the password by the service itself, instead of letting the user generate one #12561 (comment):

$ az rest -m "POST" -u https://graph.microsoft.com/v1.0/applications/b4e4d2ab-e2cb-45d5-a31a-98eb3f364001/addPassword --headers "Content-Type=application/json" -b '{"passwordCredential":{"displayName":"Password friendly name"}}'
{
  "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#microsoft.graph.passwordCredential",
  "customKeyIdentifier": null,
  "displayName": "Password friendly name",
  "endDateTime": "2022-05-25T03:39:02.2389196Z",
  "hint": "4cc",
  "keyId": "b50bcc87-0f02-4aaf-b5c0-a871189871cb",
  "secretText": "4ccP5-~jc_fWK_4AJ-pzn.y2iNy~51tGnU",
  "startDateTime": "2020-05-25T03:39:02.2389196Z"
}

It only uses punctuation marks from -_.~. Some more examples:

4ccP5-~jc_fWK_4AJ-pzn.y2iNy~51tGnU
Pn~2983OPGQfi0.T.Jaai~v3664xRreKZi
-tw1b2Y46gfnzw0-2~WtfsbTpKy4q9H~8S
--zz5zs._yz-zSC5P~QcR23MNku3mQ2TVg
k-39_0y_83qmHgm.oMkypb._F7T7.5-Jbz
vaeV.4vDtu1HW~e-Fn-O.PM57leL1DqDa6
b2.vx-7Q9~Kn~Sc2Ii5dGnbEhPBj-1-PS7
~y4_UnMLgnnBu5.aXAQj-0j0X2p1-UEk_n

[jiasli]

If the password starts with dash -, Python's argparse will treat it as a parameter, like --name, instead of an argument. See #10692.

For now, even if we only use letter or number as the first character, this issue will emerge after we migrate to Microsoft Graph (#12946).

[jiasli]

A better choice is to use secrets.choice or random.SystemRandom().choice which is the the "most secure source of randomness" instead of random.choice.

The Python official doc clearly says: https://docs.python.org/3/library/random.html#module-random

Warning: The pseudo-random generators of this module should not be used for security purposes. For security or cryptographic uses, see the secrets module.

One problem is that secrets is introduced in Python 3.6. I am not sure whether using it will cause problem in Python 3.5 for which we have already dropped support. + @fengzhou-msft

On the other hand, random.SystemRandom() is supported by Python 3.5 and is what secrets.choice internally uses.

[qianwens]

@chef-davin thanks for your contribution:) After internal discussion, we decided to use punctuation from '-_.~' to align with portal behavior.

[arrownj]

For _random_password function itself, do we need to consider the case when length is less than 5 ?

[qianwens]

the length is 34 which is not configurable for the customer

[qianwens]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[chef-davin]

Sorry for not responding to comments sooner. Thank you all for improving the code changes I submitted and pushing this change. I think it'll definitely make things a bit better in the future.

[singularpoint]

Just FYI, this problem cases problems with Azure IoT Edge runtime, because this special symbols in password broke the recommended way to connect to Azure Container Registry with service principal credentials.

Azure Edge stores the credentials in the deployment manifest as a plaintext stings in JSON, so it may be connected with JSON deserialization when doing Docker authorization.

[lastcoolnameleft]

IIRC, it used to generate a GUID for the password, which I thought was brilliant when I first encountered it. Easy to copy/paste and automate with no special characters other than '-'.

[jiasli]

IIRC, it used to generate a GUID for the password, which I thought was brilliant when I first encountered it. Easy to copy/paste and automate with no special characters other than '-'.

Hi @lastcoolnameleft, the current implementation only contains -_.~ as special characters, which shouldn't have any issue in popular shells.