Open austinzh opened 5 months ago
atomic_open is not intended for public consumption. Even still, the sole purpose of it in requests is to extract a CA truststore file from a zip (where necessary) and place it in a given location. In that case, regardless of what one might expect from umask, it's for the user's protection (and this desirable) that a file only be readable and writable by them to avoid someone corrupting the store and introducing an opportunity for a MitM attack by injecting an untrusted root.
Further still, requests itself does not change the permissions on the file (although now that you raise this I believe it should for the reasons I mentioned above) and I suspect the bug is actually in one of the standard library functions we use to implement this atomic open function, not requests.
I've been working on the issue with requests.utils.atomic_open not respecting the umask settings, and I've tried a few different approaches to fix it. Here’s what I’ve done so far and what I’ve found:
Original Approach: Initially, I used tempfile.mkstemp to create a temporary file, and then replaced it with the target file. However, the file permissions still ended up being the default settings due to umask.
Adjusting Permissions:
Added os.chmod(tmp_name, desired_permissions)
before and after replacing the file to explicitly set permissions on the temporary file. This approach still resulted in the final file having default umask
permissions (e.g., 0o666
), rather than the desired permissions (e.g., 0o777
).
Double os.chmod
Call:
Modified the function to include a second os.chmod
call after replacing the temporary file with the target file. This approach also failed to achieve the desired permissions, consistently resulting in default permissions.
umask
settings (e.g., 0o666
) rather than the intended settings (e.g., 0o777
).umask
is applied or handled during file creation and replacement.@contextlib.contextmanager
def atomic_open(filename):
# Create a temporary file with default permissions
tmp_descriptor, tmp_name = tempfile.mkstemp(dir=os.path.dirname(filename))
try:
with os.fdopen(tmp_descriptor, "wb") as tmp_handler:
yield tmp_handler
# Get the current umask and restore it after setting desired permissions
current_umask = os.umask(0)
os.umask(current_umask)
desired_permissions = 0o777 & ~current_umask
# Set permissions of the temporary file
os.chmod(tmp_name, desired_permissions)
# Replace the target file with the temporary file
os.replace(tmp_name, filename)
# Ensure the final file has the correct permissions
os.chmod(filename, desired_permissions)
# Verify and print permissions of the replaced file
file_stat = os.stat(filename)
print(f"Desired permissions: {oct(desired_permissions)}")
print(f"Actual permissions after replacement: {oct(file_stat.st_mode & 0o777)}")
except Exception as e:
# Clean up the temporary file if an exception occurs
os.remove(tmp_name)
raise e
Thank you for your attention to this matter. Any insights or suggestions would be greatly appreciated.
Create a new file using requests.utils. atomic_open with umask set to 0o000
Expected Result
0o777
Actual Result
0o600
Reproduction Steps
System Information