SNOW-1895884: Token cache refactor #2044
Conversation
…bc into SNOW-1895884-token-cache-keys
…b/snowflake-jdbc into SNOW-1895884-token-cache-keys
| Map<String, String> hostMap = localCredCache.get(host.toUpperCase()); | ||
| hostMap.put(SecureStorageManager.convertTarget(host, user, type), token); | ||
|
|
||
| localCredCache.computeIfAbsent(CACHE_FILE_TOKENS_OBJECT_NAME, tokensMap -> new HashMap<>()); |
There was a problem hiding this comment.
We should read, update and write cache here under one file lock
There was a problem hiding this comment.
Updated logic
| return null; | ||
| } | ||
|
|
||
| return hostMap.get(SecureStorageManager.convertTarget(host, user, type)); | ||
| return tokensMap.get(SecureStorageManager.buildCredentialsKey(host, user, type)); | ||
| } | ||
|
|
||
| /** May delete credentials which doesn't belong to this process */ | ||
| public synchronized SecureStorageStatus deleteCredential(String host, String user, String type) { |
There was a problem hiding this comment.
We should read, delete entry and write cache here under one file lock
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
sfc-gh-dheyman
force-pushed
the
SNOW-1895884-token-cache-keys
branch
from
74bfcd6 to
6575f5d
Compare
| } | ||
| for (Iterator<Map.Entry<String, JsonNode>> itr = m.fields(); itr.hasNext(); ) { | ||
| for (Iterator<Map.Entry<String, JsonNode>> itr = node.fields(); itr.hasNext(); ) { | ||
| Map.Entry<String, JsonNode> hostMap = itr.next(); |
There was a problem hiding this comment.
Can we change naming? Those are no longer hosts
| private FileCacheManager fileCacheManager; | ||
|
|
||
| private final Map<String, Map<String, String>> localCredCache = new HashMap<>(); | ||
| private final FileCacheManager fileCacheManager; | ||
|
|
||
| private SecureStorageLinuxManager() { | ||
| fileCacheManager = | ||
| FileCacheManager.builder() | ||
| .setCacheDirectorySystemProperty(CACHE_DIR_PROP) | ||
| .setCacheDirectoryEnvironmentVariable(CACHE_DIR_ENV) | ||
| .setBaseCacheFileName(CACHE_FILE_NAME) |
There was a problem hiding this comment.
We should respect XDG_CACHE_HOME
According to XDG specs:
There is a single base directory relative to which user-specific non-essential (cached) data should be written. This directory is defined by the environment variable $XDG_CACHE_HOME.
The design doc:
[Linux only] Directory present in environment var (XDG_CACHE_HOME/snowflake) if set and directory exists or can be created and we can set correct permissions for it (700)
| } | ||
|
|
||
| /** Reads the cache file. */ | ||
| JsonNode readCacheFile() { |
There was a problem hiding this comment.
Do we check permissions for parent directory?
| * @return true if success or false | ||
| */ | ||
| private boolean lockCacheFile() { | ||
| return cacheLockFile.mkdirs(); |
There was a problem hiding this comment.
It seems that this is the place where we create cache directory if it doesn't exits. I think it's not the best place since:
- We can create a $HOME directory by mistake and not just .cache/snowflake directory
- [Less important] We cannot fallback to other directories if we cannot use the one we chose in build function
| @@ -230,32 +249,21 @@ JsonNode readCacheFile() { | |||
|
|
|||
| void writeCacheFile(JsonNode input) { | |||
There was a problem hiding this comment.
Do we check permissions for parent directory? should be 0700
Overview
SNOW-1895884
Pre-review self checklist
masterbranchmvn -P check-style validate)mvn verifyand inspecttarget/japicmp/japicmp.html)SNOW-XXXX:External contributors - please answer these questions before submitting a pull request. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Issue: #NNNN
Fill out the following pre-review checklist:
@SnowflakeJdbcInternalApi(note that public/protected methods/fields in classes marked with this annotation are already internal)Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.