diff --git a/common/remote/hooks/git/gitfile.cpp b/common/remote/hooks/git/gitfile.cpp index ccebaddb28b..1a3e7d14ebe 100644 --- a/common/remote/hooks/git/gitfile.cpp +++ b/common/remote/hooks/git/gitfile.cpp @@ -244,7 +244,7 @@ class GitRepositoryFileIO : implements CSimpleInterfaceOf addPathSepChar(scriptPath).append("bin/hpccaskpass.sh"); env.emplace_back("GIT_ASKPASS", scriptPath); - Owned secret = getSecret("git", gitUser); + Owned secret = getSecret("git", gitUser); if (secret) { MemoryBuffer gitKey; diff --git a/common/thorhelper/thorsoapcall.cpp b/common/thorhelper/thorsoapcall.cpp index 98df33c7a32..b4a0d56976e 100644 --- a/common/thorhelper/thorsoapcall.cpp +++ b/common/thorhelper/thorsoapcall.cpp @@ -878,7 +878,7 @@ class CWSCHelperThread : public Thread bool loadConnectSecret(const char *vaultId, const char *secretName, UrlArray &urlArray, StringBuffer &issuer, StringBuffer &proxyAddress, bool required, WSCType wscType) { - Owned secret; + Owned secret; if (!isEmptyString(secretName)) secret.setown(getSecret("ecl", secretName, vaultId, nullptr)); if (!secret) diff --git a/ecl/hql/hqlrepository.cpp b/ecl/hql/hqlrepository.cpp index 4a6ad7e5b75..75ca659e302 100644 --- a/ecl/hql/hqlrepository.cpp +++ b/ecl/hql/hqlrepository.cpp @@ -959,7 +959,7 @@ unsigned EclRepositoryManager::runGitCommand(StringBuffer * output, const char * } else { - Owned secret = getSecret("git", options.gitUser.str()); + Owned secret = getSecret("git", options.gitUser.str()); if (secret) { MemoryBuffer gitKey; diff --git a/esp/clients/ws_dfsclient/ws_dfsclient.cpp b/esp/clients/ws_dfsclient/ws_dfsclient.cpp index 6a9045952f0..8270b912a8b 100644 --- a/esp/clients/ws_dfsclient/ws_dfsclient.cpp +++ b/esp/clients/ws_dfsclient/ws_dfsclient.cpp @@ -599,7 +599,7 @@ IClientWsDfs *getDfsClient(const char *serviceUrl, IUserDescriptor *userDesc) static void configureClientSSL(IEspClientRpcSettings &rpc, const char *secretName) { - Owned secretPTree = getSecret("storage", secretName); + Owned secretPTree = getSecret("storage", secretName); if (!secretPTree) throw makeStringExceptionV(-1, "secret %s.%s not found", "storage", secretName); diff --git a/esp/esdlscriptlib/esdl_script.cpp b/esp/esdlscriptlib/esdl_script.cpp index 4c4c24b7f5c..6d6427387f8 100644 --- a/esp/esdlscriptlib/esdl_script.cpp +++ b/esp/esdlscriptlib/esdl_script.cpp @@ -911,7 +911,7 @@ class CEsdlTransformOperationMySqlCall : public CEsdlTransformOperationBase recordException(ESDL_SCRIPT_MissingOperationAttr, msg.append(name)); } } - IPropertyTree *getSecretInfo(IXpathContext * sourceContext) + const IPropertyTree *getSecretInfo(IXpathContext * sourceContext) { //leaving flexibility for the secret to be configured multiple ways // the most secure option in my opinion is to at least have the server, name, and password all in the secret @@ -943,7 +943,7 @@ class CEsdlTransformOperationMySqlCall : public CEsdlTransformOperationBase options.append(name).append('=').append(value); } - void appendOption(StringBuffer &options, const char *name, IXpathContext * sourceContext, ICompiledXpath *cx, IPropertyTree *secret, bool required) + void appendOption(StringBuffer &options, const char *name, IXpathContext * sourceContext, ICompiledXpath *cx, const IPropertyTree *secret, bool required) { if (secret && secret->hasProp(name)) { @@ -971,7 +971,7 @@ class CEsdlTransformOperationMySqlCall : public CEsdlTransformOperationBase } IEmbedFunctionContext *createFunctionContext(IXpathContext * sourceContext) { - Owned secret = getSecretInfo(sourceContext); + Owned secret = getSecretInfo(sourceContext); StringBuffer options; appendOption(options, "server", sourceContext, m_server, secret, true); appendOption(options, "user", sourceContext, m_user, secret, true); diff --git a/rtl/eclrtl/eclrtl.cpp b/rtl/eclrtl/eclrtl.cpp index 7fb01efd1cf..980ebf2198e 100644 --- a/rtl/eclrtl/eclrtl.cpp +++ b/rtl/eclrtl/eclrtl.cpp @@ -6279,7 +6279,7 @@ void rtlBase64Decode(size32_t & tlen, void * & tgt, size32_t slen, const char * void rtlGetEclUserSecret(size32_t & outlen, void * & out, const char *name, const char *key) { - Owned secret = getSecret("eclUser", name); + Owned secret = getSecret("eclUser", name); if (secret) { MemoryBuffer data; diff --git a/system/codesigner/gpgcodesigner.cpp b/system/codesigner/gpgcodesigner.cpp index 8ed528fd21b..3e3c124a470 100644 --- a/system/codesigner/gpgcodesigner.cpp +++ b/system/codesigner/gpgcodesigner.cpp @@ -138,7 +138,7 @@ void GpgCodeSigner::importKeysFromSecret(const char * cat, const char *keytype) for (int keyentry = 1; ; keyentry++) { VStringBuffer keysecretname("gpg-%s-key-%d", keytype, keyentry); - Owned secretKey = getSecret(cat, keysecretname.str()); + Owned secretKey = getSecret(cat, keysecretname.str()); if (secretKey) { StringBuffer gpgKey; diff --git a/system/jlib/jptree.hpp b/system/jlib/jptree.hpp index 80888c8218d..e52db5eec1a 100644 --- a/system/jlib/jptree.hpp +++ b/system/jlib/jptree.hpp @@ -438,11 +438,14 @@ extern jlib_decl unsigned getPropertyTreeHash(const IPropertyTree & source, unsi //to not be modified and to remain valid and consistent until it is released. interface ISyncedPropertyTree : extends IInterface { +//The following functions check whether something is up to date before returning their values. + //Return a version-hash which changes whenever the property tree changes - so that a caller can determine whether it needs to update + virtual unsigned getVersion() const = 0; virtual const IPropertyTree * getTree() const = 0; virtual bool getProp(MemoryBuffer & result, const char * xpath) const = 0; virtual bool getProp(StringBuffer & result, const char * xpath) const = 0; - //Return a version-hash which changes whenever the property tree changes - so that a caller can determine whether it needs to update - virtual unsigned getVersion() const = 0; + +// The following functions return the current cached state - they do not force a check to see if the value is up to date virtual bool isStale() const = 0; // An indication that the property tree may be out of date because it couldn't be resynchronized. virtual bool isValid() const = 0; // Is the property tree non-null? Typically called at startup to check configuration is provided. }; diff --git a/system/jlib/jsecrets.cpp b/system/jlib/jsecrets.cpp index 5951ad6fbb7..322c6c12f73 100644 --- a/system/jlib/jsecrets.cpp +++ b/system/jlib/jsecrets.cpp @@ -64,36 +64,19 @@ CVaultKind getSecretType(const char *s) } interface IVaultManager : extends IInterface { - virtual bool getCachedSecretFromVault(const char *category, const char *vaultId, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) = 0; virtual bool requestSecretFromVault(const char *category, const char *vaultId, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) = 0; - virtual bool getCachedSecretByCategory(const char *category, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) = 0; virtual bool requestSecretByCategory(const char *category, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) = 0; }; -static CriticalSection secretCacheCS; -static Owned secretCache; -static CriticalSection mtlsInfoCacheCS; -static std::unordered_map> mtlsInfoCache; static Owned vaultManager; static MemoryAttr udpKey; static bool udpKeyInitialized = false; -MODULE_INIT(INIT_PRIORITY_SYSTEM) +static const IPropertyTree *getLocalSecret(const char *category, const char * name) { - secretCache.setown(createPTree()); - return true; + return getSecret(category, name, "k8s", nullptr); } -MODULE_EXIT() -{ - vaultManager.clear(); - secretCache.clear(); - mtlsInfoCache.clear(); - udpKey.clear(); -} - -static IPropertyTree *getLocalSecret(const char *category, const char * name); - //based on kubernetes secret / key names. Even if some vault backends support additional characters we'll restrict to this subset for now static const char *validSecretNameChrs = ".-"; @@ -319,8 +302,8 @@ extern jlib_decl StringBuffer &generateDynamicUrlSecretName(StringBuffer &secret unsigned portNum = port.length() ? atoi(port) : 0; return generateDynamicUrlSecretName(secretName, scheme, username, host, portNum, path); } -//--------------------------------------------------------------------------------------------------------------------- +//--------------------------------------------------------------------------------------------------------------------- static StringBuffer secretDirectory; static CriticalSection secretCS; @@ -364,18 +347,6 @@ static StringBuffer &buildSecretPath(StringBuffer &path, const char *category, c return addPathSepChar(path.append(ensureSecretDirectory())).append(category).append(PATHSEPCHAR).append(name).append(PATHSEPCHAR); } -static bool checkSecretExpired(unsigned created) -{ - if (!created) - return false; - unsigned age = msTick() - created; - return age > getSecretTimeout(); -} - -static bool hasCacheExpired(const IPropertyTree * secret) -{ - return checkSecretExpired((unsigned)secret->getPropInt("@created")); -} enum class VaultAuthType {unknown, k8s, appRole, token, clientcert}; @@ -395,6 +366,134 @@ static bool isEmptyTimeval(const timeval &tv) return (tv.tv_sec==0 && tv.tv_usec==0); } +//--------------------------------------------------------------------------------------------------------------------- + +//Represents an entry in the secret cache. Once created it is always used for the secret. +using cache_timestamp = unsigned; +class SecretCacheEntry : public CInterface +{ + friend class SecretCache; + +public: + //A cache entry is initally created that has a create and access time of now, but the checkTimestamp + //is set so that needsRefresh() will return true. + SecretCacheEntry(cache_timestamp _now) + : contentTimestamp(_now), accessedTimestamp(_now), checkedTimestamp(_now - 2 * secretTimeoutMs) + { + } + + unsigned getHash() const + { + return contentHash; + } + + // We should never replace known contents for unknown contents + // so once this returns true it should always return true + bool hasContents() const + { + return contents != nullptr; + } + + //Is the secret potentially out of date? + bool isStale() const + { + cache_timestamp now = msTick(); + cache_timestamp elapsed = (now - contentTimestamp); + return (elapsed > secretTimeoutMs); + } + + // Is it time to check if there is a new value for this secret? + bool needsRefresh(cache_timestamp now) const + { + cache_timestamp elapsed = (now - checkedTimestamp); + return (elapsed > secretTimeoutMs); + } + + bool needsRefresh() const + { + return needsRefresh(msTick()); + } + + void noteFailedUpdate(cache_timestamp now) + { + //Update the checked timestamp - so that we do not continually check for updates to secrets which + //are stale because the vault or other source of values in inaccessible. + //Keep using the last good value + checkedTimestamp = now; + } + + //The following functions can only be called from member functions of SecretCache +private: + void updateContents(IPropertyTree * _contents, cache_timestamp now) + { + contents.set(_contents); + updateHash(); + contentTimestamp = now; + accessedTimestamp = now; + checkedTimestamp = now; + } + + void updateHash() + { + if (contents) + contentHash = getPropertyTreeHash(*contents, 0x811C9DC5); + else + contentHash = 0; + } +private: + Linked contents;// Can only be accessed when SecretCache::cs is held + cache_timestamp contentTimestamp = 0; // When was this secret read from disk/vault + cache_timestamp accessedTimestamp = 0; // When was this secret last accessed? + cache_timestamp checkedTimestamp = 0; // When was this last checked for updates? + unsigned contentHash = 0; +}; + +// A cache of (secret[:version] to a secret cache entry) +// Once a hash table entry has been created for a secret it is never removed and the associated +// value is never replaced. This means it is safe to keep a pointer to the entry in another class. +class SecretCache +{ +public: + const IPropertyTree * getContents(SecretCacheEntry * match) + { + //Return contents within the critical section so no other thread can modify it + CriticalBlock block(cs); + return LINK(match->contents); + } + + //Check to see if a secret exists, and if not add a null entry that has expired. + SecretCacheEntry * resolveSecret(const std::string & secretKey, cache_timestamp now) + { + SecretCacheEntry * result; + CriticalBlock block(cs); + auto match = secrets.find(secretKey); + if (match != secrets.cend()) + { + result = match->second.get(); + result->accessedTimestamp = now; + } + else + { + //Insert an entry with a null value that is marked as out of date + result = new SecretCacheEntry(now); + secrets.emplace(secretKey, result); + } + return result; + } + + void updateSecret(SecretCacheEntry * match, IPropertyTree * value, cache_timestamp now) + { + CriticalBlock block(cs); + match->updateContents(value, now); + } + +private: + CriticalSection cs; + std::unordered_map> secrets; +}; + +//--------------------------------------------------------------------------------------------------------------------- + class CVault { private: @@ -402,7 +501,6 @@ class CVault CVaultKind kind; CriticalSection vaultCS; - Owned cache; std::string clientCertPath; std::string clientKeyPath; @@ -445,7 +543,6 @@ class CVault if (!checkFileExists(clientKeyPath.c_str())) WARNLOG("vault: client key not found, %s", clientKeyPath.c_str()); - cache.setown(createPTree()); StringBuffer url; replaceEnvVariables(url, vault->queryProp("@url"), false); PROGLOG("vault url %s", url.str()); @@ -486,7 +583,7 @@ class CVault } else if (vault->hasProp("@client-secret")) { - Owned clientSecret = getLocalSecret("system", vault->queryProp("@client-secret")); + Owned clientSecret = getLocalSecret("system", vault->queryProp("@client-secret")); if (clientSecret) { StringBuffer tokenText; @@ -672,7 +769,7 @@ class CVault return; DBGLOG("appRoleLogin%s", permissionDenied ? " because existing token permission denied" : ""); StringBuffer appRoleSecretId; - Owned appRoleSecret = getLocalSecret("system", appRoleSecretName); + Owned appRoleSecret = getLocalSecret("system", appRoleSecretName); if (!appRoleSecret) vaultAuthErrorV("appRole secret %s not found", appRoleSecretName.str()); else if (!getSecretKeyValue(appRoleSecretId, appRoleSecret, "secret-id")) @@ -712,42 +809,6 @@ class CVault if (clientToken.isEmpty()) vaultAuthError("no vault access token"); } - bool getCachedSecret(CVaultKind &rkind, StringBuffer &content, const char *secret, const char *version) - { - CriticalBlock block(vaultCS); - IPropertyTree *tree = cache->queryPropTree(secret); - if (tree) - { - VStringBuffer vername("v.%s", isEmptyString(version) ? "latest" : version); - IPropertyTree *envelope = tree->queryPropTree(vername); - if (!envelope) - return false; - if (hasCacheExpired(envelope)) - { - tree->removeTree(envelope); - return false; - } - const char *s = envelope->queryProp(""); - rkind = kind; - if (!isEmptyString(s)) - content.append(s); - return true; - } - return false; - } - void addCachedSecret(const char *content, const char *secret, const char *version) - { - VStringBuffer vername("v.%s", isEmptyString(version) ? "latest" : version); - Owned envelope = createPTree(vername); - envelope->setPropInt("@created", (int) msTick()); - if (!isEmptyString(content)) - envelope->setProp("", content); - { - CriticalBlock block(vaultCS); - IPropertyTree *parent = ensurePTree(cache, secret); - parent->setPropTree(vername, envelope.getClear()); - } - } bool requestSecretAtLocation(CVaultKind &rkind, StringBuffer &content, const char *location, const char *secretCacheKey, const char *version, bool permissionDenied) { checkAuthentication(permissionDenied); @@ -779,7 +840,6 @@ class CVault { rkind = kind; content.append(res->body.c_str()); - addCachedSecret(content.str(), secretCacheKey, version); return true; } else if (res->status == 403) @@ -801,7 +861,6 @@ class CVault else OERRLOG("Error: Vault %s http error (%d) accessing secret %s.%s location %s", name.str(), res.error(), secretCacheKey, version ? version : "", location); - addCachedSecret("", secretCacheKey, version); //cache misses so we don't keep calling the vault return false; } bool requestSecret(CVaultKind &rkind, StringBuffer &content, const char *secret, const char *version) @@ -831,16 +890,6 @@ class CVaultSet if (!isEmptyString(name)) vaults.emplace(name, std::unique_ptr(new CVault(vault))); } - bool getCachedSecret(CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) - { - auto it = vaults.begin(); - for (; it != vaults.end(); it++) - { - if (it->second->getCachedSecret(kind, content, secret, version)) - return true; - } - return false; - } bool requestSecret(CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) { auto it = vaults.begin(); @@ -851,15 +900,6 @@ class CVaultSet } return false; } - bool getCachedSecretFromVault(const char *vaultId, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) - { - if (isEmptyString(vaultId)) - return false; - auto it = vaults.find(vaultId); - if (it == vaults.end()) - return false; - return it->second->getCachedSecret(kind, content, secret, version); - } bool requestSecretFromVault(const char *vaultId, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) { if (isEmptyString(vaultId)) @@ -906,15 +946,6 @@ class CVaultManager : public CInterfaceOf it->second->addVault(&vault); } } - bool getCachedSecretFromVault(const char *category, const char *vaultId, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) override - { - if (isEmptyString(category)) - return false; - auto it = categories.find(category); - if (it == categories.end()) - return false; - return it->second->getCachedSecretFromVault(vaultId, kind, content, secret, version); - } bool requestSecretFromVault(const char *category, const char *vaultId, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) override { if (isEmptyString(category)) @@ -925,15 +956,6 @@ class CVaultManager : public CInterfaceOf return it->second->requestSecretFromVault(vaultId, kind, content, secret, version); } - bool getCachedSecretByCategory(const char *category, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) override - { - if (isEmptyString(category)) - return false; - auto it = categories.find(category); - if (it == categories.end()) - return false; - return it->second->getCachedSecret(kind, content, secret, version); - } bool requestSecretByCategory(const char *category, CVaultKind &kind, StringBuffer &content, const char *secret, const char *version) override { if (isEmptyString(category)) @@ -953,56 +975,34 @@ IVaultManager *ensureVaultManager() return vaultManager; } -static IPropertyTree *getCachedLocalSecret(const char *category, const char *name, bool &cachedMiss) +//--------------------------------------------------------------------------------------------------------------------- + +static SecretCache globalSecretCache; +static CriticalSection mtlsInfoCacheCS; +static std::unordered_map> mtlsInfoCache; + +MODULE_INIT(INIT_PRIORITY_SYSTEM) { - if (isEmptyString(name)) - return nullptr; - Owned secret; - { - CriticalBlock block(secretCacheCS); - IPropertyTree *tree = secretCache->queryPropTree(category); - if (!tree) - return nullptr; - secret.setown(tree->getPropTree(name)); - if (secret) - { - if (hasCacheExpired(secret)) - { - secretCache->removeProp(name); - return nullptr; - } - if (secret->hasProp("@miss")) - { - cachedMiss = true; - return nullptr; - } - return secret.getClear(); - } - } - return nullptr; + return true; } -static void addCachedLocalSecret(const char *category, const char *name, IPropertyTree *secret) +MODULE_EXIT() { - if (!secret || isEmptyString(name) || isEmptyString(category)) - return; - secret->setPropInt("@created", (int)msTick()); - { - CriticalBlock block(secretCacheCS); - IPropertyTree *tree = ensurePTree(secretCache, category); - tree->setPropTree(name, LINK(secret)); - } + vaultManager.clear(); + udpKey.clear(); } -static IPropertyTree *loadLocalSecret(const char *category, const char * name) + +static IPropertyTree * resolveLocalSecret(const char *category, const char * name) { StringBuffer path; buildSecretPath(path, category, name); + Owned entries = createDirectoryIterator(path); if (!entries || !entries->first()) return nullptr; - Owned tree = createPTree(name); - tree->setPropInt("@created", (int) msTick()); + + Owned tree(createPTree(name)); ForEach(*entries) { if (entries->isDir()) @@ -1018,22 +1018,8 @@ static IPropertyTree *loadLocalSecret(const char *category, const char * name) continue; tree->setPropBin(name, content.length(), content.bufferBase()); } - addCachedLocalSecret(category, name, tree); - return tree.getClear(); -} - -static IPropertyTree *getLocalSecret(const char *category, const char * name) -{ - validateCategoryName(category); - validateSecretName(name); - bool skipLocalFetch = false; - Owned tree = getCachedLocalSecret(category, name, skipLocalFetch); - if (skipLocalFetch) - return nullptr; - if (tree) - return tree.getClear(); - return loadLocalSecret(category, name); + return tree.getClear(); } static IPropertyTree *createPTreeFromVaultSecret(const char *content, CVaultKind kind) @@ -1056,30 +1042,8 @@ static IPropertyTree *createPTreeFromVaultSecret(const char *content, CVaultKind } return tree.getClear(); } -static IPropertyTree *getCachedVaultSecret(const char *category, const char *vaultId, const char * name, const char *version, bool &cachedMiss) -{ - CVaultKind kind; - StringBuffer json; - IVaultManager *vaultmgr = ensureVaultManager(); - if (isEmptyString(vaultId)) - { - if (!vaultmgr->getCachedSecretByCategory(category, kind, json, name, version)) - return nullptr; - } - else - { - if (!vaultmgr->getCachedSecretFromVault(category, vaultId, kind, json, name, version)) - return nullptr; - } - if (json.isEmpty()) - { - cachedMiss = true; - return nullptr; - } - return createPTreeFromVaultSecret(json.str(), kind); -} -static IPropertyTree *requestVaultSecret(const char *category, const char *vaultId, const char * name, const char *version) +static IPropertyTree *resolveVaultSecret(const char *category, const char * name, const char *vaultId, const char *version) { CVaultKind kind; StringBuffer json; @@ -1097,55 +1061,58 @@ static IPropertyTree *requestVaultSecret(const char *category, const char *vault return createPTreeFromVaultSecret(json.str(), kind); } -static IPropertyTree *getVaultSecret(const char *category, const char * name, const char *vaultId, const char *version) + +static SecretCacheEntry * getSecretEntry(const char *category, const char * name, const char * optVaultId, const char * optVersion) { - CVaultKind kind; - StringBuffer json; - IVaultManager *vaultmgr = ensureVaultManager(); + cache_timestamp now = msTick(); - bool cachedMiss = false; + std::string key; + key.append(category).append("/").append(name); + if (optVaultId) + key.append("@").append(optVaultId); + if (optVersion) + key.append("#").append(optVersion); - if (isEmptyString(vaultId)) + SecretCacheEntry * match = globalSecretCache.resolveSecret(key, now); + if (!match->needsRefresh(now)) + return match; + + Owned resolved; + if (!isEmptyString(optVaultId)) { - if (vaultmgr->getCachedSecretByCategory(category, kind, json, name, version)) - cachedMiss = json.isEmpty(); + if (strieq(optVaultId, "k8s")) + resolved.setown(resolveLocalSecret(category, name)); else - vaultmgr->requestSecretByCategory(category, kind, json, name, version); + resolved.setown(resolveVaultSecret(category, name, optVaultId, optVersion)); } else { - if (!vaultmgr->getCachedSecretFromVault(category, vaultId, kind, json, name, version)) - cachedMiss = json.isEmpty(); - else - vaultmgr->requestSecretFromVault(category, vaultId, kind, json, name, version); + resolved.setown(resolveLocalSecret(category, name)); + if (!resolved) + resolved.setown(resolveVaultSecret(category, name, nullptr, optVersion)); } - if (cachedMiss) - return nullptr; - return createPTreeFromVaultSecret(json.str(), kind); + + //If the secret could no longer be resolved (e.g. a vault has gone down) then keep the old one + if (resolved) + globalSecretCache.updateSecret(match, resolved, now); + else + match->noteFailedUpdate(now); + + return match; } -IPropertyTree *getSecretTree(const char *category, const char * name, const char * optVaultId, const char * optVersion) +static const IPropertyTree *getSecretTree(const char *category, const char * name, const char * optVaultId, const char * optVersion) { - if (!isEmptyString(optVaultId)) - return getVaultSecret(category, name, optVaultId, optVersion); + SecretCacheEntry * secret = getSecretEntry(category, name, optVaultId, optVersion); + if (secret) + return globalSecretCache.getContents(secret); + return nullptr; +} - //if we get back a null secret, it might be a cached miss, so don't go to the source if flag gets set - bool skipVaultFetch = false; - bool skipLocalFetch = false; - //check for any chached first - Owned secret = getCachedLocalSecret(category, name, skipLocalFetch); - if (!secret) - secret.setown(getCachedVaultSecret(category, nullptr, name, nullptr, skipVaultFetch)); - //now check local, then vaults - if (!secret && !skipLocalFetch) - secret.setown(loadLocalSecret(category, name)); - if (!secret && !skipVaultFetch) - secret.setown(requestVaultSecret(category, nullptr, name, nullptr)); - return secret.getClear(); -} +//Public interface to the secrets -IPropertyTree *getSecret(const char *category, const char * name, const char * optVaultId, const char * optVersion) +const IPropertyTree *getSecret(const char *category, const char * name, const char * optVaultId, const char * optVersion) { validateCategoryName(category); validateSecretName(name); @@ -1172,7 +1139,7 @@ bool getSecretKeyValue(StringBuffer & result, const IPropertyTree *secret, const extern jlib_decl bool getSecretValue(StringBuffer & result, const char *category, const char * name, const char * key, bool required) { - Owned secret = getSecret(category, name); + Owned secret = getSecret(category, name); if (required && !secret) throw MakeStringException(-1, "secret %s.%s not found", category, name); bool found = getSecretKeyValue(result, secret, key); @@ -1186,10 +1153,9 @@ extern jlib_decl bool getSecretValue(StringBuffer & result, const char *category class CSecret final : public CInterfaceOf { public: - CSecret(const char *_category, const char * _name, const char * _vaultId, const char * _version, const IPropertyTree * _secret) + CSecret(const char *_category, const char * _name, const char * _vaultId, const char * _version, SecretCacheEntry * _secret) : category(_category), name(_name), vaultId(_vaultId), version(_version), secret(_secret) { - updateHash(); } virtual const IPropertyTree * getTree() const override; @@ -1197,34 +1163,34 @@ class CSecret final : public CInterfaceOf virtual bool getProp(MemoryBuffer & result, const char * key) const override { CriticalBlock block(secretCs); - checkStale(); - return getSecretKeyValue(result, secret, key); + checkUptoDate(); + Owned contents = globalSecretCache.getContents(secret); + return getSecretKeyValue(result, contents, key); } virtual bool getProp(StringBuffer & result, const char * key) const override { CriticalBlock block(secretCs); - checkStale(); - return getSecretKeyValue(result, secret, key); + checkUptoDate(); + Owned contents = globalSecretCache.getContents(secret); + return getSecretKeyValue(result, contents, key); } virtual bool isStale() const override { - return secret && hasCacheExpired(secret); + return secret->isStale(); } virtual unsigned getVersion() const override { CriticalBlock block(secretCs); - checkStale(); - return secretHash; + checkUptoDate(); + return secret->getHash(); } virtual bool isValid() const override { - CriticalBlock block(secretCs); - return secret != nullptr; + return secret->hasContents(); } protected: - void checkStale() const; - void updateHash() const; + void checkUptoDate() const; protected: StringAttr category; @@ -1232,21 +1198,20 @@ class CSecret final : public CInterfaceOf StringAttr vaultId; StringAttr version; mutable CriticalSection secretCs; - mutable Linked secret; - mutable unsigned secretHash = 0; + mutable SecretCacheEntry * secret; }; const IPropertyTree * CSecret::getTree() const { CriticalBlock block(secretCs); - checkStale(); - return LINK(secret); + checkUptoDate(); + return globalSecretCache.getContents(secret); } -void CSecret::checkStale() const +void CSecret::checkUptoDate() const { - if (isStale()) + if (secret->needsRefresh()) { #ifdef TRACE_SECRETS DBGLOG("Secret %s/%s is stale updating from %u...", category.str(), name.str(), secretHash); @@ -1254,8 +1219,10 @@ void CSecret::checkStale() const //MORE: This could block or fail - in roxie especially it would be better to return the old value try { - secret.setown(getSecretTree(category, name, vaultId, version)); - updateHash(); + SecretCacheEntry * newSecret = getSecretEntry(category, name, vaultId, version); + //Check the secret is always returned consistently. It would be possible to call a slightly + //more optimal function to refresh a secret, but this is simplest. + assertex(secret == newSecret); } catch (IException * e) { @@ -1266,17 +1233,12 @@ void CSecret::checkStale() const } } -void CSecret::updateHash() const -{ - if (secret) - secretHash = getPropertyTreeHash(*secret.get(), 0x811C9DC5); - else - secretHash = 0; -} - ISyncedPropertyTree * resolveSecret(const char *category, const char * name, const char * optVaultId, const char * optVersion) { - Owned resolved = getSecret(category, name, optVaultId, optVersion); + validateCategoryName(category); + validateSecretName(name); + + SecretCacheEntry * resolved = getSecretEntry(category, name, optVaultId, optVersion); return new CSecret(category, name, optVaultId, optVersion, resolved); } @@ -1354,13 +1316,13 @@ class CSyncedCertificateBase : public CInterfaceOf virtual bool getProp(MemoryBuffer & result, const char * key) const override final { CriticalBlock block(secretCs); - checkStale(); + checkUptoDate(); return getSecretKeyValue(result, config, key); } virtual bool getProp(StringBuffer & result, const char * key) const override final { CriticalBlock block(secretCs); - checkStale(); + checkUptoDate(); return getSecretKeyValue(result, config, key); } virtual bool isStale() const override final @@ -1375,7 +1337,7 @@ class CSyncedCertificateBase : public CInterfaceOf virtual unsigned getVersion() const override final { CriticalBlock block(secretCs); - checkStale(); + checkUptoDate(); //If information that is combined with the secret (e.g. trusted peers) can also change dynamically this would //need to be a separate hash calculated from the config tree return secretHash; @@ -1385,7 +1347,7 @@ class CSyncedCertificateBase : public CInterfaceOf virtual void updateConfigFromSecret(const IPropertyTree * secretInfo) const = 0; protected: - void checkStale() const; + void checkUptoDate() const; void createConfig() const; void createDefaultConfigFromSecret(const IPropertyTree * secretInfo, bool addCertificates, bool addCertificateAuthority) const; void updateCertificateFromSecret(const IPropertyTree * secretInfo) const; @@ -1403,11 +1365,11 @@ class CSyncedCertificateBase : public CInterfaceOf const IPropertyTree * CSyncedCertificateBase::getTree() const { CriticalBlock block(secretCs); - checkStale(); + checkUptoDate(); return LINK(config); } -void CSyncedCertificateBase::checkStale() const +void CSyncedCertificateBase::checkUptoDate() const { if (secretHash != secret->getVersion()) createConfig(); diff --git a/system/jlib/jsecrets.hpp b/system/jlib/jsecrets.hpp index 9601deb4d7d..2d7f1c286be 100644 --- a/system/jlib/jsecrets.hpp +++ b/system/jlib/jsecrets.hpp @@ -28,9 +28,9 @@ extern jlib_decl void setSecretMount(const char * path); extern jlib_decl void setSecretTimeout(unsigned timeoutMs); //Return the current (cached) value of a secret. If the secret is not defined, return nullptr. -extern jlib_decl IPropertyTree *getSecret(const char *category, const char * name, const char * optVaultId = nullptr, const char * optVersion = nullptr); +extern jlib_decl const IPropertyTree *getSecret(const char *category, const char * name, const char * optVaultId = nullptr, const char * optVersion = nullptr); // resolveSecret() always returns an object, which will potentially be updated behind the scenes. If no secret is originally -// defined, but it then configured in a vault or Kubernetes secret, it will be bicked up when the cache entry is +// defined, but it then added to a vault or Kubernetes secret, it will then be picked up when the cache entry is // refreshed - allowing missing configuration to be updated for a live system. extern jlib_decl ISyncedPropertyTree * resolveSecret(const char *category, const char * name, const char * optRequiredVault, const char* optVersion); diff --git a/system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp b/system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp index fad1077b23d..cd02bcceb9a 100644 --- a/system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp +++ b/system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp @@ -286,7 +286,7 @@ AzureLogAnalyticsCurlClient::AzureLogAnalyticsCurlClient(IPropertyTree & logAcce { PROGLOG("%s: Resolving all required configuration values...", COMPONENT_NAME); - Owned secretTree = getSecret(azureLogAccessSecretCategory, azureLogAccessSecretName); + Owned secretTree = getSecret(azureLogAccessSecretCategory, azureLogAccessSecretName); if (!secretTree) throw makeStringExceptionV(-1, "%s: Could not fetch %s information!", COMPONENT_NAME, azureLogAccessSecretName); diff --git a/system/security/LdapSecurity/ldapconnection.cpp b/system/security/LdapSecurity/ldapconnection.cpp index 3b52cf7ea4e..3c55315f9a6 100644 --- a/system/security/LdapSecurity/ldapconnection.cpp +++ b/system/security/LdapSecurity/ldapconnection.cpp @@ -409,7 +409,7 @@ class CLdapConfig : implements ILdapConfig, public CInterface DBGLOG("Retrieving LDAP Admin username/password from secrets repo: %s %s", !vaultId.isEmpty() ? vaultId.str() : "", adminUserSecretKey.str()); - Owned secretTree(getSecret("authn", adminUserSecretKey.str(), vaultId, nullptr)); + Owned secretTree(getSecret("authn", adminUserSecretKey.str(), vaultId, nullptr)); if (!secretTree) throw MakeStringException(-1, "Error retrieving LDAP Admin username/password"); @@ -494,7 +494,7 @@ class CLdapConfig : implements ILdapConfig, public CInterface cfg->getProp(".//@hpccAdminVaultId", vaultId);//optional HashiCorp vault ID DBGLOG("Retrieving optional HPCC Admin username/password from secrets repo: %s %s", !vaultId.isEmpty() ? vaultId.str() : "", adminUserSecretKey.str()); - Owned secretTree(getSecret("authn", adminUserSecretKey.str(), vaultId, nullptr)); + Owned secretTree(getSecret("authn", adminUserSecretKey.str(), vaultId, nullptr)); if (secretTree) { getSecretKeyValue(m_HPCCAdminUser_username, secretTree, "username"); diff --git a/system/security/plugins/testauthSecurity/testauthSecurity.cpp b/system/security/plugins/testauthSecurity/testauthSecurity.cpp index 7da09257991..feb800cbf8f 100644 --- a/system/security/plugins/testauthSecurity/testauthSecurity.cpp +++ b/system/security/plugins/testauthSecurity/testauthSecurity.cpp @@ -230,7 +230,7 @@ class CTestAuthSecurityManager : public CBaseSecurityManager const char* secretKey = userSettings.queryProp("@secretKey"); if (!isEmptyString(secretKey)) { - Owned secretTree = getSecret("authn", secretKey); + Owned secretTree = getSecret("authn", secretKey); if (!secretTree) throw makeStringExceptionV(-1, "Error retrieving the secret for %s.", secretKey); diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp index 6a074989d43..b925505cba7 100644 --- a/testing/unittests/jlibtests.cpp +++ b/testing/unittests/jlibtests.cpp @@ -32,11 +32,14 @@ #include "jlzw.hpp" #include "jqueue.hpp" #include "jregexp.hpp" +#include "jsecrets.hpp" #include "jutil.hpp" #include "junicode.hpp" #include "unittests.hpp" +#define CPPUNIT_ASSERT_EQUAL_STR(x, y) CPPUNIT_ASSERT_EQUAL(std::string(x),std::string(y)) + static const unsigned oneMinute = 60000; // msec class JlibTraceTest : public CppUnit::TestFixture @@ -3611,4 +3614,204 @@ CPPUNIT_TEST_SUITE_REGISTRATION( JLibUnicodeTest ); CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( JLibUnicodeTest, "JLibUnicodeTest" ); +class JLibSecretsTest : public CppUnit::TestFixture +{ +public: + CPPUNIT_TEST_SUITE(JLibSecretsTest); + CPPUNIT_TEST(setup); + CPPUNIT_TEST(testUpdate1); + CPPUNIT_TEST(testUpdate2); + CPPUNIT_TEST_SUITE_END(); + + //Each test creates a different instance of the class(!) so member values cannot be used to pass items + //from one test to another + StringBuffer secretRoot; + +protected: + void checkSecret(const char * secret, const char * key, const char * expectedValue) + { + Owned match = getSecret("testing", secret); + CPPUNIT_ASSERT(match); + const char * secretValue = match->queryProp(key); + if (secretValue) + { + CPPUNIT_ASSERT_EQUAL_STR(secretValue, expectedValue); + } + else + { + //IPropertyTree doesn't allow blank values, so a missing value is the same as a blank value + //We should probably revisit some day, but it is likely to break existing code if we do. + CPPUNIT_ASSERT_EQUAL_STR("", expectedValue); + } + } + + bool hasSecret(const char * name) + { + Owned match = getSecret("testing", name); + return match != nullptr; + } + + void initPath() + { + char cwd[1024]; + CPPUNIT_ASSERT(GetCurrentDirectory(1024, cwd)); + secretRoot.set(cwd).append(PATHSEPCHAR).append("unittest-secrets"); + secretRoot.append(PATHSEPCHAR).append("testing"); // catgegory + } + + void setup() + { + char cwd[1024]; + CPPUNIT_ASSERT(GetCurrentDirectory(1024, cwd)); + secretRoot.append(cwd).append(PATHSEPCHAR).append("unittest-secrets"); + + recursiveRemoveDirectory(secretRoot); + CPPUNIT_ASSERT(recursiveCreateDirectory(secretRoot.str())); + setSecretMount(secretRoot); + setSecretTimeout(100); // Set the timeout so we can check it is working. + + secretRoot.append(PATHSEPCHAR).append("testing"); // catgegory + CPPUNIT_ASSERT(recursiveCreateDirectory(secretRoot.str())); + } + + void testUpdate1() + { + initPath(); // secretRoot needs to be called for each test + + CPPUNIT_ASSERT(!hasSecret("secret1")); + writeTestingSecret("secret1", "value", "secret1Value"); + //Secret should not appear yet - null should be cached. + CPPUNIT_ASSERT(!hasSecret("secret1")); + + Owned secret2 = resolveSecret("testing", "secret2", nullptr, nullptr); + CPPUNIT_ASSERT(!secret2->isValid()); + CPPUNIT_ASSERT(!secret2->isStale()); + + MilliSleep(50); + //Secret should not appear yet - null should be cached. + CPPUNIT_ASSERT(!hasSecret("secret1")); + CPPUNIT_ASSERT(!secret2->isValid()); + CPPUNIT_ASSERT(!secret2->isStale()); + + MilliSleep(100); + //Secret1 should now be updated - enough time has passed + checkSecret("secret1", "value", "secret1Value"); + CPPUNIT_ASSERT(!secret2->isValid()); + CPPUNIT_ASSERT(secret2->isStale()); + + //Cleanup + writeTestingSecret("secret1", "value", nullptr); + } + + void testUpdate2() + { + initPath(); // secretRoot needs to be called for each test + + Owned secret3 = resolveSecret("testing", "secret3", nullptr, nullptr); + unsigned version = secret3->getVersion(); + CPPUNIT_ASSERT(!secret3->isValid()); + CPPUNIT_ASSERT(!secret3->isStale()); + writeTestingSecret("secret3", "value", "secret3Value"); + CPPUNIT_ASSERT(!secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version, secret3->getVersion()); + + //After sleep new value should not have been picked up + MilliSleep(50); + CPPUNIT_ASSERT(!secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version, secret3->getVersion()); + + //After sleep new value should now have been picked up + MilliSleep(100); + checkSecret("secret3", "value", "secret3Value"); + unsigned version2 = secret3->getVersion(); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT(version != version2); + + //Sleep and check that the hash value has not changed + MilliSleep(200); + checkSecret("secret3", "value", "secret3Value"); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + + //Remove the secret - should have no immediate effect + writeTestingSecret("secret3", "value", nullptr); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + + MilliSleep(50); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + MilliSleep(100); + CPPUNIT_ASSERT(secret3->isStale()); // Value has gone, but the old value is still returned + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + //Update the value = the change should not be seen until the cache entry expires + writeTestingSecret("secret3", "value", "secret3NewValue"); + CPPUNIT_ASSERT(secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + MilliSleep(50); + CPPUNIT_ASSERT(secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + MilliSleep(100); + //These functions do not check for up to date values, so they return the same as before + CPPUNIT_ASSERT(secret3->isStale()); // Value still appears to be out of date + CPPUNIT_ASSERT(secret3->isValid()); + + //The getVersion() should force the value to be updated + unsigned version3 = secret3->getVersion(); + CPPUNIT_ASSERT(version2 != version3); + CPPUNIT_ASSERT(!secret3->isStale()); // New value has now been picked up + CPPUNIT_ASSERT(secret3->isValid()); + checkSecret("secret3", "value", "secret3NewValue"); + + //Finally check that writing a blank value is spotted as a change. + writeTestingSecret("secret3", "value", ""); + MilliSleep(150); + //Check the version to ensure that the value has been updated + CPPUNIT_ASSERT(version3 != secret3->getVersion()); + CPPUNIT_ASSERT(secret3->isValid()); + checkSecret("secret3", "value", ""); + + //Cleanup + writeTestingSecret("secret3", "value", nullptr); + } + + void writeTestingSecret(const char * secret, const char * key, const char * value) + { + StringBuffer filename; + filename.append(secretRoot).append(PATHSEPCHAR).append(secret); + CPPUNIT_ASSERT(recursiveCreateDirectory(filename.str())); + + filename.append(PATHSEPCHAR).append(key); + + Owned file = createIFile(filename.str()); + if (value) + { + Owned io = file->open(IFOcreate); + io->write(0, strlen(value), value); + } + else + file->remove(); + } +}; + +CPPUNIT_TEST_SUITE_REGISTRATION( JLibSecretsTest ); +CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( JLibSecretsTest, "JLibSecretsTest" ); + + + #endif // _USE_CPPUNIT