Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CertDiscovery: Ability to restrict/filter available certs #3565

Closed
mkilchhofer opened this issue Feb 8, 2024 · 1 comment · Fixed by #3591
Closed

CertDiscovery: Ability to restrict/filter available certs #3565

mkilchhofer opened this issue Feb 8, 2024 · 1 comment · Fixed by #3591
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@mkilchhofer
Copy link

mkilchhofer commented Feb 8, 2024

Is your feature request related to a problem?
Our intent is to replace the certificates with newly issued ones from a different PCA issuer (centrally managed via Terraform).
I already read through #1598 to clarify what options we have.

Describe the solution you'd like
I think it would be helpful if we can enhance this function here with some filtering methods:

func (d *acmCertDiscovery) loadAllCertificateARNs(ctx context.Context) ([]string, error) {
if rawCacheItem, ok := d.certARNsCache.Get(certARNsCacheKey); ok {
return rawCacheItem.([]string), nil
}
req := &acm.ListCertificatesInput{
CertificateStatuses: aws.StringSlice([]string{acm.CertificateStatusIssued}),
Includes: &acm.Filters{
KeyTypes: aws.StringSlice(acm.KeyAlgorithm_Values()),
},
}
certSummaries, err := d.acmClient.ListCertificatesAsList(ctx, req)
if err != nil {
return nil, err
}
var certARNs []string
for _, certSummary := range certSummaries {
certARN := aws.StringValue(certSummary.CertificateArn)
certARNs = append(certARNs, certARN)
}
d.certARNsCache.Set(certARNsCacheKey, certARNs, d.certARNsCacheTTL)
return certARNs, nil
}

I am thinking about filtering for the CertificateAuthorityArn(s) as we have a new CA. Another option would be to filter for Issued at newer than <date XYZ>.

Maybe something like this

		if len(allowedCertificateAuthorityArns) > 0 {
			resp, err := acmClient.DescribeCertificate(&acm.DescribeCertificateInput{
				CertificateArn: certSummary.CertificateArn,
			})
			if err == nil && slices.Contains(allowedCertificateAuthorityArns, aws.StringValue(resp.Certificate.CertificateAuthorityArn)) {
				certARN := aws.StringValue(certSummary.CertificateArn)
				certARNs = append(certARNs, certARN)
			}
		} else {
			certARN := aws.StringValue(certSummary.CertificateArn)
			certARNs = append(certARNs, certARN)
		}

(allowedCertificateAuthorityArns is type []string{})

Downside of this is that the CertificateAuthorityArn is not part of the summary and requires an extra API call to get the details of the certificate. But the cert details are already retrieved inside the func loadDomainsForCertificate, so maybe we can cache these in an efficient way.

Describe alternatives you've considered
Do the migration via the already available option (using multiple certificates mentioned above)

/cc: @the-technat, @blanchardma, @aescrob, @asdfgugus, @js-315385995

@mkilchhofer
Copy link
Author

Together with @the-technat we already filed a proposal over there:

@shraddhabang shraddhabang added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Feb 12, 2024
shraddhabang pushed a commit to shraddhabang/aws-load-balancer-controller that referenced this issue Mar 20, 2024
M00nF1sh pushed a commit that referenced this issue Mar 22, 2024
* fix log level in listener manager and tagging manager (#3573)

* bump up controller-gen version and update manifests (#3580)

* docs: ingress subnets annotation - clarify locale differences (#3579)

* feat: allowed ACM cert discovery to filter on CA ARNs (#3565) (#3591)

* Add example for NLB target-group-attributes to enable unhealthy target connection draining (#3577)

* Add example annotation for NLB unhealthy target connection draining

* Add emtpyline back in

* fix: ca-filter causing expontentially more api-calls (#3608)

due to missing cache

* Repo controlled build go version (#3598)

* update go version to mitigate CVE (#3615)

* Adding support for Availability Zone Affinity (#3470)

Fixes #3431

Signed-off-by: Alex Berger <alex-berger@gmx.ch>

* Update golang.org/protobuf version to fix CVE-2024-24786 (#3618)

* Add a note to recommend to use compatible chart and image versions

* Update golang.org/protobuf version to fix CVE-2024-24786

---------

Signed-off-by: Alex Berger <alex-berger@gmx.ch>
Co-authored-by: Olivia Song <sonyingy@amazon.com>
Co-authored-by: Andrey Lebedev <alebedev87@gmail.com>
Co-authored-by: Nathanael Liechti <technat@technat.ch>
Co-authored-by: Isaac Wilson <10012479+jukie@users.noreply.github.com>
Co-authored-by: Nathanael Liechti <nathanael.liechti@post.ch>
Co-authored-by: Jason Du <jasonxdu@amazon.com>
Co-authored-by: Hao Zhou <haouc@users.noreply.github.com>
Co-authored-by: Alexander Berger <alex-berger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
2 participants