crypto: update fingerprint at same time as certificate#10036
Merged
BeryJu merged 1 commit intogoauthentik:mainfrom Jun 10, 2024
Merged
crypto: update fingerprint at same time as certificate#10036BeryJu merged 1 commit intogoauthentik:mainfrom
BeryJu merged 1 commit intogoauthentik:mainfrom
Conversation
Previously the fingerprint was only set when initially adding a key, if it changed for any reason (like a renewed certificate) then every execution of `Get` would lead to a full update. The certificate itself got cached, but the fingerprint remained stale for next time. This increased the chance of a fatal race during the cache update. closes goauthentik#9907
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Contributor
Author
|
This is my first PR here so sorry if I did anything wrong. I know this doesn't eliminate all possibility of races but I think that change would need deeper architectural knowledge than I have. Since running this one I've not seen any events logged that could even potentially conflict (a single timed task seems to pick up the change and update the cache before anything actually concurrent happens, at least on my config). |
BeryJu
approved these changes
Jun 9, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10036 +/- ##
==========================================
- Coverage 92.66% 92.64% -0.03%
==========================================
Files 708 713 +5
Lines 34619 34878 +259
==========================================
+ Hits 32079 32311 +232
- Misses 2540 2567 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
kensternberg-authentik
added a commit
that referenced
this pull request
Jun 11, 2024
* main: (66 commits) rbac: filters: fix missing attribute for unauthenticated requests (#10061) tests/e2e: docker-compose.yml: remove version element forgotten last time (#10067) providers/microsoft_entra: fix error when updating connection attributes (#10039) website/integrations: aws: fix about service link (#10062) translate: Updates for file locale/en/LC_MESSAGES/django.po in it (#10060) core: bump github.com/redis/go-redis/v9 from 9.5.2 to 9.5.3 (#10046) core: bump github.com/gorilla/websocket from 1.5.1 to 1.5.2 (#10047) translate: Updates for file locale/en/LC_MESSAGES/django.po in ru (#10058) core: bump sentry-sdk from 2.5.0 to 2.5.1 (#10048) core: bump django-cte from 1.3.2 to 1.3.3 (#10049) core: bump packaging from 24.0 to 24.1 (#10050) web: bump yaml from 2.4.3 to 2.4.5 in /web (#10054) web: bump @sentry/browser from 8.7.0 to 8.8.0 in /web in the sentry group (#10051) web: bump esbuild from 0.21.4 to 0.21.5 in /web (#10053) crypto: update fingerprint at same time as certificate (#10036) translate: Updates for file locale/en/LC_MESSAGES/django.po in ru (#10056) website/integrations: gitea: fix helm values (#10043) core, web: update translations (#10038) translate: Updates for file locale/en/LC_MESSAGES/django.po in ru (#10040) docs/troubleshooting: upgrade docker: erroneous command (#10044) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Previously the fingerprint was only set when initially adding a key, if it changed for any reason (like a renewed certificate) then every execution of
Getwould lead to a full update. The certificate itself got cached, but the fingerprint remained stale for next time.This increased the chance of a fatal race during the cache update.
Should fix #9907. I've been running it locally for a while with no more races of any kind, let alone ones that actually clash and crash.
Checklist
ak test authentik/)make lint-fix)If an API change has been made (n/a)
If changes to the frontend have been made (n/a)
If applicable (documentation n/a)