HPCC-30160 Automatic SOAPCALL secret credentials #17737

afishbeck · 2023-09-01T20:56:24Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-09-01T20:56:55Z

https://track.hpccsystems.com/browse/HPCC-30160
Jira updated

afishbeck · 2023-09-01T21:04:10Z

@ghalliday this is a draft of how automatic replacement of URLs with secrets might work.

Basically, starting with some ECL using a URL:

ds := HTTPCALL('http://localhost:8010/WsWorkunits/WUQuery.json', 'GET', 'application/json', OutRec1);
OUTPUT(ds);

Enable configuration for automatic URL to secret mapping either in values.yaml or environment.conf:

mapSoapCallUrlsToSecrets=true
warnIfUrlNotMappedToSecret=true

Calculate the needed secret name via command line tool:

> ecl url-secret-name https://user:pw@localhost:8010/WsWorkunits/WUQuery.json
http-connect-ssl-localhost-8010-3065364863

Note: I think having some parts readable will help operations be able to manage. But credentials, path, and parameters should not be visible due to possible exposure of sensitive information.

Create a secret with that name in vault or kubernetes (under ecl category):

http-connect-ssl-localhost-8010-3065364863
{
  "url": "http://localhost:8880/roxie/abc?123",
  "username": "aaa",
  "password": "bbb"
 }

Publish and run the query, which should now go to the desination in the secret using that username and pw.

ghalliday

@afishbeck this looks really good. A few comments, many of which should be ignored for this PR.

system/jlib/jsecrets.cpp

ghalliday · 2023-09-04T14:41:47Z

system/jlib/jsecrets.cpp

+            hashvalue = hashcz((const unsigned char *)userPasswordPair, hashvalue);
+    }
+    if (hashvalue)
+        secretName.append('-').append(hashvalue);


would the following be better than decimal?

appendf("-%08x", hashvalue)

I agree hex would be better, but I may leave off the leading 0's unless you think it would be beneficial in some way.

ghalliday · 2023-09-04T14:53:49Z

common/thorhelper/thorsoapcall.cpp

-    CriticalBlock block(persistentCrit);
-    if (!persistentInitDone)
+    CriticalBlock block(globalFeatureCrit);
+    if (!globalFeaturesInitDone)


See https://github.com/hpcc-systems/HPCC-Platform/pull/17715/files for an alternative way of coding this that also allows the values to be updated when the configuration changes. You may want to adopt something similar, but may be better as a separate PR since this looks very close to ready

Yes that would be a good follow on PR

ghalliday · 2023-09-04T14:54:50Z

common/thorhelper/thorsoapcall.cpp

-        int maxPersistentRequests = conf->getPropInt("@maxHttpCallPersistentRequests", 0);
-#endif
+        int maxPersistentRequests = 0;
+        if (isContainerized())


I think better to always use the containerized code, and have the configuration in the esp config.

@ghalliday I'm not sure how best to make that work. But if we were going to do it for bare-metal now is the time otherwise the way it's configured would change over time.
The process this is running in wouldn't be esp, it would be roxie, or eclagent, or thor. I don't think when those are running on bare-metal they have a common way of getting at their config? I could be wrong but in a simple test I didn't see the componentConfig getting set.

Ok much better. It's now using the component config in both cloud and bare-metal.

ghalliday · 2023-09-04T14:59:21Z

common/thorhelper/thorsoapcall.cpp

+    if (!secret)
+    {
+        if (required)
+            throw MakeStringException(0, "%sCALL %s SECRET not found", wscType == STsoap ? "SOAP" : "HTTP", secretName);


trivial/unrelated.

static const char * queryCallName(WSCType wscType) { return wscType == STsoap ? "SOAPCALL" : "HTTPCALL"; }
would clean the code up.

went ahead and cleaned this up while I was in there.

ghalliday · 2023-09-04T15:07:00Z

common/thorhelper/thorsoapcall.cpp

+                url.getDynamicUrlSecretName(secretName);
+                if (secretName.length())
+                {
+                    if (loadConnectSecret(nullptr, secretName, tempArray, issuer, proxyAddress, false, wscType))


Add an option to require all soapcalls use secrets (and pass true)?

ghalliday · 2023-09-15T14:51:52Z

@afishbeck I think Jake will probably want to implement his changes for https://track.hpccsystems.com/browse/HPCC-30259 on top of this, so worth finishing it and getting it merged. Is there anything I can do to help?

afishbeck · 2023-09-19T23:47:27Z

@ghalliday updated, please review.

ghalliday

A couple of comments, but looks like it is very close to ready.

ghalliday · 2023-09-21T14:50:04Z

common/thorhelper/thorsoapcall.cpp

@@ -885,21 +884,21 @@ bool loadConnectSecret(const char *vaultId, const char *secretName, UrlArray &ur
    if (!secret)
    {
        if (required)
-            throw MakeStringException(0, "%sCALL %s SECRET not found", wscType == STsoap ? "SOAP" : "HTTP", secretName);
+            throw MakeStringException(0, "%sCALL %s SECRET not found", getWsCallTypeName(wscType), secretName);


%sCALL will now become %s

initfiles/componentfiles/configxml/eclagent_config.xsd.in

afishbeck · 2023-09-21T16:10:58Z

@ghalliday updated json schema and log/exception text.

ghalliday

@afishbeck looks good, please squash

afishbeck · 2023-09-21T16:39:37Z

@ghalliday squashed and resolved conflicts.

Signed-off-by: Anthony Fishbeck <anthony.fishbeck@lexisnexis.com>

afishbeck requested a review from ghalliday September 1, 2023 21:10

ghalliday reviewed Sep 4, 2023

View reviewed changes

ghalliday marked this pull request as ready for review September 15, 2023 14:51

afishbeck requested a review from ghalliday September 19, 2023 23:47

ghalliday reviewed Sep 21, 2023

View reviewed changes

afishbeck requested a review from ghalliday September 21, 2023 16:09

ghalliday approved these changes Sep 21, 2023

View reviewed changes

ghalliday changed the base branch from master to candidate-9.4.x September 21, 2023 16:14

afishbeck force-pushed the soapcallAutoConnectSecret branch from 264bd8f to c97e506 Compare September 21, 2023 16:37

HPCC-30160 Automatic SOAPCALL secret credentials

875692d

Signed-off-by: Anthony Fishbeck <anthony.fishbeck@lexisnexis.com>

afishbeck force-pushed the soapcallAutoConnectSecret branch from c97e506 to 875692d Compare September 21, 2023 17:28

ghalliday merged commit 71bc409 into hpcc-systems:candidate-9.4.x Sep 22, 2023
47 of 50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30160 Automatic SOAPCALL secret credentials #17737

HPCC-30160 Automatic SOAPCALL secret credentials #17737

afishbeck commented Sep 1, 2023 •

edited

github-actions bot commented Sep 1, 2023

afishbeck commented Sep 1, 2023 •

edited

ghalliday left a comment

ghalliday Sep 4, 2023

afishbeck Sep 5, 2023

ghalliday Sep 4, 2023

afishbeck Sep 5, 2023

ghalliday Sep 4, 2023

afishbeck Sep 5, 2023 •

edited

afishbeck Sep 19, 2023

ghalliday Sep 4, 2023

afishbeck Sep 19, 2023

ghalliday Sep 4, 2023

afishbeck Sep 19, 2023

ghalliday commented Sep 15, 2023

afishbeck commented Sep 19, 2023

ghalliday left a comment

ghalliday Sep 21, 2023

afishbeck commented Sep 21, 2023

ghalliday left a comment

afishbeck commented Sep 21, 2023 •

edited

HPCC-30160 Automatic SOAPCALL secret credentials #17737

HPCC-30160 Automatic SOAPCALL secret credentials #17737

Conversation

afishbeck commented Sep 1, 2023 • edited

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Sep 1, 2023

afishbeck commented Sep 1, 2023 • edited

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck Sep 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Sep 15, 2023

afishbeck commented Sep 19, 2023

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck commented Sep 21, 2023

ghalliday left a comment

Choose a reason for hiding this comment

afishbeck commented Sep 21, 2023 • edited

afishbeck commented Sep 1, 2023 •

edited

afishbeck commented Sep 1, 2023 •

edited

afishbeck Sep 5, 2023 •

edited

afishbeck commented Sep 21, 2023 •

edited