fix(eks): malformed command when installing helm chart from OCI artifact

[tsahiduek]

When using helm to pull OCI artifacts, the helm pull command doesn't works well.

The check_output uses shell=True. That means that all arguments of the commands being passed to the check_output are basically been passed to the shell and not to the helm pull command. Using shell is also discouraged from security perspective
References:
https://docs.python.org/3/library/subprocess.html

On POSIX with shell=True, the shell defaults to /bin/sh. If args is a string, the string specifies the command to execute through the shell. This means that the string must be formatted exactly as it would be when typed at the shell prompt. This includes, for example, quoting or backslash escaping filenames with spaces in them. If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself.

https://stackoverflow.com/questions/3172470/actual-meaning-of-shell-true-in-subprocess

The previous change that used the Shell=True was introduced in commit - #18547 (comment)

EDIT:
Adding commit for the following items:

• Adding integration test for helm OCI support in aws-eks
• Upgrading helm version to 3.8.1 in aws-lambda-layer because of issues with the current version of helm that is been used, for OCI chart supports
• update integ.eks-helm-asset.expected.json file

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here - NO new unconventional dependencies

New Features

• Have you added the new feature to an integration test? - NO
  • Did you use cdk-integ to deploy the infrastructure and generate the snapshot (i.e. cdk-integ without --dry-run)? - NO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 5641b66
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[tsahiduek]

Forgot to put a link about why Helm needs to be upgraded in the lambda layer.
This is an example from another project that demonstrated the differences between Helm 3.7 and above vs 3.6 and below

Basically the commands are different (the helm handler uses helm pull oci://$repo/$chart that works only at 3.7 and above)
Also in https://helm.sh/docs/topics/registries/#deprecated-features-and-strict-naming-policies

[iliapolo]

Awesome, thanks!

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[harshadbhatia]

@tsahiduek , Can you please highlight what exactly didn't work when you raised this PR. If you can include repository address would be great.

Now couple of points why shell=True was introduced. The command does login and passes token which allows helm pull to execute nicely. If you are to do it any other way, it is painful and just does not work nicely with pipe and stdout. Happy for you to put PR. Now in terms of security consideration.

Potential issues can really with some sort of shell injection. Now if we were to put it in perspective.

1. Lambda is created by cloudformation via aws-cdk.
2. Lambda is only invoked by an event.
3. And as far as command is concerned there is no other input associated it. Unless suddenly aws decides to send malicious code to themselves.

So i am unable to see how this poses a security risk. Curious to know if there is another scenario.

Now this code from previous PR was well tested by me with various AWS accounts. Now this change has caused regression as you can see from other users as well.

#20402

[tsahiduek]

Hi @harshadbhatia ,
As I described in my comment, the use of shell=True made the command not work at all (tested several times with @iliapolo). The use case was to install an OCI helm chart from the public ECR gallery (specifically ACK chart).

As described as well on my previous comment, it's not me that point out the security consideration of the shell=True usage.

The way that the code was written before removing the shell, was that all the parameters to the helm command, were actually sent to the shell and no the the helm command. That means that for the cmnd = ['HELM_EXPERIMENTAL_OCI=1', 'helm', 'pull', repository, '--version', version, '--untar'], the only command that was executed was HELM_EXPERIMENTAL_OCI=1, and all other arguments ('helm', 'pull', repository, '--version', version, '--untar') were actually sent to the shell as an arguments, resulting with a command that does nothing

If you are willing to put the shell=True back on, you have to make sure that the helm command is a single string concatenation, and not a list of arguments. This is being built in this line

Regarding your statement about tests, this PR tested several times before and after the change, and it was clear that before this PR, the feature was not working at all. I even added E2E tests to verify it.

[harshadbhatia]

@tsahiduek , so I looked at the code, technically what you are saying would never work. There is a if-else statement which would fail on pattern match, defaulting to helm pull, which explains why as public ecr was never implemented at least on my end.

Are you able to provide a sample chart location along with version please ?

Yeah, so those tests are kinda useless tbh, they don't actually use lambda to do installation. So you would have to manually test those.

[tsahiduek]

I don't know why you call them useless. They actually perform the actual installation, and looks for the expected result. I believe that @iliapolo can help better explain why those tests are important.

Anyway, you can look at any of the helm OCI installation in this sample

[hbhatia-demyst]

Probably useless was little harsh !

So they do test the cloudformation creation, but from my memory I do not think they test whether the installation of the chart actually succeed. Which explains why we regression is never picked in current tests. Happy to contribute if someone explains how to run those test.

Thanks for the info. I will push the PR shortly.

[tsahiduek]

I called it E2E tests, but it actually being referred to as integration tests.
The Contribution doc describe how to write and run them.

Please make sure in your PR that the chart example I put on my previous comment is working, as we already have content sample relying on that.

Thank you

[hbhatia-demyst]

@tsahiduek , have tested this manually with public and private ecr and this should work. PR here: #20724