Skip to content

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.

4 participants