SNOW-715510: MFA Token cache for libsnowflakeclient #773
Conversation
sfc-gh-jszczerbinski
requested review from
sfc-gh-ext-simba-hx and
sfc-gh-dprzybysz
|
|
||
| std::string credItemStr(const CredentialKey &key) | ||
| { | ||
| return key.host + ":" + key.user + ":SNOWFLAKE-ODBC-DRIVER:" + credTypeToString(key.type); |
There was a problem hiding this comment.
should it be hardcoded to ODBC-DRIVER? I wonder if there's some way to determine whether libsfclient is executed within ODBC or PHP driver or possibly even other drivers in the future.
There was a problem hiding this comment.
it should be configurable and supported not only for ODBC but also PHP
| ensureObject(cache); | ||
| picojson::object &obj = cache.get<picojson::object>(); | ||
| auto pair = obj.emplace(key.account, picojson::value(picojson::object())); | ||
| auto accountCacheIt = pair.first; |
There was a problem hiding this comment.
nit: Here I also wasn't sure why we're calling pair.first then accountCacheIt->second gets the credential value. I admit it's a skill issue but would appreciate getting verbose type instead of auto either here or for pair
There was a problem hiding this comment.
- picojson::object in pratice is an alias for std::map<std::string, picojson::value>
typedef std::map<std::string, value> object;- Doc of the emplace method https://en.cppreference.com/w/cpp/container/map/emplace
- This code would be clear with more modern c++
/* inserted is true if key.account wasn't in obj and we added new element */
auto [accountCacheIt, inserted] = obj.emplace(key.account, picojson::value(picojson::object())); 
| FileLock lock(path); | ||
| if (!lock.isLocked()) | ||
| { | ||
| CXX_LOG_ERROR("Failed to get token. Could not acquire file lock(path=%s)", lock.getPath().c_str()); |
There was a problem hiding this comment.
Should we continue if we don't have the lock? (applies to this and following two functions)
There was a problem hiding this comment.
Probably not, but I tried not to change original implementation due to BCR concerns
cpp/lib/CacheFile.cpp
Outdated
| @@ -0,0 +1,227 @@ | |||
| // | |||
There was a problem hiding this comment.
let's not add author comments but add license header for new files
There was a problem hiding this comment.
Sure. CLion generated it and I forgot to delete it.
| { | ||
| "SF_TEMPORARY_CREDENTIAL_CACHE_DIR", | ||
| "HOME", | ||
| "TMP" |
There was a problem hiding this comment.
I don't think TMP should be there
There was a problem hiding this comment.
I agree, but removing it is a BCR in ODBC.
There was a problem hiding this comment.
I will make changes after security modeling is complete
|
|
||
| std::string credItemStr(const CredentialKey &key) | ||
| { | ||
| return key.host + ":" + key.user + ":SNOWFLAKE-ODBC-DRIVER:" + credTypeToString(key.type); |
There was a problem hiding this comment.
it should be configurable and supported not only for ODBC but also PHP
| bool ensurePermissions(const std::string& path, unsigned mode) | ||
| { | ||
| CXX_LOG_ERROR("Cannot ensure permissions on current platform"); | ||
| return false; |
There was a problem hiding this comment.
is it ok that for WINDOWS it will be always false?
There was a problem hiding this comment.
File caches are not used on windows at all. This definition ensures that the code compiles on windows, but in practice it's never used.
There was a problem hiding this comment.
maybe then just make the code unnacessible from the beginning? For MacOS are we using the file cache or some secure storage?
No description provided.