From e2aadb3332344e8786688d62880a3b05d63ea44a Mon Sep 17 00:00:00 2001 From: Gavin Halliday Date: Mon, 18 Sep 2023 10:53:15 +0100 Subject: [PATCH] HPCC-30304 Introduce ISecret and simplify the secret calling code Signed-off-by: Gavin Halliday --- common/thorhelper/thorsoapcall.cpp | 2 +- esp/esdlscriptlib/esdl_script.cpp | 4 +- system/jlib/jsecrets.cpp | 171 ++++++++++++++++-- system/jlib/jsecrets.hpp | 20 +- .../security/LdapSecurity/ldapconnection.cpp | 13 +- 5 files changed, 176 insertions(+), 34 deletions(-) diff --git a/common/thorhelper/thorsoapcall.cpp b/common/thorhelper/thorsoapcall.cpp index 085f0e18d91..353b1ab9691 100644 --- a/common/thorhelper/thorsoapcall.cpp +++ b/common/thorhelper/thorsoapcall.cpp @@ -1040,7 +1040,7 @@ class CWSCHelper : implements IWSCHelper, public CInterface } StringBuffer secretName("http-connect-"); secretName.append(finger); - Owned secret = (vaultId.isEmpty()) ? getSecret("ecl", secretName) : getVaultSecret("ecl", vaultId, secretName, nullptr); + Owned secret = getSecret("ecl", secretName, vaultId, nullptr); if (!secret) throw MakeStringException(0, "%sCALL %s SECRET not found", wscType == STsoap ? "SOAP" : "HTTP", secretName.str()); diff --git a/esp/esdlscriptlib/esdl_script.cpp b/esp/esdlscriptlib/esdl_script.cpp index b1a3db77ac8..979982bcfba 100644 --- a/esp/esdlscriptlib/esdl_script.cpp +++ b/esp/esdlscriptlib/esdl_script.cpp @@ -929,9 +929,7 @@ class CEsdlTransformOperationMySqlCall : public CEsdlTransformOperationBase StringBuffer vault; if (m_vaultName) sourceContext->evaluateAsString(m_vaultName, vault); - if (vault.isEmpty()) - return getSecret("esp", name); - return getVaultSecret("esp", vault, name); + return getSecret("esp", name, vault); } void appendOption(StringBuffer &options, const char *name, const char *value, bool required) { diff --git a/system/jlib/jsecrets.cpp b/system/jlib/jsecrets.cpp index 0bd6f034426..4441d3bad38 100644 --- a/system/jlib/jsecrets.cpp +++ b/system/jlib/jsecrets.cpp @@ -92,6 +92,8 @@ MODULE_EXIT() 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 = ".-"; @@ -272,7 +274,7 @@ extern jlib_decl void setSecretMount(const char * path) secretDirectory.set(path); } -static inline bool checkSecretExpired(unsigned created) +static bool checkSecretExpired(unsigned created) { if (!created) return false; @@ -280,6 +282,11 @@ static inline bool checkSecretExpired(unsigned created) return age > getSecretTimeout(); } +static bool hasCacheExpired(const IPropertyTree * secret) +{ + return checkSecretExpired((unsigned)secret->getPropInt("@created")); +} + enum class VaultAuthType {unknown, k8s, appRole, token}; static void setTimevalMS(timeval &tv, time_t ms) @@ -570,7 +577,7 @@ class CVault IPropertyTree *envelope = tree->queryPropTree(vername); if (!envelope) return false; - if (checkSecretExpired((unsigned) envelope->getPropInt("@created"))) + if (hasCacheExpired(envelope)) { tree->removeTree(envelope); return false; @@ -813,7 +820,7 @@ static IPropertyTree *getCachedLocalSecret(const char *category, const char *nam secret.setown(tree->getPropTree(name)); if (secret) { - if (checkSecretExpired((unsigned) secret->getPropInt("@created"))) + if (hasCacheExpired(secret)) { secretCache->removeProp(name); return nullptr; @@ -877,7 +884,7 @@ static IPropertyTree *loadLocalSecret(const char *category, const char * name) return tree.getClear(); } -extern jlib_decl IPropertyTree *getLocalSecret(const char *category, const char * name) +static IPropertyTree *getLocalSecret(const char *category, const char * name) { validateCategoryName(category); validateSecretName(name); @@ -944,11 +951,8 @@ static IPropertyTree *requestVaultSecret(const char *category, const char *vault return createPTreeFromVaultSecret(json.str(), kind); } -extern jlib_decl IPropertyTree *getVaultSecret(const char *category, const char *vaultId, const char * name, const char *version) +static IPropertyTree *getVaultSecret(const char *category, const char * name, const char *vaultId, const char *version) { - validateCategoryName(category); - validateSecretName(name); - CVaultKind kind; StringBuffer json; IVaultManager *vaultmgr = ensureVaultManager(); @@ -965,10 +969,10 @@ extern jlib_decl IPropertyTree *getVaultSecret(const char *category, const char return createPTreeFromVaultSecret(json.str(), kind); } -extern jlib_decl IPropertyTree *getSecret(const char *category, const char * name) +IPropertyTree *getSecretTree(const char *category, const char * name, const char * optVaultId, const char * optVersion) { - validateCategoryName(category); - validateSecretName(name); + if (!isEmptyString(optVaultId)) + return getVaultSecret(category, name, optVaultId, optVersion); //check for any chached first Owned secret = getCachedLocalSecret(category, name); @@ -982,20 +986,35 @@ extern jlib_decl IPropertyTree *getSecret(const char *category, const char * nam return secret.getClear(); } -extern jlib_decl bool getSecretKeyValue(MemoryBuffer & result, IPropertyTree *secret, const char * key) +IPropertyTree *getSecret(const char *category, const char * name, const char * optVaultId, const char * optVersion) +{ + validateCategoryName(category); + validateSecretName(name); + + return getSecretTree(category, name, optVaultId, optVersion); +} + + +bool getSecretKeyValue(MemoryBuffer & result, const IPropertyTree *secret, const char * key) { validateKeyName(key); + if (!secret) + return false; + IPropertyTree *tree = secret->queryPropTree(key); if (tree) return tree->getPropBin(nullptr, result); return false; } -extern jlib_decl bool getSecretKeyValue(StringBuffer & result, IPropertyTree *secret, const char * key) +bool getSecretKeyValue(StringBuffer & result, const IPropertyTree *secret, const char * key) { validateKeyName(key); + if (!secret) + return false; + IPropertyTree *tree = secret->queryPropTree(key); if (!tree) return false; @@ -1018,8 +1037,6 @@ extern jlib_decl bool getSecretKeyValue(StringBuffer & result, IPropertyTree *se extern jlib_decl bool getSecretValue(StringBuffer & result, const char *category, const char * name, const char * key, bool required) { - validateKeyName(key); //name and category validated in getSecret - Owned secret = getSecret(category, name); if (required && !secret) throw MakeStringException(-1, "secret %s.%s not found", category, name); @@ -1029,6 +1046,130 @@ extern jlib_decl bool getSecretValue(StringBuffer & result, const char *category return true; } +//--------------------------------------------------------------------------------------------------------------------- + +class CSecret final : public CInterfaceOf +{ +public: + CSecret(const char *_category, const char * _name, const char * _vaultId, const char * _version, const IPropertyTree * _secret) + : category(_category), name(_name), vaultId(_vaultId), version(_version), secret(_secret) + { + updateHash(); + } + + virtual const IPropertyTree * getTree() const; + + virtual bool getKeyValue(MemoryBuffer & result, const char * key) const + { + CriticalBlock block(secretCs); + checkStale(); + return getSecretKeyValue(result, secret, key); + } + virtual bool getKeyValue(StringBuffer & result, const char * key) const + { + CriticalBlock block(secretCs); + checkStale(); + return getSecretKeyValue(result, secret, key); + } + virtual bool isStale() const + { + return secret && hasCacheExpired(secret); + } + virtual unsigned getVersion() const + { + return secretHash; + } + +protected: + void checkStale() const; + void updateHash() const; + +protected: + StringAttr category; + StringAttr name; + StringAttr vaultId; + StringAttr version; + mutable CriticalSection secretCs; + mutable Linked secret; + mutable unsigned secretHash = 0; +}; + + +const IPropertyTree * CSecret::getTree() const +{ + CriticalBlock block(secretCs); + checkStale(); + return LINK(secret); +} + +void CSecret::checkStale() const +{ + if (isStale()) + { + //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(); + } + catch (IException * e) + { + VStringBuffer msg("Failed to update secret %s.%s", category.str(), name.str()); + EXCLOG(e, msg.str()); + e->Release(); + } + } +} + +//This should probably move to jptree.?pp as a generally useful function +static unsigned calculateTreeHash(const IPropertyTree & source, unsigned hashcode) +{ + if (source.isBinary()) + { + MemoryBuffer mb; + source.getPropBin(nullptr, mb); + hashcode = hashc((const byte *)mb.bufferBase(), mb.length(), hashcode); + } + else + { + const char * value = source.queryProp(nullptr); + if (value) + hashcode = hashcz((const byte *)value, hashcode); + } + + Owned aiter = source.getAttributes(); + ForEach(*aiter) + { + hashcode = hashcz((const byte *)aiter->queryName(), hashcode); + hashcode = hashcz((const byte *)aiter->queryValue(), hashcode); + } + + Owned iter = source.getElements("*"); + ForEach(*iter) + { + IPropertyTree & child = iter->query(); + hashcode = hashcz((const byte *)child.queryName(), hashcode); + hashcode = calculateTreeHash(child, hashcode); + } + return hashcode; +} + +void CSecret::updateHash() const +{ + if (secret) + secretHash = calculateTreeHash(*secret.get(), 0x811C9DC5); + else + secretHash = 0; +} + +ISecret * resolveSecret(const char *category, const char * name, const char * optVaultId, const char * optVersion) +{ + Owned resolved = getSecret(category, name, optVaultId, optVersion); + return new CSecret(category, name, optVaultId, optVersion, resolved); +} + +//--------------------------------------------------------------------------------------------------------------------- + void initSecretUdpKey() { if (udpKeyInitialized) diff --git a/system/jlib/jsecrets.hpp b/system/jlib/jsecrets.hpp index 652c3a09ca2..a69d58fc688 100644 --- a/system/jlib/jsecrets.hpp +++ b/system/jlib/jsecrets.hpp @@ -22,15 +22,25 @@ #include "jlib.hpp" #include "jstring.hpp" +interface ISecret : extends IInterface +{ + virtual const IPropertyTree * getTree() const = 0; + virtual bool getKeyValue(MemoryBuffer & result, const char * key) const = 0; + virtual bool getKeyValue(StringBuffer & result, const char * key) const = 0; + virtual bool isStale() const = 0; + //Return a sequence number which changes whenever the secret actually changes - so that a caller can determine + //whether it needs to reload the certificates. + virtual unsigned getVersion() const = 0; +}; + extern jlib_decl void setSecretMount(const char * path); extern jlib_decl void setSecretTimeout(unsigned timeoutMs); -extern jlib_decl IPropertyTree *getLocalSecret(const char *category, const char * name); -extern jlib_decl IPropertyTree *getVaultSecret(const char *category, const char *vaultId, const char * name, const char *version=nullptr); -extern jlib_decl IPropertyTree *getSecret(const char *category, const char * name); +extern jlib_decl IPropertyTree *getSecret(const char *category, const char * name, const char * optVaultId = nullptr, const char * optVersion = nullptr); +extern jlib_decl ISecret * resolveSecret(const char *category, const char * name, const char * optRequiredVault); -extern jlib_decl bool getSecretKeyValue(MemoryBuffer & result, IPropertyTree *secret, const char * key); -extern jlib_decl bool getSecretKeyValue(StringBuffer & result, IPropertyTree *secret, const char * key); +extern jlib_decl bool getSecretKeyValue(MemoryBuffer & result, const IPropertyTree *secret, const char * key); +extern jlib_decl bool getSecretKeyValue(StringBuffer & result, const IPropertyTree *secret, const char * key); extern jlib_decl bool getSecretValue(StringBuffer & result, const char *category, const char * name, const char * key, bool required); extern jlib_decl void initSecretUdpKey(); diff --git a/system/security/LdapSecurity/ldapconnection.cpp b/system/security/LdapSecurity/ldapconnection.cpp index 6533dc6451b..3b52cf7ea4e 100644 --- a/system/security/LdapSecurity/ldapconnection.cpp +++ b/system/security/LdapSecurity/ldapconnection.cpp @@ -408,11 +408,8 @@ class CLdapConfig : implements ILdapConfig, public CInterface cfg->getProp(".//@ldapAdminVaultId", vaultId);//optional HashiCorp vault ID DBGLOG("Retrieving LDAP Admin username/password from secrets repo: %s %s", !vaultId.isEmpty() ? vaultId.str() : "", adminUserSecretKey.str()); - Owned secretTree; - if (!vaultId.isEmpty()) - secretTree.setown(getVaultSecret("authn", vaultId, adminUserSecretKey.str(), nullptr)); - else - secretTree.setown(getSecret("authn", adminUserSecretKey.str())); + + Owned secretTree(getSecret("authn", adminUserSecretKey.str(), vaultId, nullptr)); if (!secretTree) throw MakeStringException(-1, "Error retrieving LDAP Admin username/password"); @@ -497,11 +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; - if (!vaultId.isEmpty()) - secretTree.setown(getVaultSecret("authn", vaultId, adminUserSecretKey.str(), nullptr)); - else - secretTree.setown(getSecret("authn", adminUserSecretKey.str())); + Owned secretTree(getSecret("authn", adminUserSecretKey.str(), vaultId, nullptr)); if (secretTree) { getSecretKeyValue(m_HPCCAdminUser_username, secretTree, "username");