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

kds-cache: add fallback cache for CRLs on request failure #1050

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

jmxnzo
Copy link
Contributor

@jmxnzo jmxnzo commented Dec 3, 2024

This PR adds logic to the cached_client.go to allow caching Certificate Revocation Lists (CRLs). Still the CRLs are always queried from the AMD's certification server (KDS) first.
Only in case of an request failure, the cache is used as fallback to allow continuing. When the cache of the CRL is expired, the HTTPs request failure is returned as an error.

In addition the test cases where adapted to cover the different branches of VCEK and CRL caching.

@jmxnzo jmxnzo added the changelog PRs that should be part of the release notes label Dec 3, 2024
@jmxnzo jmxnzo requested a review from katexochen December 3, 2024 16:40
@jmxnzo jmxnzo changed the title kds-cache: Always request CRLs but fallback to cache on failure if not expired kds-cache: always request CRLs but fallback to cache on failure if not expired Dec 3, 2024
Copy link
Member

@3u13r 3u13r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great test coverage!

internal/attestation/certcache/cached_client.go Outdated Show resolved Hide resolved
internal/attestation/certcache/cached_client_test.go Outdated Show resolved Hide resolved
@jmxnzo jmxnzo changed the title kds-cache: always request CRLs but fallback to cache on failure if not expired kds-cache: add fallback cache for CRLs on request failure Dec 4, 2024
@jmxnzo jmxnzo requested a review from 3u13r December 4, 2024 15:23
Copy link
Member

@3u13r 3u13r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@katexochen katexochen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you verify the CRL expiration date is checked? Could you link the code that does this?

@jmxnzo
Copy link
Contributor Author

jmxnzo commented Dec 10, 2024

Did you verify the CRL expiration date is checked? Could you link the code that does this?

The expiration date is checked right here https://github.com/google/go-sev-guest/blob/main/verify/verify.go#L303-L305 . As well the usage of CachedHTTPSGetter by the CRL request was re-examined: it is ensured by handing in the shared CachedHTTPSGetter as verify.Options (see https://github.com/google/go-sev-guest/blob/main/verify/verify.go#L308).

In summary we keep sharing one cache across all of the certificates and revocation list, using the same kds-Getter in the verify.Options (https://github.com/edgelesssys/contrast/blob/main/coordinator/internal/authority/credentials.go#L82).
@3u13r Thx for code guidance yesterday:)

@jmxnzo jmxnzo merged commit 0a1bc0a into main Dec 12, 2024
10 checks passed
@jmxnzo jmxnzo deleted the kds-caching-jla branch December 12, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog PRs that should be part of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants