Skip to content
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

[SEC-6587] Databricks CLI Tool Config File inherits default system umask #522

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

shreyas-goenka
Copy link
Contributor

@shreyas-goenka shreyas-goenka commented Jul 26, 2022

This PR makes changes so that .databrickscfg file has user only read write permissions at create time rather than the original workflow which is create file -> write token to file -> change permission to user only read write (0o600) which can be used by an adversary to read the token

Tested manually by looking that the permissions on the file after its creation

@shreyas-goenka shreyas-goenka requested review from fjakobs and removed request for fjakobs July 27, 2022 09:08
Copy link
Contributor

@fjakobs fjakobs left a comment

Choose a reason for hiding this comment

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

Code change looks good but please also add a unit test to tests/configure/test_provider.py to verify that the file permissions are indeed 600.

@shreyas-goenka shreyas-goenka requested a review from fjakobs August 1, 2022 10:03
@shreyas-goenka shreyas-goenka force-pushed the SEC-6587 branch 3 times, most recently from 648cfc6 to eb7ae72 Compare August 1, 2022 12:39
@shreyas-goenka shreyas-goenka merged commit 642b6ba into main Aug 1, 2022
@fjakobs fjakobs deleted the SEC-6587 branch August 1, 2022 13:20
@pietern
Copy link
Contributor

pietern commented Aug 10, 2022

@shreyas-goenka Did you test or hypothesize how this works on Windows? It looks risky / like it might break on Windows.

@shreyas-goenka
Copy link
Contributor Author

shreyas-goenka commented Aug 10, 2022

@pietern I did not think about this testing on windows. A quick look at the docs leads me to believe the happy path mostly should be fine i.e. when no .databrickscfg file exists and users use the cli, one with the right permissions should be created.

I believe this because

  1. os.open: works for both windows and unix
  2. os.path.exists: The os availability is not specified in the docs but very highly likely it works on both systems
  3. Before my code we had os.chmod calls which did not break this workflow (Only true if a user actually used windows with our cli and it worked :P)

In any case the python os docs don't make it clear what the behavior would be here. Do we have a windows VM to do a quick test?

@pietern
Copy link
Contributor

pietern commented Aug 10, 2022

Yes, I'll be in touch with details.

@fjakobs
Copy link
Contributor

fjakobs commented Aug 10, 2022

@pietern
Copy link
Contributor

pietern commented Aug 11, 2022

@fjakobs We should, once 1) existing failing tests on Windows are fixed, and 2) we trim the # of builds (no need to test all Python versions on Windows, just 1 of them would be sufficient). Thinking about 2), it's probably easier to define a dedicated Windows test job...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants