Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Mar 21, 2022

Context

When creating a self-signed certificate in commands such as az ad sp create-for-rbac or az ad app credential reset:

  1. Create a temp_file and use its name for creds_file.
  2. The certificate and private key are saved to cert_file and key_file.
  3. The contents of cert_file and key_file are read back and combined into creds_file.
  4. Change the mode of creds_file to 0o600 (rw-------).

This procedure exposes problems:

Problem 1: temp_file is not deleted

According to https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp:

Unlike TemporaryFile(), the user of mkstemp() is responsible for deleting the temporary file when done with it.

At least, the temp file made by mkstemp is empty with permission 0o600, so it doesn't leak any credential. It's just it will be left as trash.

Solution: Use NamedTemporaryFile which automatically deletes the temp file.

Problem 2: cert_file and key_file are not deleted

The "write to file and read it back" logic was introduced by #2457. The reason behind it is unknown. At least, they are secure with permission 0o600, so no one else can read them:

$ ls -ld /tmp/tmp*
-rw------- 1 user1 user1    0 Mar 21 15:37 /tmp/tmp7v82yx_y
-rw------- 1 user1 user1 1704 Mar 21 18:57 /tmp/tmpavuictrh
-rw------- 1 user1 user1  973 Mar 21 18:57 /tmp/tmphv9npp_n

Solution: Only keep certificate and key values in memory as variables. Do not write them to files.

Problem 3: Time gap between creds_file's open and chmod

There is a time gap between creds_file's open and chmod, during which its permission is 0o755 (rwxr-xr-x) meaning other users can read its content.

Solution: Create creds_file with the right mode, instead of changing it later. ADAL-based Azure CLI uses this approach for creating accessTokens.json:

with os.fdopen(os.open(self._token_file, os.O_RDWR | os.O_CREAT | os.O_TRUNC, 0o600),
'w+') as cred_file:

msal-extensions made the same change in AzureAD/microsoft-authentication-extensions-for-python#107.

References

@ghost ghost requested a review from yonzhan March 21, 2022 11:25
@ghost ghost added the Auto-Assign Auto assign by bot label Mar 21, 2022
@ghost ghost assigned jiasli Mar 21, 2022
@ghost ghost added this to the Mar 2022 (2022-04-06) milestone Mar 21, 2022
@ghost ghost added the RBAC az role label Mar 21, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 21, 2022

Role improvement

Comment on lines +1549 to +1551
with tempfile.NamedTemporaryFile() as f:
temp_file_name = f.name
creds_file = path.join(path.expanduser("~"), path.basename(temp_file_name) + '.pem')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better solution is to create a random name by ourselves, or even create a PEM file with the current time, such as

  • Epoch time: cert-1647862488.pem
  • Datetime, like the default name created by az ad sp create-for-rbac: cert-2022-03-21-11-09-22.pem

@jiasli jiasli merged commit 0ccadba into Azure:dev Mar 24, 2022
@jiasli jiasli deleted the create-cert branch March 24, 2022 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Auto-Assign Auto assign by bot RBAC az role

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants