Adding default certificate to the certificate list for SNI client connections

[AbeOwlu]

Describe the feature you are requesting
Scenarios in controller certificate auto-discovery;

• where different encryption certificate (ECC, RSA) are auto-discovered
• the ECC certificate is discovered first and in the LB model is created as the default certificate
• the RSA encyption certificate is discovered ...and any other certificate, and added to the certificate ilst
• From the ALB documentation, however, the default certificate is never used when a client request specifies the SNI and there is a certificate list
• client attempting to negotiate ECC cipher suite with an ALB only checking only the certificate list fails TLS negotiation

Motivation

• the ALB spec would seem to expect the default certificate to also be added to the certificate list

Describe the proposed solution you'd like

• Add the default certificate also to the certificate list. Perhaps appending the certificate twice?

Contribution Intention (Optional)

• Yes, I am willing to contribute a PR to implement this feature

[sbbowers]

This bug affects my use case. We have both RSA and ECDSA certificates that cover some SNI hostnames for increased compatibility with a large variety of legacy systems.

When Certificate Discovery picks the default certificate, it eliminates it from the list of certificates used an option when negotiating TLS when using SNI. This means that a request that should be covered by two certificates will only be covered by the non-default.

The work-around is to manually specify the annotation, duplicating cert1, so it appears as both the "default" as well as the first non-default:

alb.ingress.kubernetes.io/certificate-arn: arn:aws:acm:us-west-2:xxxxx:certificate/cert1,arn:aws:acm:us-west-2:xxxxx:certificate/cert1,arn:aws:acm:us-west-2:xxxxx:certificate/cert2

https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.2/guide/ingress/annotations/#certificate-arn

It would be nice if certificate discovery was aware of this use case and supported it without manual intervention.

[wweiwei-li]

Thanks for bringing this up. I think adding default certificate to the certificate list is a valid request. We need some investigation on different options:

1. support automatically add the default to the additional certs.
2. support manually specify annotation that add the default to additional certs.

the above mentioned workaround doesn't seem to be working with current logic here

[AbeOwlu]

Opinion on different options posed;
1 In line with the modularity offered in the controller design, it would appear the choice to let this be configurable - however, since certificate auto discovery is already done automatically. I think it the default certificate available in the certificate list would naturally fall in that workflow
2 Would introduce/expose quality-of-life workflow that may not be necessarily required

---

Thoughts following on from my simplistic suggestion, and the update from @wweiwei-li ;
assuming the discovered certificates tlsCerts array has an example CertA(ECC for foo.com) and CertB(RSA for foo.com) then;

• this deduplicated set for mergedTLSCerts array can have, in yet another simplistic suggestion, a flag set for inferredTLSCert and for the first element (assuming first cert, certA is always the default certificate in build ) append to the mergedTLSCerts list just below

however, in this 2 certificates to 1 host mapping; CertA and CertB to foo.com;

• it reads to me like discover no this is fine

[sbbowers]

This was not initially obvious to me, so I'd like to share in the case it helps the conversation:

Server Name Indication (SNI) is an extension to the TLS protocol. ALB's didn't support SNI when they first came out, but support was added shortly after. When they did that, what was "the certificate" became "the default certificate", meaning the certificate that gets used if the client does not support SNI.

To me, the purpose of this controller's certificate discovery feature is to support SNI in a way that optimizes for compatibility.
Given that Certificate Discovery acts on kubernetes Ingress resources, and the kubernete's API contract doesn't allow expressing a loadbalancer in a way that doesn't use SNI, I argue that selecting a default certificate is simply an extracurricular distraction due to the nature of ALB's requiring it.

As to what to do, I'm indifferent about making behavior configurable, but I feel like:

• Default behavior should attach all certificates that match the ingress' hosts to the certificate list.
• Have some algorithm select the default certificate as one from the list. I'm not sure this algorithm matters, as long as its deterministic.
• An annotation for default-certificate-arn to provide control over Refactor josh #2 might be convenient for some rare use cases and it would still allow using discovery by omitting certificate-arn.

So I favor option 1 over option 2.

[AbeOwlu]

I agree @sbbowers , the controller does the first item, building all certs matching the ingress host specified, as would be expected.

• The ALB (ELBv2) API creating a listener is what I understand allows only a single certificate as default, and from mock tests today, only 1 certificate to be specified here period
• this controller passes the first certificate in the built certificate list as the ALB default. And ensures the certificate list it builds is not duplicated
• Selecting any certificate at random or deterministically as default etc. is all fine.
• the hange I am suggesting to the controller is to add that default certificate again to the ALB certificate list, so long a there isn't another check when passing certificate lists to the ELBv2 API

---

• and after investigating the assumptions above, the simplistic solution boils down to the extra cert build to changed to range over entire list and add the default/first cert;

func buildSDKCertificates(modelCerts []elbv2model.Certificate) ([]elbv2types.Certificate, []elbv2types.Certificate) {
...
	defaultSDKCerts = append(defaultSDKCerts, buildSDKCertificate(modelCerts[0]))
	for _, cert := range modelCerts { // from list[1:] to list[0:] : it now adds the first cert to extraSDKCerts
		extraSDKCerts = append(extraSDKCerts, buildSDKCertificate(cert))
	}
...

---

• I agree on surfacing a config default-certificate-arn if so desired, that will just have to be tracked and used as exactly here, while still ranging over the entire list
• @wweiwei-li , I can make this change and test it out if it works for the use case here

PS: a possible erroneous assumption, currently specifying a cert stop the auto-discovery workflow, so the default-certificate-arn may not seem counter-intuitive to auto-discovery?

[sbbowers]

I agree that default-certificate-arn is a misleading annotation name. I think it could be improved with proper documentation and maybe calling it something like auto-discovery-default-certificate-arn.