Secure env vars deployed together with the plain ones as clear text

[prein]

Not sure if I understand what is happening in the following piece:

aci-deploy/lib/taskparameters.js

Line 33 in bcb0a1c

this._getEnvironmentVariables(environmentVariables, secureEnvironmentVariables);

but it looks like the secure is mixed together with insecure?

This can also be observed if I use Terraform azurerm_container_group resource to deploy the same container group instance and have both environment_variables and secure_environment_variables defined in both places.

For example, if my github workflow goes like this:

    [...]
    - name: Deploy to Azure Container Instances
      uses: azure/aci-deploy@v1
      with:
        [...]
        environment-variables: FOO = "bar"
        secure-environment-variables: BAR = "foo"

And my TF goes like this

resource "azurerm_container_group" "containergroup" {

   [...]
   container {
     [...]
     environment_variables = {
      FOO = "bar"
     }
     secure_environment_variables = {
      BAR = "foo"
     }
  }
}

Then terraform plan will find BAR among clear text variables

          ~ environment_variables        = { # forces replacement
              - "BAR"  = "foo" -> null
            }

One can also confirm the secure env vars are stored as clear text using az container show or in the azure portal
Example

$ az container show -g my-group --name my-container | jq .containers[].environmentVariables
[
  {
    "name": "BAR",
    "secureValue": null,
    "value": "foo"
  },

[prein]

Just realized I created dup to #42
Feel free to merge.

[dbrooks5]

The az cli supports two forms of environment variables for azure container instances.
--secure-environment-variables - all variables specified in this block have their values obfuscated in the azure portal so they are not visible and cannot be retrieved via the cli. This functionality is useful for things like passwords.
--environment-variables - all variables specified here have their values visible in the azure portal and cli.

It appears that the github action is grouping both flags together and just treating them all as environment variables.

In the taskparameters.ts file it appears that the two flags are being processed independently, but the processes for both is the same.

private _getEnvironmentVariables(environmentVariables: string, secureEnvironmentVariables: string) {
	if(environmentVariables) {
		// split on whitespace, but ignore the ones that are enclosed in quotes
		let keyValuePairs = environmentVariables.match(/(?:[^\s"]+|"[^"]*")+/g) || [];
		keyValuePairs.forEach((pair: string) => {
			// value is either wrapped in quotes or not
			let pairList = pair.split(/=(?:"(.+)"|(.+))/);
			let obj: ContainerInstanceManagementModels.EnvironmentVariable = { 
				"name": pairList[0], 
				"value": pairList[1] || pairList[2]
			};
			this._environmentVariables.push(obj);
		})
	}
	if(secureEnvironmentVariables) {
		// split on whitespace, but ignore the ones that are enclosed in quotes
		let keyValuePairs = secureEnvironmentVariables.match(/(?:[^\s"]+|"[^"]*")+/g) || [];
		keyValuePairs.forEach((pair: string) => {
			// value is either wrapped in quotes or not
			let pairList = pair.split(/=(?:"(.+)"|(.+))/);
			let obj: ContainerInstanceManagementModels.EnvironmentVariable = { 
				"name": pairList[0], 
				"value": pairList[1] || pairList[2]
			};
			this._environmentVariables.push(obj);
		})
	}
}

[github-actions]

This issue is marked need-to-triage for generating issues report.

[kanika1894]

@prein @dbrooks5 Is there anything that we can help you with?
(Seems like you have figured out something on your own :) )

[el-pato]

@kanika1894 @prein @dbrooks5 fix should be to usesecureValue instead of value when adding secure environment variables to _environmentVariables, per the snippet below. I tested this on one of my projects and confirm that secure environment variables are no longer visible in the properties of the the container instance. Also created a PR: #51

Problem code:

aci-deploy/src/taskparameters.ts

Lines 154 to 166 in abb2c5f

	if(secureEnvironmentVariables) {
	// split on whitespace, but ignore the ones that are enclosed in quotes
	let keyValuePairs = secureEnvironmentVariables.match(/(?:[^\s"]+|"[^"]*")+/g) || [];
	keyValuePairs.forEach((pair: string) => {
	// value is either wrapped in quotes or not
	let pairList = pair.split(/=(?:"(.+)"|(.+))/);
	let obj: ContainerInstanceManagementModels.EnvironmentVariable = {
	"name": pairList[0],
	"value": pairList[1] || pairList[2]
	};
	this._environmentVariables.push(obj);
	})
	}

Potential fix:

if(secureEnvironmentVariables) {
            // split on whitespace, but ignore the ones that are enclosed in quotes
            let keyValuePairs = secureEnvironmentVariables.match(/(?:[^\s"]+|"[^"]*")+/g) || [];
            keyValuePairs.forEach((pair: string) => {
                // value is either wrapped in quotes or not
                let pairList = pair.split(/=(?:"(.+)"|(.+))/);
                let obj: ContainerInstanceManagementModels.EnvironmentVariable = { 
                    "name": pairList[0], 
                    "secureValue": pairList[1] || pairList[2]
                };
                this._environmentVariables.push(obj);
            })

[kanika1894]

Fixed by the PR : #51

[sroebert]

It is fixed in the source ts file, but not in the lib js file, so when using it in Github actions secure environment variables are still passed on as normal environment variables.

[el-pato]

@sroebert yeah, this project needs to be built again (and preferably a new release). More info in #57