Skip to content

SNOW-1825789 Secure token cache #1012

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

Open
wants to merge 60 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
d72bac3
Secure token cache
sfc-gh-astachowski Feb 18, 2025
89e3b4b
Merge branch 'master' into SNOW-1825789-secure-token-cache
sfc-gh-astachowski Feb 18, 2025
ada2872
Test fixes
sfc-gh-astachowski Feb 18, 2025
f638e60
Disable tests on windows
sfc-gh-astachowski Feb 18, 2025
c9c6bd6
Enable tests on windows
sfc-gh-astachowski Feb 18, 2025
a431cb8
Diagnostic logs
sfc-gh-astachowski Feb 18, 2025
770e3b1
Potential windows fix
sfc-gh-astachowski Feb 18, 2025
7e3dc4f
More logs
sfc-gh-astachowski Feb 18, 2025
dcc6d2e
Attempted fix
sfc-gh-astachowski Feb 20, 2025
d51750d
Added error log
sfc-gh-astachowski Feb 20, 2025
b0c0752
Windows fix
sfc-gh-astachowski Feb 20, 2025
1c3dc35
Fixes
sfc-gh-astachowski Feb 20, 2025
f813df5
Extra logging
sfc-gh-astachowski Feb 20, 2025
9cb1c16
Improved logging
sfc-gh-astachowski Feb 21, 2025
03e376f
Improved logging
sfc-gh-astachowski Feb 21, 2025
800a58a
Added more logging
sfc-gh-astachowski Feb 21, 2025
47a684a
Test fixes
sfc-gh-astachowski Feb 21, 2025
7f4afd4
Test fixes
sfc-gh-astachowski Feb 21, 2025
ef6e428
Further test fixes
sfc-gh-astachowski Feb 21, 2025
d279ca6
Added even more logs
sfc-gh-astachowski Feb 21, 2025
52a088c
Change to util exists
sfc-gh-astachowski Feb 21, 2025
43e1040
Improved cleanup
sfc-gh-astachowski Feb 21, 2025
6d49ccf
More logs
sfc-gh-astachowski Feb 21, 2025
1778e9f
More logs
sfc-gh-astachowski Feb 21, 2025
7a1b31d
Fixes, cleanup
sfc-gh-astachowski Feb 21, 2025
c99010b
Merge branch 'master' into SNOW-1825789-secure-token-cache
sfc-gh-astachowski Feb 21, 2025
cdc5484
Sym link fix
sfc-gh-astachowski Feb 25, 2025
3ba209a
Added key hashing
sfc-gh-astachowski Feb 25, 2025
948cd27
Switched to file handles
sfc-gh-astachowski Feb 25, 2025
8e9e85d
Windows fix
sfc-gh-astachowski Feb 25, 2025
13e04df
Windows fix?
sfc-gh-astachowski Feb 25, 2025
90f758d
Windows fix?
sfc-gh-astachowski Feb 25, 2025
f1e8ebf
Logging
sfc-gh-astachowski Feb 25, 2025
74c6d1c
Remove logs
sfc-gh-astachowski Feb 25, 2025
5f2cf3c
Close description in case of loch error
sfc-gh-astachowski Feb 25, 2025
d5898c9
Close descriptor before opening a new one
sfc-gh-astachowski Feb 28, 2025
2d65bbf
Change flag to integer
sfc-gh-astachowski Feb 28, 2025
47e6449
revert change flag to integer
sfc-gh-astachowski Feb 28, 2025
3f66eb2
Some logging
sfc-gh-astachowski Feb 28, 2025
dc36c39
Additional cleanup
sfc-gh-astachowski Feb 28, 2025
3097c53
Handle closing fix
sfc-gh-astachowski Feb 28, 2025
c036882
Change error handling
sfc-gh-astachowski Mar 3, 2025
f2ea3b3
Change flags to use NOFOLLOW
sfc-gh-astachowski Mar 3, 2025
a24a749
Force cleanup
sfc-gh-astachowski Mar 3, 2025
50e7e39
Close file handles in tests
sfc-gh-astachowski Mar 3, 2025
25f6ccb
Review improvements
sfc-gh-astachowski Mar 7, 2025
86fd21b
Fix imports
sfc-gh-astachowski Mar 7, 2025
3514cd7
Merge branch 'master' into SNOW-1825789-secure-token-cache
sfc-gh-astachowski Mar 7, 2025
81a56df
Permission fixes
sfc-gh-astachowski Mar 10, 2025
24995a4
Add directory check
sfc-gh-astachowski Mar 11, 2025
3048378
Remove anti-symlink flag
sfc-gh-astachowski Mar 11, 2025
0140865
Remove chmod
sfc-gh-astachowski Mar 14, 2025
76462a5
Review fixes
sfc-gh-astachowski Mar 21, 2025
1d15e2e
Removed unused import
sfc-gh-astachowski Mar 21, 2025
380c2f0
Review feedback
sfc-gh-astachowski Mar 24, 2025
cf9b613
Flag changes
sfc-gh-astachowski Mar 25, 2025
55a6b4b
Removed duplicated code
sfc-gh-astachowski Mar 25, 2025
aed35d0
Extract retry interval to a constant
sfc-gh-astachowski Mar 25, 2025
29c74f2
Review improvements
sfc-gh-astachowski Mar 31, 2025
669e214
Merge branch 'master' into SNOW-1825789-secure-token-cache
sfc-gh-dheyman Apr 2, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
231 changes: 191 additions & 40 deletions lib/authentication/secure_storage/json_credential_manager.js
Original file line number Diff line number Diff line change
@@ -1,82 +1,233 @@
const path = require('path');
const Logger = require('../../logger');
const fs = require('node:fs/promises');
const os = require('os');
const Util = require('../../util');
const { validateOnlyUserReadWritePermissionAndOwner } = require('../../file_util');
const os = require('os');
const crypto = require('crypto');
const { getSecureHandle } = require('../../file_util');

function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {
const topLevelKey = 'tokens';

this.hashKey = function (key) {
return crypto.createHash('sha256').update(key).digest('hex');
};

this.getTokenDirCandidates = function () {
const candidates = [];
if (Util.exists(credentialCacheDir)) {
candidates.push({ folder: credentialCacheDir, subfolders: [] });
}
const sfTemp = process.env.SF_TEMPORARY_CREDENTIAL_CACHE_DIR;
if (Util.exists(sfTemp)) {

Choose a reason for hiding this comment

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

Checking if this exists seems like redundant code. Can we do it in tryTokenDir?

Copy link
Author

Choose a reason for hiding this comment

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

Done

candidates.push({ folder: sfTemp, subfolders: [] });
}
const xdgCache = process.env.XDG_CACHE_HOME;
if (Util.exists(xdgCache) && process.platform === 'linux') {
candidates.push({ folder: xdgCache, subfolders: ['snowflake'] });
}
const home = process.env.HOME;
switch (process.platform) {

Choose a reason for hiding this comment

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

Nit: it's hard to see what's returned for each system. Can you do it like this

switch(proces.platform) {
  case 'win32':
    return [ ... ]
  case 'linux':
    return [ ... ]
  case 'darwin':
    return [ ... ]
}

Copy link
Author

Choose a reason for hiding this comment

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

Didn't do exactly that, but without the null/undefined checks the function is much more readable now

Choose a reason for hiding this comment

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

Looks good

case 'win32':
candidates.push({ folder: os.homedir(), subfolders: ['AppData', 'Local', 'Snowflake', 'Caches'] });
break;
case 'linux':
if (Util.exists(home)) {
candidates.push({ folder: home, subfolders: ['.cache', 'snowflake'] });
}
break;
case 'darwin':
if (Util.exists(home)) {
candidates.push({ folder: home, subfolders: ['Library', 'Caches', 'Snowflake'] });
}
}
return candidates;
};

this.tryTokenDir = async function (dir, subDirs) {
const cacheDir = path.join(dir, ...subDirs);
try {
const stat = await fs.stat(dir);
if (!stat.isDirectory()) {
Logger.getInstance().info(`Path ${dir} is not a directory`);
return false;
}
const cacheStat = await fs.lstat(cacheDir).catch(() => {});
if (!Util.exists(cacheStat)) {
const options = { recursive: true };
if (process.platform !== 'win32') {
options.mode = 0o700;

Choose a reason for hiding this comment

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

We are using here 0o700 permissions and recursive. This is problematic since for example:

  • 0o700 is too narrow for $HOME/.cache directory (this directory has usually 0755 on mac and linux systems)

Copy link
Author

Choose a reason for hiding this comment

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

Fair, changed to creating recursively with 0o755, then changing the final directory's permission to 0o700

}
await fs.mkdir(cacheDir, options);
return true;
} else {
if (process.platform === 'win32') {
return true;
}
if ((cacheStat.mode & 0o777) === 0o700) {

Choose a reason for hiding this comment

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

We should also check owner

Copy link
Author

Choose a reason for hiding this comment

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

Added

return true;
}
await fs.chmod(cacheDir, 0o700);
return true;
}
} catch (err) {
Logger.getInstance().warn(`The path location ${cacheDir} is invalid. Please check this location is accessible or existing`);
return false;
}
};

function JsonCredentialManager(credentialCacheDir) {

this.getTokenDir = async function () {
let tokenDir = credentialCacheDir;
if (!Util.exists(tokenDir)) {
tokenDir = os.homedir();
} else {
Logger.getInstance().info(`The credential cache directory is configured by the user. The token will be saved at ${tokenDir}`);
const candidates = this.getTokenDirCandidates();
for (const candidate of candidates) {
const { folder: dir, subfolders: subDirs } = candidate;
if (await this.tryTokenDir(dir, subDirs)) {
return path.join(dir, ...subDirs);
} else {
Logger.getInstance().info(`${path.join(dir, ...subDirs)} is not a valid cache directory`);
}
}
return null;
};

this.getTokenFile = async function () {
const tokenDir = await this.getTokenDir();

if (!Util.exists(tokenDir)) {
throw new Error(`Temporary credential cache directory is invalid, and the driver is unable to use the default location(home).
throw new Error(`Temporary credential cache directory is invalid, and the driver is unable to use the default location.
Please set 'credentialCacheDir' connection configuration option to enable the default credential manager.`);
}

const tokenCacheFile = path.join(tokenDir, 'temporary_credential.json');
await validateOnlyUserReadWritePermissionAndOwner(tokenCacheFile);
return tokenCacheFile;
const tokenCacheFile = path.join(tokenDir, 'credential_cache_v1.json');
return [await getSecureHandle(tokenCacheFile, fs.constants.O_RDWR | fs.constants.O_CREAT, fs), tokenCacheFile];

Choose a reason for hiding this comment

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

I have an issue with how this function is used and O_CREAT flag. It seems to me that it will create cache file (empty one) during read-only operations (retrieving key), and creating file lock

Copy link
Author

Choose a reason for hiding this comment

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

Good point, changed

};

this.readJsonCredentialFile = async function () {
this.readJsonCredentialFile = async function (fileHandle) {
try {
const cred = await fs.readFile(await this.getTokenDir(), 'utf8');
const cred = await fileHandle.readFile('utf8');
return JSON.parse(cred);
} catch (err) {
Logger.getInstance().warn('Failed to read token data from the file. Err: %s', err.message);
return null;
}
};

this.removeStale = async function (file) {
const stat = await fs.stat(file).catch(() => {
return undefined;
});
if (!Util.exists(stat)) {
return;
}
if (new Date().getTime() - stat.birthtimeMs > timeoutMs) {
try {
await fs.rmdir(file);
} catch (err) {
Logger.getInstance().warn('Failed to remove stale file. Error: %s', err.message);
}
}

};


this.withFileLocked = async function (fun) {
const [fileHandle, file] = await this.getTokenFile();
const lckFile = file + '.lck';
await this.removeStale(lckFile);
let attempts = 1;
let locked = false;
const options = {};
if (process.platform !== 'win32') {
options.mode = 0o600;
}
while (attempts <= 10) {
Logger.getInstance().debug('Attempting to get a lock on file %s, attempt: %d', file, attempts);
attempts++;
await fs.mkdir(lckFile, options).then(() => {
locked = true;
}, () => {});
if (locked) {
break;
}
await new Promise(resolve => setTimeout(resolve, 100));
}
if (!locked) {
if (Util.exists(fileHandle)) {
await fileHandle.close();
}
Logger.getInstance().warn('Could not acquire lock on cache file %s', file);
return null;
}
const res = await fun(fileHandle, file);

Choose a reason for hiding this comment

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

Nit: Can you more the file locking functionality to a seperate object/functions? I would like this function to look similar to this:

const lck = lockFile()
const res = func(fileHandle, file)
lck.unlock()

Copy link
Author

Choose a reason for hiding this comment

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

Changed it a bit, is this better?

if (Util.exists(fileHandle)) {
await fileHandle.close();
}
await fs.rmdir(lckFile);
return res;
};

this.write = async function (key, token) {
if (!validateTokenCacheOption(key)) {
return null;
}

const jsonCredential = await this.readJsonCredentialFile() || {};
jsonCredential[key] = token;

try {
await fs.writeFile(await this.getTokenDir(), JSON.stringify(jsonCredential), { mode: 0o600 });
} catch (err) {
throw new Error(`Failed to write token data. Please check the permission or the file format of the token. ${err.message}`);
}
const keyHash = this.hashKey(key);

await this.withFileLocked(async (fileHandle, filename) => {
const jsonCredential = await this.readJsonCredentialFile(fileHandle) || {};
if (!Util.exists(jsonCredential[topLevelKey])) {
jsonCredential[topLevelKey] = {};
}
jsonCredential[topLevelKey][keyHash] = token;

try {
const flag = Util.exists(fileHandle) ? fs.constants.O_RDWR | fs.constants.O_CREAT : fs.constants.O_WRONLY;
const writeFileHandle = await getSecureHandle(filename, flag, fs);
await writeFileHandle.writeFile(JSON.stringify(jsonCredential), { mode: 0o600 });
await writeFileHandle.close();
} catch (err) {
Logger.getInstance().warn(`Failed to write token data in ${filename}. Please check the permission or the file format of the token. ${err.message}`);
}
});
};

this.read = async function (key) {
if (!validateTokenCacheOption(key)) {
return null;
}

const jsonCredential = await this.readJsonCredentialFile();
if (!!jsonCredential && jsonCredential[key]){
return jsonCredential[key];
} else {
return null;
}
const keyHash = this.hashKey(key);

return await this.withFileLocked(async (fileHandle) => {
const jsonCredential = await this.readJsonCredentialFile(fileHandle);
if (!!jsonCredential && jsonCredential[topLevelKey] && jsonCredential[topLevelKey][keyHash]) {
return jsonCredential[topLevelKey][keyHash];
} else {
return null;
}
});
};

this.remove = async function (key) {
if (!validateTokenCacheOption(key)) {
return null;
}
const jsonCredential = await this.readJsonCredentialFile();

if (jsonCredential && jsonCredential[key]) {
try {
jsonCredential[key] = null;
await fs.writeFile(await this.getTokenDir(), JSON.stringify(jsonCredential), { mode: 0o600 });
} catch (err) {
throw new Error(`Failed to write token data from the file in ${await this.getTokenDir()}. Please check the permission or the file format of the token. ${err.message}`);
}
}

const keyHash = this.hashKey(key);

await this.withFileLocked(async (fileHandle, filename) => {
const jsonCredential = await this.readJsonCredentialFile(fileHandle);

if (jsonCredential && jsonCredential[topLevelKey] && jsonCredential[topLevelKey][keyHash]) {
try {
jsonCredential[topLevelKey][keyHash] = null;
const flag = Util.exists(fileHandle) ? fs.constants.O_RDWR | fs.constants.O_CREAT : fs.constants.O_WRONLY;
const writeFileHandle = await getSecureHandle(filename, flag, fs);
await writeFileHandle.writeFile(JSON.stringify(jsonCredential), { mode: 0o600 });
await writeFileHandle.close();
} catch (err) {
Logger.getInstance().warn(`Failed to remove token data from the file in ${filename}. Please check the permission or the file format of the token. ${err.message}`);
}
}
});
};

function validateTokenCacheOption(key) {
Expand Down
49 changes: 48 additions & 1 deletion lib/file_util.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
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.`);
});
}

const userInfo = os.userInfo();
Expand All @@ -183,6 +185,51 @@ exports.validateOnlyUserReadWritePermissionAndOwner = async function (filePath,
}
};

/**
* Checks if the provided file is writable only by the user and os tha file owner is the same as os user. FsPromises can be provided.
* @param filePath
* @param expectedMode
* @param fsPromises
* @returns {Promise<FileHandle>}
*/
exports.getSecureHandle = async function (filePath, flags, fsPromises) {
const fsp = fsPromises ? fsPromises : require('fs/promises');
try {
const fileHandle = await fsp.open(filePath, flags | fsp.constants.O_NOFOLLOW, 0o600);
if (os.platform() === 'win32') {
return fileHandle;
}
const stats = await fileHandle.stat();
const mode = stats.mode;
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it duplication of validateOnlyUserReadWritePermissionAndOwner method?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue you attached doesn't address the migration of control for configuration files. Do we need to create a new issue?

Choose a reason for hiding this comment

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

Why convert it to string? just compare with octal value?

Copy link
Author

Choose a reason for hiding this comment

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

This was copied from an analogous function, fixed

if (octalPermissions === '600') {
Logger.getInstance().debug(`Validated that the user has only read and write permission for file: ${filePath}, Permission: ${permission}`);
} else {
await fileHandle.chmod(0o600).catch(() => {
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.`);
});
}

const userInfo = os.userInfo();
if (stats.uid === userInfo.uid) {
Logger.getInstance().debug('Validated file owner');
} else {
throw new Error(`Invalid file owner for file ${filePath}). Make sure the system user are the owner of the file otherwise please remove the file and re-run the driver.`);
}
return fileHandle;
} catch (err) {
//When file doesn't exist - return
if (err.code === 'ENOENT') {
return null;
} else {
throw err;
}
}
};

/**
* Checks if the provided file or directory permissions are correct.
* @param filePath
Expand Down
10 changes: 1 addition & 9 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ exports.buildCredentialCacheKey = function (host, username, credType) {
Logger.getInstance().debug('Cannot build the credential cache key because one of host, username, and credType is null');
return null;
}
return `{${host.toUpperCase()}}:{${username.toUpperCase()}}:{SF_NODE_JS_DRIVER}:{${credType.toUpperCase()}}`;
return `{${host.toUpperCase()}}:{${username.toUpperCase()}}:{${credType.toUpperCase()}}`;
};

/**
Expand All @@ -645,14 +645,6 @@ exports.checkParametersDefined = function (...parameters) {
return parameters.every((element) => element !== undefined && element !== null);
};

exports.buildCredentialCacheKey = function (host, username, credType) {
if (!host || !username || !credType) {
Logger.getInstance().debug('Cannot build the credential cache key because one of host, username, and credType is null');
return null;
}
return `{${host.toUpperCase()}}:{${username.toUpperCase()}}:{SF_NODE_JS_DRIVER}:{${credType.toUpperCase()}}`;
};

/**
*
* @param {Object} customCredentialManager
Expand Down
10 changes: 0 additions & 10 deletions test/integration/testCache.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,6 @@ describe('Validate cache permissions test', async function () {
await fs.unlink(validPermissionsFilePath);
});

it('should return error on insecure permissions', async function () {
await assert.rejects(
validateOnlyUserReadWritePermissionAndOwner(invalidPermissionsFilePath),
(err) => {
assert.match(err.message, /Invalid file permissions/);
return true;
},
);
});

it('should return error when system user is not a file owner', async function () {
const anotherFileOwnerPath = path.join(wrongOwner);
const fsMock = createFsMock()
Expand Down
Loading
Loading