Verify bundle signature is valid and is not modified #9209

panktishah26 · 2025-01-29T03:12:09Z

Issue #, if available:
#6793

Description of changes:
Validate the EKS-A bundle has not been modified by verifying the signature in the bundle annotation. We sign the bundle by adding signature annotation on the bundle field. To verify the bundle, we have used ecdsa.VerifyASN1 method to support air gapped environment scenario. The validation is only added for controller for now.

Testing (if applicable):

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

panktishah26 · 2025-01-29T19:19:07Z

release/cli/pkg/constants/constants.go

@@ -62,13 +62,4 @@ const (

 	// KMS key alias
 	KmsKey = "arn:aws:kms:us-west-2:857151390494:alias/signingEKSABundlesKey"
-


I have moved these constants to eks-anywhere constants package since these are used for both signing and verification purpose and to keep a single source of truth.

codecov · 2025-01-29T19:50:53Z

Codecov Report

Attention: Patch coverage is 74.60317% with 32 lines in your changes missing coverage. Please review.

Project coverage is 72.38%. Comparing base (5583d64) to head (fe12c2c).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/signature/manifest.go	68.47%	20 Missing and 9 partials ⚠️
pkg/validations/extendedversion.go	80.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #9209    +/-   ##
========================================
  Coverage   72.37%   72.38%            
========================================
  Files         585      587     +2     
  Lines       45710    45836   +126     
========================================
+ Hits        33084    33178    +94     
- Misses      10888    10910    +22     
- Partials     1738     1748    +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

panktishah26 · 2025-01-29T21:35:51Z

pkg/validations/extendedversion.go

+)
+
+// ValidateExtendedK8sVersionSupport validates all the validations needed for the support of extended kubernetes support.
+func ValidateExtendedK8sVersionSupport(_ context.Context, _ *anywherev1.Cluster, bundle *v1alpha1.Bundles, _ kubernetes.Client) error {


I am not using context, cluster and client field in this PR but I will need those for further extended support related validations.

controllers/cluster_controller_test.go

jacobweinstock · 2025-01-29T21:34:21Z

pkg/constants/constants.go

+	// fields depending on your signing requirements.
+	Excludes = "LnNwZWMudmVyc2lvbnNCdW5kbGVzW10uYm9vdHN0cmFwCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmJvdHRsZXJvY2tldEhvc3RDb250YWluZXJzCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmNlcnRNYW5hZ2VyCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmNpbGl1bQouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS5jbG91ZFN0YWNrCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmNsdXN0ZXJBUEkKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uY29udHJvbFBsYW5lCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmRvY2tlcgouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS5la3NhCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmV0Y2RhZG1Cb290c3RyYXAKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uZXRjZGFkbUNvbnRyb2xsZXIKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uZmx1eAouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS5oYXByb3h5Ci5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmtpbmRuZXRkCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLm51dGFuaXgKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10ucGFja2FnZUNvbnRyb2xsZXIKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uc25vdwouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS50aW5rZXJiZWxsCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLnVwZ3JhZGVyCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLnZTcGhlcmU="
+	// KMSPublicKey to verify bundle signature.
+	KMSPublicKey = "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEFZU/Z6VVMU9HioT7rGkPdJg3frC2xyQZhWFIrz5HeZEfeQ2nAdnJMLrs2Qr3V9xVrJrHA54wnIHDoPGbEhojqg=="


from where did this value come? how is it produced?

This is the public key of the KMS key that we use to sign the bundle.

I know public keys are meant to be public, but should we be putting this out in the open?

https://github.com/aws/eks-anywhere-packages/blob/main/charts/eks-anywhere-packages/values.yaml#L93

We have it in our packages repo too for verifying package bundle signature in the webhook.

pkg/signature/manifest.go

jacobweinstock · 2025-01-29T21:43:51Z

pkg/signature/manifest.go

+
+	pubparsed, err := x509.ParsePKIXPublicKey(pubdecoded)
+	if err != nil {
+		return false, digest, yml, fmt.Errorf("unable parse the public key (not PKIX): %v", err)


might think about this: %v -> %w.

"Wrap an error to expose it to callers. Do not wrap an error when doing so would expose implementation details."
reference: https://go.dev/blog/go1.13-errors#wrapping-errors-with-w

pkg/signature/manifest.go

jacobweinstock · 2025-01-29T21:53:43Z

pkg/validations/extendedversion.go

+	if !valid {
+		return fmt.Errorf("signature on the bundle is invalid, error: %v, digest: %s", err, base64.StdEncoding.EncodeToString(digest[:]))
+	}
+	logger.Info("Signature on the bundle is valid.")


i recommend not logging in a function in a package like this. I recommend letting consumers/callers of the function do the logging. Also, this this looks like a global logger. Where is it even defined?

Agree, I added it as a part of my debugging and forgot to remove. I have removed this log.

vivek-koppuru · 2025-01-30T20:15:21Z

controllers/cluster_controller.go

+func validateExtendedK8sVersionSupport(ctx context.Context, client client.Client, cluster *anywherev1.Cluster) error {
+	bundle, err := c.BundlesForCluster(ctx, clientutil.NewKubeClient(client), cluster)
+	if err != nil {
+		return fmt.Errorf("failed to get bundle for cluster: %w", err)


We generally want to avoid saying error or failed again right?

Suggested change

return fmt.Errorf("failed to get bundle for cluster: %w", err)

return fmt.Errorf("getting bundle for cluster: %w", err)

vivek-koppuru · 2025-01-30T20:20:27Z

pkg/constants/constants.go

+	// Excludes is a base64-encoded, newline-delimited list of JSON/YAML paths to remove
+	// from the Bundles manifest prior to computing the digest. You can add or remove
+	// fields depending on your signing requirements.
+	Excludes = "LnNwZWMudmVyc2lvbnNCdW5kbGVzW10uYm9vdHN0cmFwCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmJvdHRsZXJvY2tldEhvc3RDb250YWluZXJzCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmNlcnRNYW5hZ2VyCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmNpbGl1bQouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS5jbG91ZFN0YWNrCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmNsdXN0ZXJBUEkKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uY29udHJvbFBsYW5lCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmRvY2tlcgouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS5la3NhCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmV0Y2RhZG1Cb290c3RyYXAKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uZXRjZGFkbUNvbnRyb2xsZXIKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uZmx1eAouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS5oYXByb3h5Ci5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLmtpbmRuZXRkCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLm51dGFuaXgKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10ucGFja2FnZUNvbnRyb2xsZXIKLnNwZWMudmVyc2lvbnNCdW5kbGVzW10uc25vdwouc3BlYy52ZXJzaW9uc0J1bmRsZXNbXS50aW5rZXJiZWxsCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLnVwZ3JhZGVyCi5zcGVjLnZlcnNpb25zQnVuZGxlc1tdLnZTcGhlcmU="


How did we determine what to remove here?

https://quip-amazon.com/cUORAQaTboBZ/Extended-Kubernetes-version-support-for-EKS-Anywhere#temp:C:ceKc799977659fd44e0916fb1899

We are only concerned with the tampering of the above fields for extended support. In the future, if we need to include more fields in the bundle signature for particular usecase, it can be easily added by removing it from the Excludes annotation here.

vivek-koppuru · 2025-01-30T20:21:25Z

pkg/signature/manifest.go

+
+	pubdecoded, err := base64.StdEncoding.DecodeString(pubKey)
+	if err != nil {
+		return false, fmt.Errorf("unable to decode the public key (not base 64): %w", err)


Yea these should be phrased as such right?

Suggested change

return false, fmt.Errorf("unable to decode the public key (not base 64): %w", err)

return false, fmt.Errorf("decoding the public key as string: %w", err)

vivek-koppuru · 2025-01-30T20:21:49Z

pkg/signature/manifest.go

+	// Marshal Bundles object to YAML
+	yamlBytes, err := yaml.Marshal(bundle)
+	if err != nil {
+		return zero, nil, fmt.Errorf("marshalling bundle to YAML: %w", err)


Yea this is a right error message

vivek-koppuru · 2025-01-30T20:26:52Z

controllers/cluster_controller.go

@@ -389,6 +389,10 @@ func (r *ClusterReconciler) preClusterProviderReconcile(ctx context.Context, log
 		return controller.Result{}, err
 	}

+	if err := validateExtendedK8sVersionSupport(ctx, r.client, cluster); err != nil {


Are we planning to update the cluster status in any way, or this will just be reflected as an error in the controller logs?

I have updated the status fields with error.

panktishah26 · 2025-01-31T00:51:27Z

/test eks-anywhere-release-tooling-test-presubmit

vivek-koppuru · 2025-01-31T22:26:39Z

pkg/api/v1alpha1/cluster_types.go

+	BundleNotFoundReason FailureReasonType = "BundleNotFoundForCluster"
+
+	// K8sVersionNotSupportedReason reports that validation for supporting extended kubernetes version failed.
+	K8sVersionNotSupportedReason FailureReasonType = "ExtendedKubernetesVersionNotSupported"


Suggested change

K8sVersionNotSupportedReason FailureReasonType = "ExtendedKubernetesVersionNotSupported"

ExtendedK8sVersionSupportNotSupportedReason FailureReasonType = "ExtendedKubernetesVersionSupportNotSupported"

vivek-koppuru · 2025-01-31T22:27:55Z

pkg/signature/manifets_test.go

@@ -0,0 +1,338 @@
+package signature


Can you rename this file to fix spelling?

sp1999 · 2025-01-31T23:22:27Z

/lgtm

eks-distro-bot · 2025-02-01T05:53:08Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 29, 2025

panktishah26 force-pushed the extended-k8s-bundle-validation branch from e17bb84 to 486d237 Compare January 29, 2025 19:16

panktishah26 commented Jan 29, 2025

View reviewed changes

panktishah26 force-pushed the extended-k8s-bundle-validation branch from 486d237 to 4c86845 Compare January 29, 2025 19:45

eks-distro-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 29, 2025

panktishah26 force-pushed the extended-k8s-bundle-validation branch from 4c86845 to 4b17c77 Compare January 29, 2025 20:33

panktishah26 commented Jan 29, 2025

View reviewed changes

panktishah26 force-pushed the extended-k8s-bundle-validation branch from 4b17c77 to 8506072 Compare January 29, 2025 21:41

jacobweinstock reviewed Jan 29, 2025

View reviewed changes

panktishah26 force-pushed the extended-k8s-bundle-validation branch from 8506072 to b67e52f Compare January 29, 2025 23:20

jacobweinstock approved these changes Jan 29, 2025

View reviewed changes

eks-distro-bot assigned jacobweinstock Jan 29, 2025

eks-distro-bot added the lgtm label Jan 29, 2025

panktishah26 force-pushed the extended-k8s-bundle-validation branch from b67e52f to 83cf0f2 Compare January 29, 2025 23:42

eks-distro-bot removed the lgtm label Jan 29, 2025

jacobweinstock approved these changes Jan 29, 2025

View reviewed changes

eks-distro-bot added the lgtm label Jan 29, 2025

vivek-koppuru reviewed Jan 30, 2025

View reviewed changes

panktishah26 force-pushed the extended-k8s-bundle-validation branch from 83cf0f2 to 97804a2 Compare January 31, 2025 00:41

eks-distro-bot removed the lgtm label Jan 31, 2025

panktishah26 force-pushed the extended-k8s-bundle-validation branch from 97804a2 to 7cdc7cb Compare January 31, 2025 00:43

panktishah26 force-pushed the extended-k8s-bundle-validation branch 3 times, most recently from fb5465a to da9a9dd Compare January 31, 2025 20:46

vivek-koppuru reviewed Jan 31, 2025

View reviewed changes

Verify bundle signature is valid and is not modified

cc64819

Verify bundle signature is valid and is not modified

fe12c2c

eks-distro-bot assigned sp1999 Jan 31, 2025

eks-distro-bot added the lgtm label Jan 31, 2025

panktishah26 force-pushed the extended-k8s-bundle-validation branch from da9a9dd to fe12c2c Compare January 31, 2025 23:27

eks-distro-bot removed the lgtm label Jan 31, 2025

vivek-koppuru approved these changes Feb 1, 2025

View reviewed changes

eks-distro-bot assigned vivek-koppuru Feb 1, 2025

eks-distro-bot added the lgtm label Feb 1, 2025

panktishah26 added the approved label Feb 1, 2025

panktishah26 merged commit 26f5eab into aws:main Feb 1, 2025
13 of 15 checks passed

panktishah26 mentioned this pull request Feb 6, 2025

Move signature package to eks-anywhere #9204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify bundle signature is valid and is not modified #9209

Verify bundle signature is valid and is not modified #9209

panktishah26 commented Jan 29, 2025 •

edited

Loading

panktishah26 Jan 29, 2025

codecov bot commented Jan 29, 2025 •

edited

Loading

panktishah26 Jan 29, 2025

jacobweinstock Jan 29, 2025

panktishah26 Jan 29, 2025 •

edited

Loading

vivek-koppuru Jan 30, 2025

sp1999 Jan 31, 2025

jacobweinstock Jan 29, 2025

jacobweinstock Jan 29, 2025

panktishah26 Jan 29, 2025

vivek-koppuru Jan 30, 2025

vivek-koppuru Jan 30, 2025

sp1999 Jan 31, 2025

vivek-koppuru Jan 30, 2025

vivek-koppuru Jan 30, 2025

vivek-koppuru Jan 30, 2025 •

edited

Loading

panktishah26 Jan 31, 2025 •

edited

Loading

panktishah26 commented Jan 31, 2025

vivek-koppuru Jan 31, 2025

vivek-koppuru Jan 31, 2025

sp1999 commented Jan 31, 2025

eks-distro-bot commented Feb 1, 2025

		@@ -62,13 +62,4 @@ const (

		// KMS key alias
		KmsKey = "arn:aws:kms:us-west-2:857151390494:alias/signingEKSABundlesKey"

	return fmt.Errorf("failed to get bundle for cluster: %w", err)
	return fmt.Errorf("getting bundle for cluster: %w", err)

	return false, fmt.Errorf("unable to decode the public key (not base 64): %w", err)
	return false, fmt.Errorf("decoding the public key as string: %w", err)

	K8sVersionNotSupportedReason FailureReasonType = "ExtendedKubernetesVersionNotSupported"
	ExtendedK8sVersionSupportNotSupportedReason FailureReasonType = "ExtendedKubernetesVersionSupportNotSupported"

Verify bundle signature is valid and is not modified #9209

Verify bundle signature is valid and is not modified #9209

Conversation

panktishah26 commented Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panktishah26 Jan 29, 2025 • edited Loading

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

vivek-koppuru Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

panktishah26 Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

panktishah26 commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp1999 commented Jan 31, 2025

eks-distro-bot commented Feb 1, 2025

panktishah26 commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

panktishah26 Jan 29, 2025 •

edited

Loading

vivek-koppuru Jan 30, 2025 •

edited

Loading

panktishah26 Jan 31, 2025 •

edited

Loading