Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
138 changes: 73 additions & 65 deletions lib/authentication/secure_storage/json_credential_manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const fs = require('node:fs/promises');
const Util = require('../../util');
const os = require('os');
const crypto = require('crypto');
const { getSecureHandle } = require('../../file_util');
const { getSecureHandle, closeHandle } = require('../../file_util');

const defaultJsonTokenCachePaths = {
'win32': ['AppData', 'Local', 'Snowflake', 'Caches'],
Expand All @@ -21,68 +21,66 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {

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)) {
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;
candidates.push({ folder: credentialCacheDir, subfolders: [] });

candidates.push({ folder: process.env.SF_TEMPORARY_CREDENTIAL_CACHE_DIR, subfolders: [] });

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: module.exports.defaultJsonTokenCachePaths['win32'] });
candidates.push({ folder: os.homedir(), subfolders: defaultJsonTokenCachePaths['win32'] });
break;
case 'linux':
if (Util.exists(home)) {
candidates.push({ folder: home, subfolders: defaultJsonTokenCachePaths['linux'] });
}
candidates.push({ folder: process.env.XDG_CACHE_HOME, subfolders: ['snowflake'] });
candidates.push({ folder: process.env.HOME, subfolders: defaultJsonTokenCachePaths['linux'] });
break;
case 'darwin':
if (Util.exists(home)) {
candidates.push({ folder: home, subfolders: defaultJsonTokenCachePaths['darwin'] });
}
candidates.push({ folder: process.env.HOME, subfolders: defaultJsonTokenCachePaths['darwin'] });
}
return candidates;
};

this.createCacheDir = async function (cacheDir) {
const options = { recursive: true };
if (process.platform !== 'win32') {
options.mode = 0o755;
}
await fs.mkdir(cacheDir, options);
if (process.platform !== 'win32') {
await fs.chmod(cacheDir, 0o700);
}
};

this.tryTokenDir = async function (dir, subDirs) {
if (!Util.exists(dir)) {
return false;
}
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((err) => {
const cacheStat = await fs.stat(cacheDir).catch(async (err) => {
if (err.code !== 'ENOENT') {

Choose a reason for hiding this comment

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

Maybe create dir here when ENOENT? Then stat it again and recover from error?

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 that

throw err;
}
await this.createCacheDir(cacheDir);
return await fs.stat(cacheDir);
});
if (!Util.exists(cacheStat)) {
const options = { recursive: true };
if (process.platform !== 'win32') {
options.mode = 0o700;
}
await fs.mkdir(cacheDir, options);
if (!cacheStat.isDirectory()) {
return false;
}
if (process.platform === 'win32') {
return true;
} else {
if (!stat.isDirectory()) {
return false;
}
if (process.platform === 'win32') {
return true;
}
if ((cacheStat.mode & 0o777) === 0o700) {
return true;
}
}
if ((cacheStat.mode & 0o777) !== 0o700 && cacheStat.uid === os.userInfo().uid) {
Logger.getInstance().warn(`Token cache directory ${cacheDir} has unsecure permissions.`);
return true;
}
if (cacheStat.uid !== os.userInfo().uid) {
Logger.getInstance().warn(`Token cache directory ${cacheDir} has unsecure owner.`);
}
return true;
} catch (err) {
Logger.getInstance().warn(`The path location ${cacheDir} is invalid. Please check this location is accessible or existing`);
return false;
Expand All @@ -95,26 +93,26 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {
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 () {
this.getTokenFilePath = 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.
Please set 'credentialCacheDir' connection configuration option to enable the default credential manager.`);
}

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

this.readJsonCredentialFile = async function (fileHandle) {
if (!Util.exists(fileHandle)) {
return null;
}
try {
const cred = await fileHandle.readFile('utf8');
return JSON.parse(cred);
Expand All @@ -141,10 +139,8 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {

};


this.withFileLocked = async function (fun) {
const [fileHandle, file] = await this.getTokenFile();
const lckFile = file + '.lck';
this.lockFile = async function (filename) {
const lckFile = filename + '.lck';
await this.removeStale(lckFile);
let attempts = 1;
let locked = false;
Expand All @@ -153,7 +149,7 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {
options.mode = 0o600;
}
while (attempts <= 10) {
Logger.getInstance().debug('Attempting to get a lock on file %s, attempt: %d', file, attempts);
Logger.getInstance().debug('Attempting to get a lock on file %s, attempt: %d', filename, attempts);
attempts++;
await fs.mkdir(lckFile, options).then(() => {
locked = true;
Expand All @@ -164,18 +160,24 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {
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);
if (Util.exists(fileHandle)) {
await fileHandle.close();
Logger.getInstance().warn('Could not acquire lock on cache file %s', filename);
}
return locked;
};

this.unlockFile = async function (filename) {
const lckFile = filename + '.lck';
await fs.rmdir(lckFile);
return res;
};

this.withFileLocked = async function (fun) {
const filename = await this.getTokenFilePath();
if (await this.lockFile(filename)) {
const res = await fun(filename);
await this.unlockFile(filename);
return res;
}
return null;
};

this.write = async function (key, token) {
Expand All @@ -184,18 +186,20 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {
}
const keyHash = this.hashKey(key);

await this.withFileLocked(async (fileHandle, filename) => {
await this.withFileLocked(async (filename) => {
const fileHandle = await getSecureHandle(filename, fs.constants.O_RDWR, fs);
const jsonCredential = await this.readJsonCredentialFile(fileHandle) || {};
await closeHandle(fileHandle);
if (!Util.exists(jsonCredential[tokenMapKey])) {
jsonCredential[tokenMapKey] = {};
}
jsonCredential[tokenMapKey][keyHash] = token;

try {
const flag = Util.exists(fileHandle) ? fs.constants.O_RDWR | fs.constants.O_CREAT : fs.constants.O_WRONLY;
const flag = Util.exists(fileHandle) ? fs.constants.O_WRONLY : fs.constants.O_RDWR | fs.constants.O_CREAT;

Choose a reason for hiding this comment

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

Why ternary? isn't it always O_RDWR | O_CREAT?

const writeFileHandle = await getSecureHandle(filename, flag, fs);
await writeFileHandle.writeFile(JSON.stringify(jsonCredential), { mode: 0o600 });
await writeFileHandle.close();
await closeHandle(writeFileHandle);
} 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}`);
}
Expand All @@ -209,8 +213,10 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {

const keyHash = this.hashKey(key);

return await this.withFileLocked(async (fileHandle) => {
return await this.withFileLocked(async (filename) => {
const fileHandle = await getSecureHandle(filename, fs.constants.O_RDWR, fs);
const jsonCredential = await this.readJsonCredentialFile(fileHandle);
await closeHandle(fileHandle);
if (!!jsonCredential && jsonCredential[tokenMapKey] && jsonCredential[tokenMapKey][keyHash]) {
return jsonCredential[tokenMapKey][keyHash];
} else {
Expand All @@ -226,16 +232,18 @@ function JsonCredentialManager(credentialCacheDir, timeoutMs = 60000) {

const keyHash = this.hashKey(key);

await this.withFileLocked(async (fileHandle, filename) => {
await this.withFileLocked(async (filename) => {
const fileHandle = await getSecureHandle(filename, fs.constants.O_RDWR, fs);
const jsonCredential = await this.readJsonCredentialFile(fileHandle);
await closeHandle(fileHandle);

if (jsonCredential && jsonCredential[tokenMapKey] && jsonCredential[tokenMapKey][keyHash]) {
try {
jsonCredential[tokenMapKey][keyHash] = null;
const flag = Util.exists(fileHandle) ? fs.constants.O_RDWR | fs.constants.O_CREAT : fs.constants.O_WRONLY;
const flag = Util.exists(fileHandle) ? fs.constants.O_WRONLY : fs.constants.O_RDWR | fs.constants.O_CREAT;
const writeFileHandle = await getSecureHandle(filename, flag, fs);
await writeFileHandle.writeFile(JSON.stringify(jsonCredential), { mode: 0o600 });
await writeFileHandle.close();
await closeHandle(writeFileHandle);
} 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}`);
}
Expand Down
11 changes: 8 additions & 3 deletions lib/file_util.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,10 @@ exports.getSecureHandle = async function (filePath, flags, fsPromises) {
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);
if (octalPermissions === '600') {
if (permission === 0o600) {
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.`);
throw new Error(`Invalid file permissions (${permission.toString(8)} 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 @@ -226,6 +225,12 @@ exports.getSecureHandle = async function (filePath, flags, fsPromises) {
}
};

exports.closeHandle = async function (fileHandle) {
if (fileHandle !== undefined && fileHandle !== null) {
await fileHandle.close();
}
};

/**
* Checks if the provided file or directory permissions are correct.
* @param filePath
Expand Down
5 changes: 1 addition & 4 deletions test/unit/authentication/json_credential_manager_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,8 @@ const pathFromHome = function () {
};

const assertCachePath = async function (credentialManager, path) {
const [fileHandle, filePath] = await credentialManager.getTokenFile();
const filePath = await credentialManager.getTokenFilePath();
assert.strictEqual(filePath, path);
if (Util.exists(fileHandle)) {
fileHandle.close();
}
};

describe('Json credential manager basic test', function () {
Expand Down
Loading