Implement mutual TLS authentication support for Ingress #3532

shraddhabang · 2024-01-17T21:25:38Z

Issue

Description

Support for (mTLS) Mutual Transport Layer Security on Ingress through AWS LB Controller

This PR adds the support for mutual TLS authentication support for Ingress. Its delivers mTLS feature by integrating the Trust Stores into listener management. The customer will be able to set the mTLS mode and will be able to provide the existing Trust Store Name/ARN (they have created through CLI/Console) through new annotations for Ingress.

New Ingress Annotation

alb.ingress.kubernetes.io/mutual-authentication
Type: string
Default Value: '[{"port": 443, "mode": "off"}]'
# port: listen port
# mode: "off" (deafult) | "passthrough" | "verify"
# trustStore: "arn:aws:elasticloadbalancing:trustStoreArn" or "my-trust-store"// required when the mode is "verify"
# ignoreClientCertificateExpiry : true | false (default)

Example

# Listen port configuration
alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS": 80}, {"HTTPS": 443}, {"HTTPS": 8080}, {"HTTPS": 8443}]'


# mTLS configuration
alb.ingress.kubernetes.io/mutual-authentication: '[{"port": 80, "mode": "passthrough"}, 
                                    {"port": 443, "mode": "verify", "trustStore": "arn:aws:elasticloadbalancing:trustStoreArn", "ignoreClientCertificateExpiry" : true}]'

We will identify the HTTPS listener based on its port and set the specified the mTLS configuration. So in the above example, we specified four HTTPS listeners with ports 80, 443, 8080, 8443. As per the annotation, the listener HTTPS:80 will be set to "passthrough” mode, the listener HTTPS:443 will be set to “verify” mode and will be associated with the provided trust-store-arn “arn:aws:elasticloadbalancing:trustStoreArn”. The remaining listeners HTTPS:8080 and HTTPS:8443 will be set to default mTLS mode i.e “off”.

New load balancer attributes for enabling connection logs for Ingresses and IngressGroup

We are also adding the support for connection logs support similar to what we support for access logs today for ALB.

Example on Ingress annotation

alb.ingress.kubernetes.io/load-balancer-attributes: connection_logs.s3.enabled=true,connection_logs.s3.bucket=my-connection-log-bucket,access_logs.s3.prefix=my-app

Example on IngressClassParams specification

apiVersion: elbv2.k8s.aws/v1beta1
kind: IngressClassParams
metadata:
    name: awesome-class
spec:
  loadBalancerAttributes:
  - key: connection_logs.s3.enabled
    value: "true"
  - key: connection_logs.s3.bucket
    value: "my-connection-logs"  
  - key: connection_logs.s3.prefix
    value: "myapp"

More : https://docs.aws.amazon.com/elasticloadbalancing/latest/application/mutual-authentication.html

Checklist

Added tests that cover your change (if possible) - Added some unit tests
Added/modified documentation as required (such as the README.md, or the docs directory) - Will raise separate PR for this
Manually tested
- Verified the config set on ingress annotation for mTLS is reflected on the associated ALB for all the modes while creating and modifying listener
- Verified the trustStore name is accepted through ingress annotation and controller was able to fetch the associated trust-store-arn and create and modify the listener properly
- Verified that the ingress class param is accepted and given precedence over ingress annotation
- Manually tested input validations
- Verified the new connection logs attribute is being updated through ingress annotation
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

k8s-ci-robot · 2024-01-17T21:25:47Z

Hi @shraddhabang. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

oliviassss · 2024-01-17T21:58:20Z

/ok-to-test

shraddhabang · 2024-01-18T17:01:32Z

/retest

codecov-commenter · 2024-01-18T19:45:04Z

Codecov Report

Attention: 118 lines in your changes are missing coverage. Please review.

Comparison is base (0b82ebb) 55.75% compared to head (a10b7de) 55.11%.

❗ Current head a10b7de differs from pull request most recent head 5db0cc7. Consider uploading reports for the commit 5db0cc7 to get more accurate results

Files	Patch %	Lines
pkg/ingress/model_build_listener.go	15.38%	97 Missing and 2 partials ⚠️
pkg/deploy/elbv2/listener_manager.go	7.69%	11 Missing and 1 partial ⚠️
pkg/ingress/model_builder.go	65.00%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3532      +/-   ##
==========================================
- Coverage   55.75%   55.11%   -0.64%     
==========================================
  Files         149      149              
  Lines        8843     8979     +136     
==========================================
+ Hits         4930     4949      +19     
- Misses       3579     3691     +112     
- Partials      334      339       +5

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

go.mod

oliviassss · 2024-01-19T00:19:17Z

pkg/ingress/model_build_listener.go

+	if exists := t.annotationParser.ParseStringAnnotation(annotations.IngressSuffixMutualAuthentication, &rawMtlsConfigString, ing.Ing.Annotations); !exists {
+		if !fromIngressClassParams {
+			return map[int64]*elbv2model.MutualAuthenticationAttributes{443: {
+				Mode: string(elbv2model.MutualAuthenticationOffMode),


What if the users do not have port 443 in the listeners (like they have port 8080), and neither do they specify the mtls setting?

also need to call out the default mtls in the ingress annotation and ingress class param doc.

This will be ignored then while building the Listener SDK request since we only map the port and their respective setting in mTLS.

torredil · 2024-01-19T18:05:34Z

/retest

shraddhabang · 2024-01-22T22:56:19Z

/retest

shraddhabang · 2024-01-23T22:19:54Z

/retest

M00nF1sh · 2024-01-24T19:01:11Z

apis/elbv2/v1beta1/ingressclassparams_types.go

@@ -107,6 +109,10 @@ type IngressClassParamsSpec struct {
 	// +optional
 	SSLPolicy string `json:"sslPolicy,omitEmpty"`

+	// MutualAuthentication specifies the mutual TLS authentication config for all Ingresses that belong to IngressClass with this IngressClassParams.
+	// +optional
+	MutualAuthentication *MutualAuthentication `json:"mutualAuthentication,omitempty"`


type MutualAuthentication struct {
Mode ...
TrustStoreArn...
}
type IngressClassParamsSpec struct {
....

// MutualAuthentication specifies the mutual TLS authentication config for all Ingresses that belong to IngressClass with this IngressClassParams. // +optional MutualAuthentication *MutualAuthentication `json:"mutualAuthentication,omitempty"`

}

As discussed, removed the support for IngressClassParam from this release. We will add a documentation update which will specify that warning to use mTLS config on IngressGroups. Thanks for the review.
We will add the IngressClassParam support for mTLS in follow up patch release.

M00nF1sh

/lgtm
/approve

k8s-ci-robot · 2024-01-26T18:54:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh, shraddhabang

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~~ [M00nF1sh]

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

…igs#3532) * Implement mutual TLS authentication support for Ingress * Addressing comments and adding policy updates * Adding dependency for maps * removing ingressClassParam support

Implement mutual TLS authentication support for Ingress

6e13fb9

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 17, 2024

k8s-ci-robot requested review from kishorj and oliviassss January 17, 2024 21:25

shraddhabang mentioned this pull request Jan 17, 2024

REQUEST: New membership for shraddhabang kubernetes/org#4691

Closed

9 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 17, 2024

oliviassss reviewed Jan 19, 2024

View reviewed changes

shraddhabang added 2 commits January 19, 2024 10:08

Addressing comments and adding policy updates

09ab418

Adding dependency for maps

7bbd197

M00nF1sh reviewed Jan 24, 2024

View reviewed changes

shraddhabang and others added 2 commits January 24, 2024 14:06

Merge branch 'kubernetes-sigs:main' into main

a10b7de

removing ingressClassParam support

5db0cc7

oliviassss added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 24, 2024

Merge branch 'kubernetes-sigs:main' into main

fe57ea4

M00nF1sh reviewed Jan 26, 2024

View reviewed changes

k8s-ci-robot assigned M00nF1sh Jan 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2024

k8s-ci-robot merged commit d0c13bf into kubernetes-sigs:main Jan 26, 2024
9 checks passed

This was referenced Feb 2, 2024

[iam-role-for-service-accounts-eks]: new IAM policy action required for aws-load-balancer-controller v2.7.0 terraform-aws-modules/terraform-aws-iam#457

Closed

feat: Support new mTLS feature in awslb IAM policy terraform-aws-modules/terraform-aws-iam#458

Merged

M00nF1sh mentioned this pull request Feb 9, 2024

🥳 aws-load-balancer-controller v2.7.0 Automated Release! 🥑 aws/eks-charts#1059

Closed

andreybutenko mentioned this pull request Apr 23, 2024

Add DescribeTrustStores to IAM policy for China partition #3662

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement mutual TLS authentication support for Ingress #3532

Implement mutual TLS authentication support for Ingress #3532

shraddhabang commented Jan 17, 2024 •

edited

Loading

k8s-ci-robot commented Jan 17, 2024

oliviassss commented Jan 17, 2024

shraddhabang commented Jan 18, 2024

codecov-commenter commented Jan 18, 2024 •

edited

Loading

oliviassss Jan 19, 2024

oliviassss Jan 19, 2024

shraddhabang Jan 19, 2024

torredil commented Jan 19, 2024

shraddhabang commented Jan 22, 2024

shraddhabang commented Jan 23, 2024

M00nF1sh Jan 24, 2024

shraddhabang Jan 24, 2024

M00nF1sh left a comment

k8s-ci-robot commented Jan 26, 2024

Implement mutual TLS authentication support for Ingress #3532

Implement mutual TLS authentication support for Ingress #3532

Conversation

shraddhabang commented Jan 17, 2024 • edited Loading

Issue

Description

New Ingress Annotation

Example

New load balancer attributes for enabling connection logs for Ingresses and IngressGroup

Example on Ingress annotation

Example on IngressClassParams specification

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

k8s-ci-robot commented Jan 17, 2024

oliviassss commented Jan 17, 2024

shraddhabang commented Jan 18, 2024

codecov-commenter commented Jan 18, 2024 • edited Loading

Codecov Report

oliviassss Jan 19, 2024

Choose a reason for hiding this comment

oliviassss Jan 19, 2024

Choose a reason for hiding this comment

shraddhabang Jan 19, 2024

Choose a reason for hiding this comment

torredil commented Jan 19, 2024

shraddhabang commented Jan 22, 2024

shraddhabang commented Jan 23, 2024

M00nF1sh Jan 24, 2024

Choose a reason for hiding this comment

shraddhabang Jan 24, 2024

Choose a reason for hiding this comment

M00nF1sh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 26, 2024

shraddhabang commented Jan 17, 2024 •

edited

Loading

codecov-commenter commented Jan 18, 2024 •

edited

Loading