Added AWS Credentials Support for Scanning Private Registry (ECR) by VF-mbrauer · Pull Request #1103 · aquasecurity/starboard

VF-mbrauer · 2022-04-04T08:28:20Z

Added support for scanning the private registry of AWS-ECR.
Feature still in Beta-mode but usable when prerequisites are met.

Prerequisite:
K8S-Kiam or any other method IRSA (Future) to be used to allow Assuming Instance Roles.

Configurations:
Annotate the Starboard Namespace to be allowed to use KIAM:
Annotations: iam.amazonaws.com/permitted: .*

Add the Role you want to Assume which has the proper right to create ECR-Credentials

podAnnotations: 
    # iam.amazonaws.com/role: <yourRolewithPermissions>

Configuration in HELM value file to enable the feature
trivy.useEcrRoleCreds: false

pschulten · 2022-04-08T14:13:59Z

@VF-mbrauer there's an issue with your if config.UseECRCredentials guard, the bracketing is wrong. If the below patch is applied, it works flawlessly

Index: pkg/plugin/trivy/plugin.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pkg/plugin/trivy/plugin.go b/pkg/plugin/trivy/plugin.go
--- a/pkg/plugin/trivy/plugin.go	(revision 1411d127246e8fe9e856dd436cdcee821ce1f1af)
+++ b/pkg/plugin/trivy/plugin.go	(date 1649427066287)
@@ -624,7 +624,7 @@
 				}
 			}
 
-			var creds (ecr_credentials) = ecr_credentials{aws_creds[0][1], aws_creds[0][2]}
+			var creds = ecr_credentials{aws_creds[0][1], aws_creds[0][2]}
 
 			env = append(env, corev1.EnvVar{
 				Name:  "TRIVY_USERNAME",
@@ -953,90 +953,90 @@
 					},
 				})
 			}
+		}
 
-			env, err = p.appendTrivyInsecureEnv(config, container.Image, env)
-			if err != nil {
-				return corev1.PodSpec{}, nil, err
-			}
+		env, err = p.appendTrivyInsecureEnv(config, container.Image, env)
+		if err != nil {
+			return corev1.PodSpec{}, nil, err
+		}
 
-			env, err = p.appendTrivyNonSSLEnv(config, container.Image, env)
-			if err != nil {
-				return corev1.PodSpec{}, nil, err
-			}
+		env, err = p.appendTrivyNonSSLEnv(config, container.Image, env)
+		if err != nil {
+			return corev1.PodSpec{}, nil, err
+		}
 
-			if config.GetServerInsecure() {
-				env = append(env, corev1.EnvVar{
-					Name:  "TRIVY_INSECURE",
-					Value: "true",
-				})
-			}
+		if config.GetServerInsecure() {
+			env = append(env, corev1.EnvVar{
+				Name:  "TRIVY_INSECURE",
+				Value: "true",
+			})
+		}
 
-			if config.IgnoreFileExists() {
-				volumes = []corev1.Volume{
-					{
-						Name: ignoreFileVolumeName,
-						VolumeSource: corev1.VolumeSource{
-							ConfigMap: &corev1.ConfigMapVolumeSource{
-								LocalObjectReference: corev1.LocalObjectReference{
-									Name: trivyConfigName,
-								},
-								Items: []corev1.KeyToPath{
-									{
-										Key:  keyTrivyIgnoreFile,
-										Path: ".trivyignore",
-									},
-								},
-							},
-						},
-					},
-				}
+		if config.IgnoreFileExists() {
+			volumes = []corev1.Volume{
+				{
+					Name: ignoreFileVolumeName,
+					VolumeSource: corev1.VolumeSource{
+						ConfigMap: &corev1.ConfigMapVolumeSource{
+							LocalObjectReference: corev1.LocalObjectReference{
+								Name: trivyConfigName,
+							},
+							Items: []corev1.KeyToPath{
+								{
+									Key:  keyTrivyIgnoreFile,
+									Path: ".trivyignore",
+								},
+							},
+						},
+					},
+				},
+			}
 
-				volumeMounts = []corev1.VolumeMount{
-					{
-						Name:      ignoreFileVolumeName,
-						MountPath: "/etc/trivy/.trivyignore",
-						SubPath:   ".trivyignore",
-					},
-				}
+			volumeMounts = []corev1.VolumeMount{
+				{
+					Name:      ignoreFileVolumeName,
+					MountPath: "/etc/trivy/.trivyignore",
+					SubPath:   ".trivyignore",
+				},
+			}
 
-				env = append(env, corev1.EnvVar{
-					Name:  "TRIVY_IGNOREFILE",
-					Value: "/etc/trivy/.trivyignore",
-				})
-			}
+			env = append(env, corev1.EnvVar{
+				Name:  "TRIVY_IGNOREFILE",
+				Value: "/etc/trivy/.trivyignore",
+			})
+		}
 
-			requirements, err := config.GetResourceRequirements()
-			if err != nil {
-				return corev1.PodSpec{}, nil, err
-			}
+		requirements, err := config.GetResourceRequirements()
+		if err != nil {
+			return corev1.PodSpec{}, nil, err
+		}
 
-			optionalMirroredImage, err := GetMirroredImage(container.Image, config.GetMirrors())
-			if err != nil {
-				return corev1.PodSpec{}, nil, err
-			}
+		optionalMirroredImage, err := GetMirroredImage(container.Image, config.GetMirrors())
+		if err != nil {
+			return corev1.PodSpec{}, nil, err
+		}
 
-			containers = append(containers, corev1.Container{
-				Name:                     container.Name,
-				Image:                    trivyImageRef,
-				ImagePullPolicy:          corev1.PullIfNotPresent,
-				TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
-				Env:                      env,
-				Command: []string{
-					"trivy",
-				},
-				Args: []string{
-					"--quiet",
-					"client",
-					"--format",
-					"json",
-					"--remote",
-					trivyServerURL,
-					optionalMirroredImage,
-				},
-				VolumeMounts: volumeMounts,
-				Resources:    requirements,
-			})
-		}
+		containers = append(containers, corev1.Container{
+			Name:                     container.Name,
+			Image:                    trivyImageRef,
+			ImagePullPolicy:          corev1.PullIfNotPresent,
+			TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
+			Env:                      env,
+			Command: []string{
+				"trivy",
+			},
+			Args: []string{
+				"--quiet",
+				"client",
+				"--format",
+				"json",
+				"--remote",
+				trivyServerURL,
+				optionalMirroredImage,
+			},
+			VolumeMounts: volumeMounts,
+			Resources:    requirements,
+		})
 	}
 
 	return corev1.PodSpec{

VF-mbrauer · 2022-04-08T15:45:42Z

@pschulten, great catch. If have removed the 2 Redundant parentheses as they are not needed. And for the brackets, I fixed that in getPodSpecForClientServerMode. I simply forgot that somehow in this ClientServerMode but I did it correctly in the other area like getPodSpecForStandaloneMode. Therefore, my E2E test was OK at that time.

I have corrected that already and committed d361d7f
Thanks again

chen-keinan · 2022-04-13T12:00:23Z

+		} else {
+			errormsg = err.Error()
+		}
+		return nil, errors.New((errormsg))


Suggested change

return nil, errors.New((errormsg))

return nil, errors.New(errormsg)

Done and pushed.

chen-keinan · 2022-04-13T12:05:47Z

+				}
+			}
+
+			var creds (ecr_credentials) = ecr_credentials{aws_creds[0][1], aws_creds[0][2]}


Suggested change

var creds (ecr_credentials) = ecr_credentials{aws_creds[0][1], aws_creds[0][2]}

var creds = ecr_credentials{aws_creds[0][1], aws_creds[0][2]}

Done and pushed.

chen-keinan · 2022-04-17T08:34:28Z

+
+		if config.UseECRCredentials() && CheckAwsEcrPrivateRegistry(container.Image) != "" {
+
+			if utils.TokenTTLValidation(EcrTokenGen, TTLResult) {


I think its better not to have EcrTokenGen and aws_creds as global variables, I would, I would create ECRRegistry struct as a property of plugin which will encapsulate both properties (EcrTokenGen and aws_creds)

Agreed and fixed.

chen-keinan · 2022-04-17T08:35:03Z

+		if config.UseECRCredentials() && CheckAwsEcrPrivateRegistry(container.Image) != "" {
+
+			if utils.TokenTTLValidation(EcrTokenGen, TTLResult) {
+				EcrTokenGen = time.Now()


Suggested change

EcrTokenGen = time.Now()

EcrTokenGen = p.clock.Now()

Agreed and fixed.

chen-keinan · 2022-04-17T09:20:56Z

+  # AWS authorization token is valid for 12 hours by default
+  # Unit is hours(h)
+  #
+  # ecrTokenRefreshTTL = 11h


ecrTokenRefreshTTL has to be shorter then 12h? as Trivy might want to authenticate with an expired credential

Also, maybe a retry logic should be placed for GetAuthorizationToken in case tokens are expired unless we force refresh before 12h

No, Trivy will never use an expired token as in the original part I pushed, the Token creation was part of "each" individual request Trivy did per repository. The only reason we have this timer now in place is to reduce the number of requests (Throttling) against the ECR-API.

So, if you want to detect an expired token and you want to make it part of a retry to refresh the token now you need to get the first trivy job triggered which tries to get the image and will run into the error.
In this case, a Token Refresh needs to be triggered. But this is a completely new scenario, which needs to be plugged into the trivy process in combination with the PlugIn routines.
Also, a drawback and a negative aspect are here is, that each request needs to be sent towards API to check if still valid or not. But this will be contradicting the Throttling which I wanted to solve.

As of now, I see no point in that we need that. The Throttling does its job, and it is up to the implementer to configure the TTLs on their own. In my view, this is the best approach.

chen-keinan · 2022-04-17T09:34:53Z

+				EcrTokenGen = time.Now()
+				aws_creds, err = GetAuthorizationToken(CheckAwsEcrPrivateRegistry(container.Image))
+				if err != nil {
+					return corev1.PodSpec{}, nil, err


EcrTokenGen should be set to p.clock.Now() only if the GetAuthorizationToken returns no error. to avoid index out of range error in the next authorization cycle as aws_creds will be nil (GetAuthorizationToken will not be called again until TTL expire)

suggestions:

aws_creds, err = GetAuthorizationToken(CheckAwsEcrPrivateRegistry(container.Image)) if err != nil { return corev1.PodSpec{}, nil, err } EcrTokenGen = p.clock.Now()

Agreed and fixed.

…al Variables and put it into struct. Changed Session NEW deprecation.

chen-keinan · 2022-04-26T18:17:34Z

 # follow Semantic Versioning. They should reflect the version the application is using.
-appVersion: 0.15.0
+appVersion: 0.15.1



this file should be restored as it was , we only change versions when we release

Changed back to normal.

Back to normal before the change as this is part of the release process which will change this values.

danielpacak

I had a closer look at this PR from the integration stand point and it feels to me overly complex and not easy to maintain. Especially that it requires kiam, which is not actively maintained anymore, and even kiam maintainers suggested switching over to IAM roles for service accounts some time ago.

Besides, my concern is that we cannot provide a real support in case of problems with such implementation, because proposed solution requires configuring Starboard in many places: Trivy plugin config, custom annotation added to each scan job, annotation added the starboard-operator namespace, and IAM Roles.

I missed high level explanation how this solution works, but after spending some time reverse engineering the source code I realized it does the following:

Retrieve an ECR authorization token based on AWS environment variables that kiam injects by looking at the custom iam.amazonaws.com/role annotation added to each scan job.
Pass the authorization token to Trivy with TRIVY_USERNAME=AWS and TRIVY_PASSWROD=<token> environment variables:
```
TRIVY_USERNAME="AWS" \
TRIVY_PASSWORD="<token>" \
trivy image <image ref>
```

However, we can accomplish the same by passing AWS environment variables directly to Trivy, which will exchange them for authorization token to pull down an image before scan:

AWS_ACCESS_KEY_ID=*** \
AWS_SECRET_ACCESS_KEY=*** \
AWS_DEFAULT_REGION=eu-central-1 \
trivy image <image ref>

This would not require kiam or any solution like that. The same capability is actually used when you deploy Starboard in EKS as explained on
https://aquasecurity.github.io/starboard/v0.15.4/vulnerability-scanning/managed-registries/#amazon-elastic-container-registry-ecr

Having said that, I would think of a solution that does not require installing and documenting any additional software that's not actively maintained. Ideally Starboard should be self-contained and self-sufficient to perform scans without overly complex configuration.

Notice also, that we've been experimenting with Trivy filesystem scanner to avoid cloud privider-specifi code in Starboard and scan without exchanging credentials. Check our design docs in this repo for more details.

pschulten · 2022-05-13T17:37:17Z

@danielpacak I very much share your reluctance to introduce complexity, but using the normal aws sdk credential auth flow for ECR pulls would be very helpful for people running non-EKS cluster (like us 😊)

This branch works great for us using our standard node role via the instance profile flow.
It would also work with any other mechanism like kube2iam (which isn't unmaintained) or a custom HashiCorp vault backed credential_process, etc.

Ideally we would like to run in trivy.mode=ClientServer and the server just receives the image URL, scans the image in a central place (and benefits a lot by container image layer caching) and sends the response report back to starboard, but I guess this would be a major architectural refactoring.

VF-mbrauer · 2023-04-02T13:22:09Z

@chen-keinan I guess we can close? Do we?

Added support for AWS Private Registry (ECR)

b875dc8

This was referenced Apr 4, 2022

Added AWS Credentials Support for Scanning Private Registry #1062

Closed

Support private container registries for Pods which do not specifying image pull secrets #120

Open

VF-mbrauer force-pushed the ecr_cred_support branch from 19e1523 to 265aac9 Compare April 6, 2022 08:50

AWS-API Throttling feature implemented.

1411d12

VF-mbrauer force-pushed the ecr_cred_support branch from 265aac9 to 1411d12 Compare April 6, 2022 08:52

Corrected Redundant parentheses and wrong bracket closings

d361d7f

danielpacak self-requested a review April 13, 2022 06:52

chen-keinan self-requested a review April 13, 2022 07:27

chen-keinan reviewed Apr 13, 2022

View reviewed changes

Comment thread pkg/utils/durationutil.go Outdated

Comment thread pkg/utils/durationutil.go Outdated

Comment thread pkg/plugin/trivy/plugin.go Outdated

Comment thread pkg/plugin/trivy/plugin.go Outdated

Merged Date function in existing go-file. Adapted error handlings.

23bce93

VF-mbrauer requested a review from chen-keinan April 13, 2022 08:56

chen-keinan reviewed Apr 13, 2022

View reviewed changes

Comment thread deploy/helm/Chart.yaml

chen-keinan reviewed Apr 13, 2022

View reviewed changes

Unnecessary parenthesis removed.

58fb2c0

VF-mbrauer requested a review from chen-keinan April 13, 2022 12:56

chen-keinan suggested changes Apr 17, 2022

View reviewed changes

chen-keinan reviewed Apr 17, 2022

View reviewed changes

Set EcrTokenGen after successfull GetAuthorizationToken. Removed glob…

bc24ddd

…al Variables and put it into struct. Changed Session NEW deprecation.

VF-mbrauer requested a review from chen-keinan April 18, 2022 21:12

danielpacak removed their request for review April 22, 2022 09:08

chen-keinan reviewed Apr 26, 2022

View reviewed changes

Update Chart.yaml

10768d2

Back to normal before the change as this is part of the release process which will change this values.

VF-mbrauer requested a review from chen-keinan April 28, 2022 08:00

danielpacak self-requested a review May 11, 2022 08:05

danielpacak reviewed May 11, 2022

View reviewed changes

Comment thread deploy/helm/Chart.yaml Outdated

	return nil, errors.New((errormsg))
	return nil, errors.New(errormsg)

	var creds (ecr_credentials) = ecr_credentials{aws_creds[0][1], aws_creds[0][2]}
	var creds = ecr_credentials{aws_creds[0][1], aws_creds[0][2]}


		if config.UseECRCredentials() && CheckAwsEcrPrivateRegistry(container.Image) != "" {

		if utils.TokenTTLValidation(EcrTokenGen, TTLResult) {

Uh oh!

Conversation

VF-mbrauer commented Apr 4, 2022

Uh oh!

pschulten commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VF-mbrauer commented Apr 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chen-keinan Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chen-keinan Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielpacak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pschulten commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VF-mbrauer commented Apr 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pschulten commented Apr 8, 2022 •

edited

Loading

chen-keinan Apr 17, 2022 •

edited

Loading

chen-keinan Apr 17, 2022 •

edited

Loading

danielpacak left a comment •

edited

Loading

pschulten commented May 13, 2022 •

edited

Loading