HPCC-28950 JFrog for Manifest/Resource File Management #17674

jackdelv · 2023-08-14T16:00:54Z

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-08-14T16:01:15Z

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

ghalliday

@jackdelv Don't be put off by the number of comments! Feel free to ask me about any of them.

ghalliday · 2023-08-31T10:16:56Z

ecl/hql/hqlmanifest.cpp

@@ -198,6 +198,31 @@ void ResourceManifest::loadResource(const char *filepath, MemoryBuffer &content)
    read(fio, 0, (size32_t) f->size(), content);
 }

+void queryJfrog(const char *localpath, IPropertyTree &item)


style: We use query as a prefix for functions that return a value, e.g. queryName().

Changed to callJfrog.

ghalliday · 2023-08-31T10:18:40Z

ecl/hql/hqlmanifest.cpp

+        getFileNameOnly(filename, false);
+        StringBuffer filepath;
+        StringBuffer machine;
+        if (repo[0] == '/' && repo[1] == '/')


What are the security implications of this change? As far as I can see it would allow me to overwrite any local file with something stored in a jfrog repository.

Yes, jfrog would overwrite the file with a resource from artifactory. I have changed it to write to a temporary directory.

ghalliday · 2023-08-31T10:25:14Z

ecl/hql/hqlmanifest.cpp

+        if (user && getSecretValue(secretValue.clear(), "jfrog", user, "key", true))
+            jfrog_cmd.appendf("--user=%s --password=%s ", user, secretValue.trimRight().str());
+        jfrog_cmd.appendf("\"%s%s\" %s >/dev/null 2>&1", filepath.str(), filename.str(), localpath);
+        system(jfrog_cmd.str());


Use invoke_program() or runExternalCommand() rather than system.
What happens if the jfrog command fails?

I am now using runExternalCommand to run the jfrog cli and checking for any errors.

ghalliday · 2023-08-31T10:27:23Z

ecl/hql/hqlmanifest.cpp

+            splitFilename(repo, nullptr, &filepath, nullptr, nullptr);
+        StringBuffer jfrog_cmd("export CI=true; jf rt dl --flat --quiet ");
+        if (!machine.isEmpty())
+            jfrog_cmd.appendf("--url=http:%s/artifactory ", machine.str());


http is not secure. Ideally we should be verifying that the server is valid.

I agree and JFrog has steps for setting up an artifactory repository to work with https, but it is not available on the open source version that I am running of artifactory.

ghalliday · 2023-08-31T10:28:35Z

ecl/hql/hqlmanifest.cpp

+    const char *repo = item.queryProp("@repository");
+    if(repo)
+    {
+        StringBuffer filename(localpath);


question: Should localpath be passed in, or should this function be responsible for creating a temporary local path in a valid location? (I don't know what typical requirements would be.)

For now, I have created a temporary directory under /tmp/HPCCSystems/jfrog_downloads/

I think this should be using

getTempFilePath(tmpDirName, "jfrog", nullptr);

or similar to create the temporary directory.

ghalliday · 2023-08-31T10:30:33Z

ecl/hql/hqlmanifest.cpp

+        StringBuffer secretValue;
+        const char *user = item.queryProp("@user");
+        if (user && getSecretValue(secretValue.clear(), "jfrog", user, "key", true))
+            jfrog_cmd.appendf("--user=%s --password=%s ", user, secretValue.trimRight().str());


Passing secrets on the command line isn't very secure. Is there a better alternative way?

The only other way is to have the user configure jfrog at the command line with jf c add.

ghalliday · 2023-08-31T10:31:29Z

ecl/hqlcpp/hqlres.cpp

@@ -99,6 +99,31 @@ static void loadResource(const char *filepath, MemoryBuffer &content)
    read(fio, 0, (size32_t) f->size(), content);
 }

+static void queryJfrog(const char *localpath, IPropertyTree &item)


A duplicate of the code above? It should only be in one place.

Removed duplicate code and included hqlmanifest header file.

ghalliday · 2023-08-31T10:32:27Z

ecl/hql/hqlmanifest.cpp

@@ -223,6 +248,7 @@ void ResourceManifest::addToArchive(IPropertyTree *archive)
                    IPropertyTree *resTree = additionalFiles->addPropTree("Resource", createPTree("Resource"));
                    resTree->setProp("@filename", filename);
                    resTree->setProp("@md5", md5);
+                    queryJfrog(filename, item);


I don't think we want to be downloading from jfrog when we create the archives - otherwise that defeats the object of putting off downloading the resource.

I am not sure where else to download from jfrog. Wouldn't any other time be too late?

ghalliday · 2023-08-31T10:34:47Z

ecl/hql/hqlmanifest.cpp

@@ -240,6 +266,7 @@ void ResourceManifest::addToArchive(IPropertyTree *archive)
                const char *filepath = item.queryProp("@originalFilename");
                resTree->setProp("@originalFilename", filepath);
                resTree->setProp("@resourcePath", respath);
+                queryJfrog(filepath, item);                


This doesn't need to write the resource to a known filename - it could read it into the content. Otherwise there is the issue of cleaning up the downloads.

I am not sure what you mean by this. The only way to access the contents in jfrog artifactory is to download the files.

ghalliday · 2023-08-31T10:37:14Z

ecl/hql/hqlmanifest.cpp

+            jfrog_cmd.appendf("--user=%s --password=%s ", user, secretValue.trimRight().str());
+        jfrog_cmd.appendf("\"%s%s\" %s >/dev/null 2>&1", filepath.str(), filename.str(), localpath);
+        system(jfrog_cmd.str());
+    }


We also need an option to verify that the hash for an entry we have downloaded matches a hash specified in the resource.

Added option for adding md5 hash of resource in callJfrog.

ghalliday · 2023-08-31T10:38:47Z

@jackdelv also note some of the tests are failing. It is worth going back to your PRs after a day to check if they are showing up any problems.

jackdelv · 2023-09-13T17:21:53Z

@ghalliday back to you. I have responded to all of your comments and left a couple of questions.

ghalliday

@jackdelv sorry for not re-reviewing this earlier. I have added a few comments, but I think it would be worth scheduling some time to discuss the feature a bit more before responding to them.
I will email the potential users to check exactly what would be useful for this functionality.

ghalliday · 2023-12-05T11:37:57Z

ecl/hql/hqlmanifest.cpp

@@ -221,9 +267,12 @@ void ResourceManifest::addToArchive(IPropertyTree *archive)
                if (!additionalFiles->hasProp(xpath.str()))
                {
                    IPropertyTree *resTree = additionalFiles->addPropTree("Resource", createPTree("Resource"));
+                    StringBuffer filepath(filename);
+                    callJfrog(filepath, absFilename, item);


Clearer to have the if (repo) check outside the call - otherwise when reading this code is poses the question "why is it calling jfrog for a local file.)
Alternatively rename the function to make it clearer what it is doing. E.g.

checkForDownloadFromJfrog()

if the test was outside it would still be better renamed getResourceFromJfrog() or similar

Changed to getResourceFromJfrog() and moved check outside the call.

ghalliday · 2023-12-05T11:42:19Z

ecl/hql/hqlmanifest.cpp

+        StringBuffer userValue;
+        StringBuffer keyValue;
+        const char *repoName = item.queryProp("@repoName");
+        if (repoName && getSecretValue(keyValue, "jfrog", repoName, "key", true) && getSecretValue(userValue, "jfrog", repoName, "user", true))


Is there a more secure way of passing the credentials? Command lines can be read by other processes.

The user can do a one time configuration of the artifactory connection to avoid passing credentials each time we download from JFrog. I removed the code for passing credentials via the command line.

ghalliday · 2023-12-05T11:44:17Z

ecl/hql/hqlmanifest.cpp

+            jfrog_cmd.appendf("--url=http:%s/artifactory ", machine.str());
+        StringBuffer userValue;
+        StringBuffer keyValue;
+        const char *repoName = item.queryProp("@repoName");


repoName isn't very descriptive. How does it relate to repository?

I intended it to allow for multiple artifactory servers to be used. I removed it because it is no longer needed now that the connection is configured once by the user.

ghalliday · 2023-12-05T11:45:20Z

ecl/hql/hqlmanifest.cpp

+        jfrog_cmd.appendf("\"%s%s\" %s", filepath.str(), filename.str(), downloadPath.str());
+        StringBuffer errOut;
+        StringBuffer jfrogOut;
+        if (system("export CI=true") != 0)


I'm surprised this works. The correct way is to use the version of runExternalCommand which allows environment variables to be passed.

ghalliday · 2023-12-05T11:47:12Z

ecl/hql/hqlmanifest.cpp

+            throw makeStringExceptionV(0, "Error exporting jfrog setting: export CI=true");
+        else if(runExternalCommand(errOut, jfrogOut, jfrog_cmd.str(), nullptr) != 0)
+            throw makeStringExceptionV(0, "Error loading resource from jfrog: \n\"%s\"\n %s", jfrogOut.str(), errOut.str());
+        const char *md5 = item.queryProp("@md5");


This check would make sense for non-jfrog as well - so I would move out of this function.

I removed this check because it already occurs outside of the JFrog call.

ghalliday · 2023-12-05T11:49:30Z

ecl/hql/hqlmanifest.cpp

+        if (repoName && getSecretValue(keyValue, "jfrog", repoName, "key", true) && getSecretValue(userValue, "jfrog", repoName, "user", true))
+            jfrog_cmd.appendf("--user=%s --password=%s ", userValue.trimRight().str(), keyValue.trimRight().str());
+        jfrog_cmd.appendf("\"%s%s\" %s", filepath.str(), filename.str(), downloadPath.str());
+        StringBuffer errOut;


personally I would add a few newlines to break up the function and make it easier to read. For instance before this line, which is the beginning of the code to execute the command, and line 220 which is the start of the code to build up the command string.

ghalliday · 2023-12-05T11:54:07Z

ecl/hql/hqlmanifest.cpp

+    const char *repo = item.queryProp("@repository");
+    if(repo)
+    {
+        StringBuffer filename(localpath);


I think this should be using

getTempFilePath(tmpDirName, "jfrog", nullptr);

or similar to create the temporary directory.

jackdelv · 2024-01-05T16:12:13Z

@ghalliday I addressed all your comments and added some changes. Back to you.

ghalliday

@jackdelv a few more comments.

ghalliday · 2024-01-09T14:22:48Z

testing/regress/ecl/javaembed_ex7.manifest

@@ -1,3 +1,3 @@
 <Manifest>
- <Resource type='jar' filename='javaembed_ex7.jar'/>
+ <Resource type='jar' filename='javaembed_ex7.jar' repository='/generic-local/javaFiles/' md5='d933045e7fed10ff9ccddfc9743b0538'/>


I assume this is for testing and it should be reverted before merging.

Yes, that will be reverted.

ghalliday · 2024-01-09T14:23:23Z

ecl/hql/hqlmanifest.cpp

+    getTempFilePath(downloadPath, "jfrog", nullptr);
+    downloadPath.append(filename);
+
+    StringBuffer jfrogCmd("jf rt dl --flat --quiet ");


How is authentication handled?

The server address and credentials are configured with the JFrog cli before making any calls to Artifactory. The connections from getResourceFromJfrog use the new configuration instead of having to pass the credentials at the command line.

ghalliday · 2024-01-09T14:25:03Z

ecl/hql/hqlmanifest.cpp

@@ -221,9 +242,13 @@ void ResourceManifest::addToArchive(IPropertyTree *archive)
                if (!additionalFiles->hasProp(xpath.str()))
                {
                    IPropertyTree *resTree = additionalFiles->addPropTree("Resource", createPTree("Resource"));
+                    StringBuffer filepath(filename);
+                    if (item.queryProp("@repository"))


I think this is a case where you do not want to fetch the artefact at this point. This is before the query is sent to the system - so it would be downloaded by the user, added to the archive and then uploaded. The aim of this change is to avoid that step.

Ok, I think I understand. I am just not sure where the resource is used after being added to the archive.

jackdelv · 2024-01-16T16:23:45Z

@ghalliday I moved the call to getResourceFromJfrog to when the resource is fetch from the archive rather than storing it in the archive.

ghalliday · 2024-01-23T10:37:33Z

Let's discuss this verbally. We missed out clearly talking through the requirements first - we need to do that and then see where we stand.

jackdelv · 2024-01-24T20:59:46Z

@ghalliday Authentication for Jfrog is now set up using the secrets vault. A Jfrog config file is created rather than passing the credentials through the command line.

ghalliday

@jackdelv This looks close. A few final comments.

Also a general comment - it is better not to rebase your branch unless you need to because it makes it harder to track which changes have taken place since the last review.

Also please can you update the jira to contain a summary of what has been implemented - e.g. the format and options in the resource definition to use the feature.

It will also need some changes to ensure the secrets are accessible from containerized builds, but I will add a separate comment for that,

ghalliday · 2024-02-16T11:14:58Z

common/dllserver/CMakeLists.txt

@@ -38,7 +38,8 @@ include_directories (
         ./../../rtl/eclrtl 
         ./../../system/include 
         ./../../dali/base 
-         ./../../system/jlib 
+         ./../../system/jlib
+         ./../../ecl/hql


This dll shouldn't really have a dependency on hql.
I think the cleanest solution is to move getResourceFromJfrog into jlib. Either into a new file, or added to something like jutil.hpp/cpp

Moved getResourceFromJfrog to jutil.

ghalliday · 2024-02-16T11:15:53Z

ecl/hql/hqlmanifest.cpp

+
+void getResourceFromJfrog(StringBuffer &localPath, IPropertyTree &item)
+{
+    addSecretToJfrog(item);


security: Ensure this file is deleted when the command has executed (for success or failure)

ghalliday · 2024-02-16T11:17:29Z

ecl/hql/hqlmanifest.cpp

+    ],\n\
+    \"version\": \"6\"\n}\n", server, server, server, server, server, server, user, password.str());
+
+    StringBuffer configPath(getenv("HOME"));


Use getHomeDir() in jmisc.hpp - which will also work on windows.
I assume this is a location defined by jfrog.

Yes, this is the location where the JFrog CLI expects the config file to be if there are no credentials passed through the command line.

ghalliday · 2024-02-16T11:25:32Z

ecl/hql/hqlmanifest.cpp

+{
+    StringBuffer password;
+    const char *user = item.queryProp("@jfrogUser");
+    getSecretValue(password, "jfrog", user, "password", true);


@afishbeck Do you think this is reasonable to add a new category to access jfrog secrets? Are we going to continue to have new secret categories?
Would it be better to have an "external" category with a key of "jfrog:"? What other alternatives are there. The only other similar example at the moment is "git", but maybe we will end up with docker, or other examples.
Thoughts?

ghalliday · 2024-02-20T11:53:39Z

@jackdelv great, it looks like it is ready to merge. Please can you

squash all the commits
Add details into the jira about how it is used - what the options for the manifest are, how the secrets are configured etc.
Create a documentation jira and link to this one.
Talk to @cloLN about adding a test using the opinionated cluster to make sure it works end to end on a container system.

Actually... we still need to think about making the secrets available in container builds from the helm files. I will ask Tony exactly what he thinks, but I think worth squashing all changes so far.

jackdelv · 2024-02-20T13:43:17Z

@ghalliday I added a description to this JIRA detailing the options and adding the secrets. If I left anything out please let me know. Here is the documentation JIRA: https://track.hpccsystems.com/browse/HPCC-31310. Back to you.

ghalliday · 2024-02-21T10:17:08Z

I think the final change is to update the helm files to ensure the secrets are available in the cloud version. Two places need to change (the same place that "git" secrets are currently supported):
issuers.yaml: line 139 ($categories :=) add "jfrog" to the list
eclccserver.yaml: line 127 ($secretsCategories :=...) . add jfrog to the list.
Please feel free to make those changes and squash them at the same time.

jackdelv · 2024-02-21T13:52:21Z

@ghalliday Squashed.

dcamper requested a review from ghalliday August 14, 2023 16:02

ghalliday reviewed Aug 31, 2023

View reviewed changes

jackdelv requested a review from ghalliday September 26, 2023 12:03

jackdelv marked this pull request as ready for review October 26, 2023 15:55

jackdelv requested review from ghalliday and removed request for ghalliday October 26, 2023 15:57

ghalliday reviewed Dec 5, 2023

View reviewed changes

jackdelv force-pushed the JFrog-dev branch from f489741 to 88c5141 Compare January 5, 2024 15:48

jackdelv requested a review from ghalliday January 5, 2024 16:12

ghalliday reviewed Jan 9, 2024

View reviewed changes

jackdelv requested a review from ghalliday January 9, 2024 14:57

jackdelv force-pushed the JFrog-dev branch 2 times, most recently from 6b30f8a to e0673a8 Compare January 16, 2024 15:27

ghalliday reviewed Feb 16, 2024

View reviewed changes

jackdelv requested a review from ghalliday February 16, 2024 15:57

ghalliday approved these changes Feb 20, 2024

View reviewed changes

jackdelv force-pushed the JFrog-dev branch from 83f970f to 3289f11 Compare February 20, 2024 13:10

jackdelv requested a review from ghalliday February 20, 2024 13:43

HPCC-28950 JFrog Authentication for Manifest/Resource File Management

72e9200

jackdelv force-pushed the JFrog-dev branch from 3289f11 to 72e9200 Compare February 21, 2024 13:50

ghalliday merged commit 4aa1362 into hpcc-systems:master Feb 22, 2024
49 of 51 checks passed

HPCC-28950 JFrog for Manifest/Resource File Management #17674

HPCC-28950 JFrog for Manifest/Resource File Management #17674

Conversation

jackdelv commented Aug 14, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Aug 14, 2023

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

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

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

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 Aug 31, 2023

jackdelv commented Sep 13, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackdelv commented Jan 5, 2024

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

Choose a reason for hiding this comment

jackdelv commented Jan 16, 2024

ghalliday commented Jan 23, 2024

jackdelv commented Jan 24, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Feb 20, 2024

jackdelv commented Feb 20, 2024

ghalliday commented Feb 21, 2024

jackdelv commented Feb 21, 2024

jackdelv commented Aug 14, 2023 •

edited

Loading