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

fix(certificate): hybrid dp cannot refresh certificate entity with vault reference #12868

Merged
merged 4 commits into from
Apr 19, 2024

Conversation

windmgc
Copy link
Member

@windmgc windmgc commented Apr 16, 2024

Summary

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

The root cause of this problem is that in a dp node running hybrid mode, kong cache interfaces force the cache TTL to be infinite(and it also does not respect cache TTL configured in kong.conf), thus the certificate object cannot be updated unless a new configuration is received and cache purge being called.

The PR fixes the problem by caching the vault reference fields $ref and call kong.vault.update every time in get_certificate, and l1 serializer will keep the value serialized inside the certificate object.

This PR contains a test case that tests a certificate entity with a vault-referenced key that can be refreshed promptly, according to the TTL configured on the vault reference. The test case will fail consistently without the fix.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

FTI-5881

@github-actions github-actions bot added core/proxy core/pdk cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Apr 16, 2024
@windmgc windmgc force-pushed the upstream-certificate-vault-ref-update branch from 474bc7d to 152772d Compare April 16, 2024 14:59
@bungle
Copy link
Member

bungle commented Apr 16, 2024

why wasn't this enough:
https://github.com/Kong/kong/blob/master/kong/runloop/certificate.lua#L217

I don't understand. Is it because of hit_level ~= 3, but it shoud be only 3 when caches have been flushed, and in that case DAO has updated it already?

@windmgc
Copy link
Member Author

windmgc commented Apr 16, 2024

@bungle The value certificate returned by this line

local certificate, err, hit_level = kong.core_cache:get(cache_key,
is something like

{cert = cdata<void *>: 0x0118f09730, key = cdata<void *>: 0x0118f0be20}

Without the $ref fields and plain cert/key fields. Because it has a l1 serializer configured here

l1_serializer = parse_key_and_cert,

So we're actually caching these cdata objects inside the L1 cache.

Calling kong.vault.update on this table will have no real effect

On the other hand secrets may gets updated when certificate entity is still in cache(hit L1/L2), so certificate entity's TTL must follow vault's TTL in some way

@bungle
Copy link
Member

bungle commented Apr 16, 2024

certificate object cannot be refreshed unless a new configuration is received and cache purge being called.

The certificate entity itself is not refreshed unless there is new config. In which case it is read again from LMDB (because caches are purged)? But vault.update is always called before certificate is served.

The PR fixes the problem by overriding the TTL value returned by the certificate fetching callback function

This is where I have problem of understanding. Why would infinite value be a problem if caches are purged, and entity cannot change without caches being purged?

@bungle
Copy link
Member

bungle commented Apr 16, 2024

Because it has a l1 serializer configured here

Oh... that is the key! Thanks.

@bungle
Copy link
Member

bungle commented Apr 16, 2024

@windmgc check last commit here for alternate proposal (basically only changing certificate.lua):
#12870

My proposal:
d0deba8#diff-44f5bd52a34ad738a8e7a355f439efdb4cff0711743986d7ce52384e7d2cd1d2

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@windmgc
Copy link
Member Author

windmgc commented Apr 17, 2024

@bungle I've applied the changes in your proposal, please review again, thanks!

@bungle
Copy link
Member

bungle commented Apr 18, 2024

@kikito I added some backport labels to this.

@windmgc windmgc merged commit 7dd29d4 into master Apr 19, 2024
25 checks passed
@windmgc windmgc deleted the upstream-certificate-vault-ref-update branch April 19, 2024 06:03
github-actions bot pushed a commit that referenced this pull request Apr 19, 2024
…ult reference (#12868)

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

FTI-5881
---------

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit 7dd29d4)
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

@team-gateway-bot
Copy link
Collaborator

github-actions bot pushed a commit that referenced this pull request Apr 19, 2024
…ult reference (#12868)

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

FTI-5881
---------

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit 7dd29d4)
@team-gateway-bot
Copy link
Collaborator

github-actions bot pushed a commit that referenced this pull request Apr 19, 2024
…ult reference (#12868)

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

FTI-5881
---------

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit 7dd29d4)
@team-gateway-bot
Copy link
Collaborator

ms2008 pushed a commit that referenced this pull request Apr 19, 2024
…ult reference (#12868)

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

FTI-5881
---------

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit 7dd29d4)
ms2008 pushed a commit that referenced this pull request Apr 19, 2024
…ult reference (#12868)

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

FTI-5881
---------

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit 7dd29d4)
windmgc added a commit that referenced this pull request Apr 19, 2024
…ult reference (#12868)

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

FTI-5881
---------

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit 7dd29d4)
locao pushed a commit that referenced this pull request Apr 24, 2024
…ult reference (#12868)

This PR fixes a problem that a certificate entity with cert/keys stored in a vault-referenced type cannot be refreshed even if the vault reference secret value has been updated both in L1/L2 vault PDK.

FTI-5881
---------

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit 7dd29d4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/pdk core/proxy size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants