From f0178778142ccee6d62eddbb7c78e8bf0e7f27bb Mon Sep 17 00:00:00 2001 From: Gavin Halliday Date: Tue, 7 Nov 2023 11:21:24 +0000 Subject: [PATCH] More changes following review Signed-off-by: Gavin Halliday --- esp/clients/wsdfuaccess/wsdfuaccess.cpp | 2 +- system/jlib/jptree.cpp | 9 +++++---- system/jlib/jptree.hpp | 10 ++++------ system/jlib/jsecrets.cpp | 9 ++++++++- system/jlib/jsecrets.hpp | 4 ++++ 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/esp/clients/wsdfuaccess/wsdfuaccess.cpp b/esp/clients/wsdfuaccess/wsdfuaccess.cpp index 3726aee1f95..b8d59e7189e 100644 --- a/esp/clients/wsdfuaccess/wsdfuaccess.cpp +++ b/esp/clients/wsdfuaccess/wsdfuaccess.cpp @@ -555,7 +555,7 @@ StringBuffer &encodeDFUFileMeta(StringBuffer &metaInfoBlob, IPropertyTree *metaI metaInfoEnvelope->setProp("certificate", certificate); Owned privateKey = loadPrivateKeyFromMemory(privateKeyText, nullptr); #else - const char *privateKeyFName = privateKeyFName = environment->getPrivateKeyPath(keyPairName); + const char *privateKeyFName = environment->getPrivateKeyPath(keyPairName); if (isEmptyString(privateKeyFName)) throw makeStringExceptionV(-1, "Key name '%s' is not found in environment settings: /EnvSettings/Keys/KeyPair.", keyPairName); Owned privateKey = loadPrivateKeyFromFile(privateKeyFName, nullptr); diff --git a/system/jlib/jptree.cpp b/system/jlib/jptree.cpp index bcf44f01aba..994d916f422 100644 --- a/system/jlib/jptree.cpp +++ b/system/jlib/jptree.cpp @@ -10141,7 +10141,8 @@ void setExpertOpt(const char *opt, const char *value) //--------------------------------------------------------------------------------------------------------------------- -//More: Should this move inside PTree? It may allow a more efficient hash caclulaton. +//HPCC-30752 This should move inside PTree to allow a more efficient hash calculation and possible caching. +//Currently the values are not persisted so the implementation could change. That may change in the future. unsigned getPropertyTreeHash(const IPropertyTree & source, unsigned hashcode) { if (source.isBinary()) @@ -10174,10 +10175,10 @@ unsigned getPropertyTreeHash(const IPropertyTree & source, unsigned hashcode) return hashcode; } -class UnchangingPropertyTree : extends CInterfaceOf +class SyncedPropertyTreeWrapper : extends CInterfaceOf { public: - UnchangingPropertyTree(IPropertyTree * _tree) : tree(_tree) + SyncedPropertyTreeWrapper(IPropertyTree * _tree) : tree(_tree) { } @@ -10221,5 +10222,5 @@ class UnchangingPropertyTree : extends CInterfaceOf ISyncedPropertyTree * createSyncedPropertyTree(IPropertyTree * tree) { - return new UnchangingPropertyTree(tree); + return new SyncedPropertyTreeWrapper(tree); } diff --git a/system/jlib/jptree.hpp b/system/jlib/jptree.hpp index 789343a96bf..80888c8218d 100644 --- a/system/jlib/jptree.hpp +++ b/system/jlib/jptree.hpp @@ -435,18 +435,16 @@ extern jlib_decl void setExpertOpt(const char *opt, const char *value); extern jlib_decl unsigned getPropertyTreeHash(const IPropertyTree & source, unsigned hashcode); //Interface for encapsulating an IPropertyTree that can be atomically updated. The result of getTree() is guaranteed -//to not be modified and to remain vali and consistent until it is released. +//to not be modified and to remain valid and consistent until it is released. interface ISyncedPropertyTree : extends IInterface { 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 sequence number which changes whenever the secret actually changes - so that a caller can determine - //whether it needs to reload the certificates. + //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; - //An indication that the property tree may be out of date because it couldn't be resynchronized. - virtual bool isStale() const = 0; - virtual bool isValid() const = 0; // Was an entry found for this secret? + 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. }; extern jlib_decl ISyncedPropertyTree * createSyncedPropertyTree(IPropertyTree * tree); diff --git a/system/jlib/jsecrets.cpp b/system/jlib/jsecrets.cpp index afb465e0259..813ad588264 100644 --- a/system/jlib/jsecrets.cpp +++ b/system/jlib/jsecrets.cpp @@ -1474,10 +1474,17 @@ void CIssuerConfig::updateConfigFromSecret(const IPropertyTree * secretInfo) con if (!isClientConnection || !strieq(issuer, "public")) updateCertificateFromSecret(secretInfo); - if (!isClientConnection || addCACert) // same condition as before, but is it correct? + + // addCACert is usually true. A client hitting a public issuer is the case where we don't want the ca cert + // defined. Otherwise, for MTLS we want control over our CACert using addCACert. When hitting public services + // using public certificate authorities we want the well known (browser compatible) CA list installed on the + // system instead. + if (!isClientConnection || addCACert) updateCertificateAuthorityFromSecret(secretInfo); IPropertyTree *verify = config->queryPropTree("verify"); + assertex(verify); // Should always be defined by this point. + //For now only the "public" issuer implies client certificates are not required verify->setPropBool("@enable", !disableMTLS && (isClientConnection || !strieq(issuer, "public"))); verify->setPropBool("@address_match", false); diff --git a/system/jlib/jsecrets.hpp b/system/jlib/jsecrets.hpp index 029716986b1..9601deb4d7d 100644 --- a/system/jlib/jsecrets.hpp +++ b/system/jlib/jsecrets.hpp @@ -27,7 +27,11 @@ interface ISyncedPropertyTree; 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); +// 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 +// 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); extern jlib_decl bool getSecretKeyValue(MemoryBuffer & result, const IPropertyTree *secret, const char * key);