More changes following review

Signed-off-by: Gavin Halliday <[email protected]>

esp/clients/wsdfuaccess/wsdfuaccess.cpp

@@ -555,7 +555,7 @@ StringBuffer &encodeDFUFileMeta(StringBuffer &metaInfoBlob, IPropertyTree *metaI
         metaInfoEnvelope->setProp("certificate", certificate);
         Owned<CLoadedKey> 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<CLoadedKey> privateKey = loadPrivateKeyFromFile(privateKeyFName, nullptr);

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<ISyncedPropertyTree>
+class SyncedPropertyTreeWrapper : extends CInterfaceOf<ISyncedPropertyTree>
 {
 public:
-    UnchangingPropertyTree(IPropertyTree * _tree) : tree(_tree)
+    SyncedPropertyTreeWrapper(IPropertyTree * _tree) : tree(_tree)
     {
     }
 
@@ -10221,5 +10222,5 @@ class UnchangingPropertyTree : extends CInterfaceOf<ISyncedPropertyTree>
 
 ISyncedPropertyTree * createSyncedPropertyTree(IPropertyTree * tree)
 {
-    return new UnchangingPropertyTree(tree);
+    return new SyncedPropertyTreeWrapper(tree);
 }

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);

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);

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);