feat: Add plugin call variables to sidecar plugin discovery (#9273) #9319

pierrecregut · 2022-05-06T14:25:21Z

Gives access to variables declared in the call of the plugin in the application
manifest to the discover command run on the CMP server.

Variables are prefixed with ENV_ to avoid security issues (plugin call
overiding important variables).
Modified variable names are also given to the generate command but the variables
with unmodified names are also defined to preserve the old behavior until it is
deprecated.

Fixes #9273
Fixes #8986

Signed-off-by: Pierre Crégut pierre.cregut@orange.com

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

codecov · 2022-05-06T14:37:32Z

Codecov Report

Merging #9319 (ce0ad30) into master (c026189) will increase coverage by 0.01%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master    #9319      +/-   ##
==========================================
+ Coverage   45.77%   45.79%   +0.01%     
==========================================
  Files         220      220              
  Lines       26168    26171       +3     
==========================================
+ Hits        11979    11984       +5     
+ Misses      12531    12529       -2     
  Partials     1658     1658

Impacted Files	Coverage Δ
util/app/discovery/discovery.go	`40.27% <33.33%> (ø)`
cmpserver/plugin/plugin.go	`49.28% <50.00%> (+1.80%)`	⬆️
reposerver/repository/repository.go	`59.78% <66.66%> (+0.08%)`	⬆️
util/settings/settings.go	`48.16% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c026189...ce0ad30. Read the comment docs.

crenshaw-dev

lgtm, thanks!

reposerver/repository/repository.go

crenshaw-dev · 2022-05-09T20:58:03Z

reposerver/repository/repository.go

-			pluginEnv[i].Value = parsedEnv.Envsubst(j.Value)
+		for _, entry := range pluginEnv {
+			newValue := parsedEnv.Envsubst(entry.Value)
+			if legacy {


@leoluz suggested we consider removing the legacy behavior completely in 2.4. I think it's worth considering moving aggressively, since it's a security enhancement. I've added a note to Thursday's contributor meeting to discuss. https://docs.google.com/document/d/1xkoFkVviB70YBzSEa4bDnu-rUZ1sIFtwKKG1Uw8XsY8/edit#

That would clearly simplify my code.

But this will also break published plugins. Have you considered building the env array in reverse order ? This way a user cannot override an existing variable. This may break some usage where the environment of the plugin were just default values but it has probably less impact than forcing a change in parameter names.

I will modify the code according to the outcome of Thursday meeting.

As a side note, in repository.go:

argo-cd/reposerver/repository/repository.go

Line 1274 in 0800921

env := append(os.Environ(), envVars.Environ()...)

the environment of the caller reposerver is given and added to the list that will modify the environment of the cmpServer

argo-cd/cmpserver/plugin/plugin.go

Line 191 in 0800921

env := append(os.Environ(), environ(envEntries)...)

I am not sure if this is really intended.

But this will also break published plugins.

True. I think it's only justified if we think the security benefit is significant and we can communicate the change effectively.

Have you considered building the env array in reverse order ? This way a user cannot override an existing variable.

I don't think that's sufficient to resolve the security concern. Tooling in the CMP might pick up env vars that aren't explicitly set elsewhere and allow malicious behavior.

I am not sure if this is really intended.

I think allowing the user to override CMP-side vars was intentional, but I think it was a bad idea.

By prefixing the env var names, I think we'll make it so that the array order doesn't matter - there should be no overlap in env var names.

The side note was about the fact that both the repo server and the cmp server appended their environment variables to the environment used in the call (priority for the repoServer. For example, the value of PATH is the wrong one). I think it is a side effect of the fact that getPluginEnvs is also used for plugin loaded in the repoServer.

Oh wait. So the repo-server PATH var (and other vars) end up taking precedence over the sidecar? If so, yikes.

I have made a real test adding a variable TEST to both containers with value either repo-server or cmp-server and I can confirm what I inferred from the code TEST value was repo-server in the plugin. I can add a boolean variable remote to distinguish between the behavior for

argo-cd/reposerver/repository/repository.go

Line 1255 in 0800921

env, err := getPluginEnvs(envVars, q, creds)

and the behavior required for sidecars.

But we should probably keep it in a separate commit as it is a separate bugfix.

Agreed, separate PR makes sense.

I have pushed #9397

crenshaw-dev · 2022-05-10T17:38:20Z

Thanks for making the change! I'll let you know the outcome of tomorrow's meeting, if you're not able to attend yourself.

crenshaw-dev · 2022-05-12T21:13:40Z

@pierrecregut the conclusion was that we should probably go ahead and aggressively remove the legacy behavior, but that 1) we should document the change and migration instructions in the upgrade guide and release notes (I'll open a PR) and 2) we should post in Slack about the change to make sure the community isn't strongly opposed (I'll do this as well).

For now if you want to go ahead and change this PR to remove legacy behavior, I'll approve. We can revert if there's strong opposition in Slack.

cc @todaywasawesome @leoluz @jopit

pierrecregut · 2022-05-13T08:45:50Z

I have removed the implementation of the legacy behavior and implemented the "aggressive" approach. I have removed the small documentation change letting you implement it in a separate PR.

This PR is rebased on top of the bug fixing PR #9397

getPluginEnvs is both used for local plugins and sidecar plugins. For the later do not include the environement variables of the repo-server in the supplied variables. Fixes: argoproj#9393 Signed-off-by: Pierre Crégut <pierre.cregut@orange.com>

…#9273) Gives access to variables declared in the call of the plugin in the application manifest to the discover command run on the CMP server. Variables are prefixed with ARGOCD_ENV_ to avoid security issues (plugin call overiding important variables). Fixes argoproj#9273 Signed-off-by: Pierre Crégut <pierre.cregut@orange.com>

crenshaw-dev · 2022-05-31T20:11:33Z

reposerver/repository/repository.go

-func getPluginEnvs(envVars *v1alpha1.Env, q *apiclient.ManifestRequest, creds git.Creds) ([]string, error) {
-	env := append(os.Environ(), envVars.Environ()...)
+func getPluginEnvs(envVars *v1alpha1.Env, q *apiclient.ManifestRequest, creds git.Creds, remote bool) ([]string, error) {
+	env := envVars.Environ()


Note: this is a breaking change. Plugins that rely on the environment variables from the main API server container will need that env var duplicated.

crenshaw-dev · 2022-05-31T20:19:39Z

util/app/discovery/discovery.go

@@ -33,7 +33,7 @@ func Discover(ctx context.Context, repoPath string, enableGenerateManifests map[
 	apps := make(map[string]string)

 	// Check if it is CMP
-	conn, _, err := DetectConfigManagementPlugin(ctx, repoPath)
+	conn, _, err := DetectConfigManagementPlugin(ctx, repoPath, []string{})


It's weird to me that this doesn't send any env. I think it's wrong. But I think that's a separate bug, not an issue with this PR.

crenshaw-dev

lgtm, thanks for your patience!

…9319) * fix: do not export repo-server environment to sidecar (#9393) getPluginEnvs is both used for local plugins and sidecar plugins. For the later do not include the environement variables of the repo-server in the supplied variables. Fixes: #9393 Signed-off-by: Pierre Crégut <pierre.cregut@orange.com> * feat: Add plugin call variables to sidecar plugin discovery (#9273) Gives access to variables declared in the call of the plugin in the application manifest to the discover command run on the CMP server. Variables are prefixed with ARGOCD_ENV_ to avoid security issues (plugin call overiding important variables). Fixes #9273 Signed-off-by: Pierre Crégut <pierre.cregut@orange.com>

crenshaw-dev · 2022-06-02T21:53:48Z

Cherry-picked onto 2.4.

pierrecregut force-pushed the feat-env-discover branch from e0e11b0 to f74e78f Compare May 6, 2022 14:41

crenshaw-dev approved these changes May 7, 2022

View reviewed changes

crenshaw-dev requested a review from leoluz May 7, 2022 16:37

crenshaw-dev requested changes May 9, 2022

View reviewed changes

pierrecregut force-pushed the feat-env-discover branch 2 times, most recently from 2e53f86 to ec8393b Compare May 10, 2022 08:34

pierrecregut force-pushed the feat-env-discover branch from ec8393b to ebe324e Compare May 13, 2022 08:42

pierrecregut force-pushed the feat-env-discover branch from ebe324e to e265d53 Compare May 13, 2022 09:32

pierrecregut force-pushed the feat-env-discover branch 2 times, most recently from 8e9dd7b to 91dac81 Compare May 13, 2022 15:32

pierrecregut force-pushed the feat-env-discover branch from 91dac81 to ce0ad30 Compare May 13, 2022 16:24

crenshaw-dev reviewed May 31, 2022

View reviewed changes

crenshaw-dev approved these changes May 31, 2022

View reviewed changes

crenshaw-dev merged commit 61f48d5 into argoproj:master May 31, 2022

crenshaw-dev mentioned this pull request May 31, 2022

fix: do not export repo-server environment to sidecar (#9393) #9397

Closed

10 tasks

crenshaw-dev added the cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch label Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add plugin call variables to sidecar plugin discovery (#9273) #9319

feat: Add plugin call variables to sidecar plugin discovery (#9273) #9319

pierrecregut commented May 6, 2022 •

edited by crenshaw-dev

codecov bot commented May 6, 2022 •

edited

crenshaw-dev left a comment

crenshaw-dev May 9, 2022

pierrecregut May 10, 2022

crenshaw-dev May 10, 2022

pierrecregut May 12, 2022

crenshaw-dev May 12, 2022

pierrecregut May 12, 2022 •

edited

crenshaw-dev May 12, 2022

pierrecregut May 13, 2022

crenshaw-dev commented May 10, 2022

crenshaw-dev commented May 12, 2022

pierrecregut commented May 13, 2022

crenshaw-dev May 31, 2022

crenshaw-dev May 31, 2022

crenshaw-dev left a comment

crenshaw-dev commented Jun 2, 2022

feat: Add plugin call variables to sidecar plugin discovery (#9273) #9319

feat: Add plugin call variables to sidecar plugin discovery (#9273) #9319

Conversation

pierrecregut commented May 6, 2022 • edited by crenshaw-dev

codecov bot commented May 6, 2022 • edited

Codecov Report

crenshaw-dev 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

pierrecregut May 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crenshaw-dev commented May 10, 2022

crenshaw-dev commented May 12, 2022

pierrecregut commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Jun 2, 2022

pierrecregut commented May 6, 2022 •

edited by crenshaw-dev

codecov bot commented May 6, 2022 •

edited

pierrecregut May 12, 2022 •

edited