SNOW-1825789 Secure token cache #1012
Conversation
| return false; | ||
| } | ||
| const cacheStat = await fs.lstat(cacheDir).catch(() => {}); | ||
| if (!Util.exists(cacheStat)) { |
There was a problem hiding this comment.
In this logic, the empty cacheStats is the correct equivalent for the directory that doesn't exist. We also return empty even for any cached error.
There was a problem hiding this comment.
Looking back at it, I should probably check if the error is ENOENT and only ignore that one.
| if (process.platform !== 'win32') { | ||
| options.mode = 0o700; | ||
| } | ||
| await fs.mkdir(cacheDir, options); |
There was a problem hiding this comment.
According to the previous comment, we can skip verification checking the directory exists?
There was a problem hiding this comment.
With the fix mentioned above, we should enter this branch iff the directory doesn't exist
| if ((cacheStat.mode & 0o777) === 0o700) { | ||
| return true; | ||
| } | ||
| await fs.chmod(cacheDir, 0o700); |
There was a problem hiding this comment.
Why is the permission changing for the existing directory? For me it is unsafe ...
There was a problem hiding this comment.
This is in accordance with https://docs.google.com/document/d/1taFHFZNaKr3D5kPwOJw-0PDGoFKB2TFaMtx6gSZ5iY4/edit?usp=sharing , section "Location"
| }; | ||
|
|
||
|
|
||
| this.withFileLocked = async function (fun) { |
There was a problem hiding this comment.
Should we treat fun as a callback?
There was a problem hiding this comment.
Well, I'm not sure how you define callback. It is called within the function, but not at the end of the function, as we need to cleanup after
| const { getSecureHandle } = require('../../file_util'); | ||
|
|
||
| function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) { | ||
| const topLevelKey = 'tokens'; |
There was a problem hiding this comment.
It would be more readable for me if the name indicated its relation to the token.
lib/file_util.js
Outdated
| @@ -164,7 +164,9 @@ exports.validateOnlyUserReadWritePermissionAndOwner = async function (filePath, | |||
| if (octalPermissions === '600') { | |||
| Logger.getInstance().debug(`Validated that the user has only read and write permission for file: ${filePath}, Permission: ${permission}`); | |||
| } else { | |||
| throw new Error(`Invalid file permissions (${octalPermissions} for file ${filePath}). Make sure you have read and write permissions and other users do not have access to it. Please remove the file and re-run the driver.`); | |||
| await fsp.chmod(filePath, 0o600).catch(() => { | |||
There was a problem hiding this comment.
Why are we changing permission for existing file? Potentially, it could be modified by an attacker.
There was a problem hiding this comment.
From the design doc: "If cache file doesn’t have exactly 600 permissions: all operations should attempt to change the permissions to 600 or fail if not possible"
That being said, as I ended up creating a new function based on this one and no longer use it, I can revert this change
| const permission = mode & 0o777; | ||
|
|
||
| //This should be 600 permission, which means the file permission has not been changed by others. | ||
| const octalPermissions = permission.toString(8); |
There was a problem hiding this comment.
Isn't it duplication of validateOnlyUserReadWritePermissionAndOwner method?
There was a problem hiding this comment.
It is very similar, but operates on file handles in order to mitigate any manipulations between our checks and operations. validateOnlyUserReadWritePermissionAndOwner should probably be replaced with this later on, but I didn't want to do it in this PR. See https://snowflakecomputing.atlassian.net/browse/SNOW-1944224
test/integration/testCache.js
Outdated
| @@ -21,16 +21,6 @@ describe('Validate cache permissions test', async function () { | |||
| await fs.unlink(validPermissionsFilePath); | |||
| }); | |||
|
|
|||
| it('should return error on insecure permissions', async function () { | |||
There was a problem hiding this comment.
Should we remove this test? why?
There was a problem hiding this comment.
This was removed because of the change to the tested function that attempted to change the permissions. I'll restore it together with reverting the mentioned change
| const pathFromHome = function () { | ||
| switch (process.platform) { | ||
| case 'win32': | ||
| return ['AppData', 'Local', 'Snowflake', 'Caches']; |
There was a problem hiding this comment.
We use the paths in JsonCredentialManager. What du you thin about extracting subfolders to some variables?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1012 +/- ##
==========================================
- Coverage 88.91% 88.88% -0.04%
==========================================
Files 72 72
Lines 7003 7124 +121
==========================================
+ Hits 6227 6332 +105
- Misses 776 792 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.