-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Add periodic PKC JWK set reloading capability to JWT realm. #136996
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
Conversation
|
Hi @ebarlas, I've created a changelog YAML for you. |
b970c65 to
414c39c
Compare
jfreden
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice job on this! Didn't get that far with my review today but wanted to leave some initial comments at least. Will keep looking tomorrow.
...gin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java
Show resolved
Hide resolved
| return false; | ||
| } | ||
|
|
||
| record JwksResponse(byte[] content, Instant expires) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also parse Cache-Control?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Cache-Control was mentioned in the ticket, but it's less clear how we ought to apply that, since there are many directives. I thought that could be a follow-up enhancement since it doesn't fundamentally change the data flow or reload mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was specifically requested for serverless, what does it look like there? I agree that this could be done in a follow up if not needed there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll add a simple implementation based on the max-age directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache-Control max-age directives are now considered in addition to Expires.
| } | ||
|
|
||
| static class FilePkcJwkSetLoader implements PkcJwkSetLoader { | ||
| final RealmConfig realmConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason these instance variables are not private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only for aesthetic reasons. I opted for less ceremony on the inner class fields, since private fields on an inner class still permit access from the enclosing class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inner class is package-private so you can still access the internals from outside JwkSetLoader within the org.elasticsearch.xpack.security.authc.jwt. The convention in the code base is also to use private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private added to inner class fields.
| } | ||
|
|
||
| static class UrlPkcJwkSetLoader implements PkcJwkSetLoader { | ||
| final RealmConfig realmConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason these instance variables are not private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private added to inner class fields.
| private void handleReloadedContentAndJwksAlgs(byte[] bytes) { | ||
| private void handleReloadedContentAndJwksAlgs(byte[] bytes, boolean init) { | ||
| final ContentAndJwksAlgs newContentAndJwksAlgs = parseContent(bytes); | ||
| assert newContentAndJwksAlgs != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to your change but I noticed this assert can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtRealm.java
Show resolved
Hide resolved
| Scheduler.Cancellable task; | ||
|
|
||
| FilePkcJwkSetLoader(RealmConfig realmConfig, ThreadPool threadPool, String jwkSetPath, Consumer<byte[]> listener) { | ||
| this(realmConfig, threadPool, threadPool == null ? null : threadPool.generic(), jwkSetPath, listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that you're not interested in ThreadPool per se, more so the ExecutorService instance it provides. It might be more informative then to have an ExecutorService var be the arg for this chain of constructors starting from the very top.
Also, then for tests, you don't need to pass in a null you can do EsExecutors.DIRECT_EXECUTOR_SERVICE for a simple test executor.
You'll find various search hits for:
when(threadPool.generic()).thenReturn(EsExecutors.DIRECT_EXECUTOR_SERVICE);
which suggests that this is fairly common substitution for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent quite a while fussing with this aspect of the change. It is clunky. But ExecutorService alone won't work. A scheduler abstraction is what I'm after here. And, for better or worse, the ES Scheduler also need an Executor. So, awkwardly, both are required, I think. Which is why I decided to just propagate ThreadPool.
And I opted for null rather than actual ThreadPool instances (such as TestThreadPool) because the ThreadPool constructor starts threads, which I want to avoid in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah I missed that you need both. makes sense, it's the best you can do here I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you land on using the generic threadpool here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I opted for null rather than actual ThreadPool instances (such as TestThreadPool) because the ThreadPool constructor starts threads, which I want to avoid in tests.
I think handling the null case here is adding test code to the production code. I think you should consider sticking with the TestThreadPool or a mock instance for that reason if that works?
If you do want to support null here the parameter should be marked as @Nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you land on using the
genericthreadpool here?
The JwkSetLoader async workloads are inexpensive and they don't seem related to the other categories
I think handling the
nullcase here is adding test code to the production code.
Agreed. I'll take another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ThreadPool is now used throughout.
I discovered that Mockito mocks of ThreadPool use Objenesis internally to create mock instances that don't invoke the superclass (ThreadPool) constructor. As a result, mocks don't result in thread creation.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtUtil.java
Show resolved
Hide resolved
...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwkSetLoader.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished looking at the changes. I have a couple of comments but overall I think this looks very good.
I think some more integration testing would be nice. Did you consider adding some tests for this to JwtRestIT? Or maybe a separate ESRestTestCase? It's difficult to test because of the minimum refresh interval of 5 minutes, but at least testing the init could be achieved? If not an ESRestTestCase (external cluster) an ESIntegTestCase could work that operates in the same JVM as the test and therefore allows you to do some pretty invasive things.
| } | ||
|
|
||
| static class FilePkcJwkSetLoader implements PkcJwkSetLoader { | ||
| final RealmConfig realmConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inner class is package-private so you can still access the internals from outside JwkSetLoader within the org.elasticsearch.xpack.security.authc.jwt. The convention in the code base is also to use private.
| Scheduler.Cancellable task; | ||
|
|
||
| FilePkcJwkSetLoader(RealmConfig realmConfig, ThreadPool threadPool, String jwkSetPath, Consumer<byte[]> listener) { | ||
| this(realmConfig, threadPool, threadPool == null ? null : threadPool.generic(), jwkSetPath, listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you land on using the generic threadpool here?
| Scheduler.Cancellable task; | ||
|
|
||
| FilePkcJwkSetLoader(RealmConfig realmConfig, ThreadPool threadPool, String jwkSetPath, Consumer<byte[]> listener) { | ||
| this(realmConfig, threadPool, threadPool == null ? null : threadPool.generic(), jwkSetPath, listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I opted for null rather than actual ThreadPool instances (such as TestThreadPool) because the ThreadPool constructor starts threads, which I want to avoid in tests.
I think handling the null case here is adding test code to the production code. I think you should consider sticking with the TestThreadPool or a mock instance for that reason if that works?
If you do want to support null here the parameter should be marked as @Nullable.
| } | ||
| } | ||
|
|
||
| static TimeValue calculateNextUrlReload(TimeValue minVal, TimeValue maxVal, Instant targetTime, double maxJitterPct) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: targetTime should be marked @Nullable
| try { | ||
| // HTTP dates are in RFC 1123 format | ||
| return ZonedDateTime.parse(expires, DateTimeFormatter.RFC_1123_DATE_TIME).toInstant(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just catching DateTimeException here is sufficient and add a trace log (or maybe even debug?) would be helpful.
|
|
||
| boolean changed() throws IOException { | ||
| fileWatcher.checkAndNotify(); // may call onFileInit, onFileChanged | ||
| boolean c = changed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is doing more than just checking if something changed, I wonder if the name could be updated to better reflect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileChangeWatcher presents a simple file-changed abstraction. That could be implemented in several different ways. I'd prefer not to encode the implementation in the method name, if possible. I realize there is some hand waving since it's a concrete inner class, but that's the ideal I was striving for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reservation is that it's not idempotent (since it updates the state of the file watcher) and only reading the method name kind of suggests that it is, but I see your point. An example of a problem this could cause is someone doing logger.debug("File changed [{}]", fileWatcher.changed()) before actually doing the check. Maybe that's a stretch, I'll leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I missed that subtlety in the initial comment. I'll update the name.
|
|
||
| static class FileChangeWatcher implements FileChangesListener { | ||
| final FileWatcher fileWatcher; | ||
| boolean changed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how your approach removes some multithreading concerns. I wonder if this should still be a final AtomicBoolean since a single instance could in theory be accessed from multiple threads when scheduled on a tight interval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FileChangeWatcher instance in FilePkcJwkSetLoader cannot be accessed by multiple threads simultaneously. The associated task is scheduled with scheduleWithFixedDelay, so no concurrent access is possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, looking at this again that makes sense, thanks for clarifying! Really nice that we don't have to worry about concurrency here.
| return false; | ||
| } | ||
|
|
||
| record JwksResponse(byte[] content, Instant expires) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was specifically requested for serverless, what does it look like there? I agree that this could be done in a follow up if not needed there.
| this.listener = listener; | ||
| this.httpClient = httpClient; | ||
| this.reloadIntervalMin = realmConfig.getSetting(JwtRealmSettings.PKC_JWKSET_RELOAD_URL_INTERVAL_MIN); | ||
| this.reloadIntervalMax = realmConfig.getSetting(JwtRealmSettings.PKC_JWKSET_RELOAD_URL_INTERVAL_MAX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a check is required here to make sure min <= max.
|
|
||
| void reload() { | ||
| doLoad(ActionListener.wrap(res -> { | ||
| logger.debug("Successfully reloaded PKC JWK set from HTTPS URI [{}]", jwkSetPathUri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to log the next reload here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea!
| return minVal; | ||
| } | ||
| var min = Duration.ofSeconds(minVal.seconds()); | ||
| var max = Duration.ofSeconds(maxVal.seconds()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An assertion that min <= max could help to find bugs here.
|
Thanks for the thorough review! My intuition was that broader integration tests would do more harm than good in terms of cost and complexity. This change is neutral about the underlying loading mechanisms, which are already tested thoroughly. But it would be nice to have something that verifies that everything is linked properly and that the tasks are running. I'll look into that. |
|
I managed to add It turns out that the existing integration test scaffolding in |
94f6576 to
88ea7ce
Compare
jfreden
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great job!
I have a couple of non-blocking comments. The PR is still in draft mode, so let's take it out of draft before merging.
|
|
||
| @Override | ||
| public void stop() { | ||
| if (task != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think closed should be set here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|
|
||
| static class FileChangeWatcher implements FileChangesListener { | ||
| final FileWatcher fileWatcher; | ||
| boolean changed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, looking at this again that makes sense, thanks for clarifying! Really nice that we don't have to worry about concurrency here.
|
|
||
| boolean changed() throws IOException { | ||
| fileWatcher.checkAndNotify(); // may call onFileInit, onFileChanged | ||
| boolean c = changed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reservation is that it's not idempotent (since it updates the state of the file watcher) and only reading the method name kind of suggests that it is, but I see your point. An example of a problem this could cause is someone doing logger.debug("File changed [{}]", fileWatcher.changed()) before actually doing the check. Maybe that's a stretch, I'll leave it up to you.
| record JwksResponse(byte[] content, Instant expires, Integer maxAgeSeconds) { | ||
| private static final Pattern MAX_AGE_PATTERN = Pattern.compile("\\bmax-age\\s*=\\s*(\\d+)\\b", Pattern.CASE_INSENSITIVE); | ||
|
|
||
| JwksResponse(byte[] content, String expires, String cacheControl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: expires and maxAgeSeconds can be marked @Nullable
| * The Expires header follows RFC 7231 format (e.g., "Thu, 01 Jan 2024 00:00:00 GMT"). | ||
| * @return the parsed Instant, or null if the header is null or cannot be parsed | ||
| */ | ||
| static Instant parseExpires(String expires) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can be marked @Nullable
| } | ||
|
|
||
| /** | ||
| * Parse the Cache-Control header to extract the max-age value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since you reference the RFC in the other method you could potentially add RFC 7234 reference here.
|
|
||
| package org.elasticsearch.xpack.security.authc.jwt; | ||
|
|
||
| interface PkcJwkSetReloadNotifier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be public since it's an argument of the DelegatingJwtSignatureValidator constructor that's public.
| ThreadPool threadPool = mock(ThreadPool.class); | ||
| CountingCallback callback = new CountingCallback(); | ||
|
|
||
| new JwkSetLoader.FilePkcJwkSetLoader(realmConfig, threadPool, path.toString(), callback).start(); // schedules task |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You could also verify close() here.
| expectThrows(SettingsException.class, () -> new JwkSetLoader.UrlPkcJwkSetLoader(realmConfig, null, null, null, null)); | ||
| } | ||
|
|
||
| static class CountingCallback implements Consumer<byte[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: This could be a mock object (looks like there are already some instances doing similar stuff with doAnswer).
| @Before | ||
| public void init() throws Exception { | ||
| threadPool = new TestThreadPool("JWT realm tests"); | ||
| immediateThreadPool = new FixedDelayThreadPool(TimeValue.timeValueMillis(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: I think doing something like this is a little cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this example invokes the Runnable task synchronously in the foreground thread. I think this would create an infinite recursion since the reloader tasks re-schedule themselves.
Also, despite the clunkiness, I do think there's value in including a multi-threading environment in these in integration tests.
|
Pinging @elastic/es-security (Team:Security) |
f0f38a4 to
49219bf
Compare
🔍 Preview links for changed docs |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
f440ce2 to
28b92ab
Compare
… interval. Use PrivilegedFileWatcher to address entitlement exception.
…rmining PKC JWK set reload delay.
28b92ab to
6205941
Compare
Add automatic reloading of PKC (Public Key Cryptography) JWK (JSON Web Key) sets from both file and URL sources, with configurable intervals. File-based JWK sets are reloaded at a fixed interval, with a default of 5 minutes. URL-based JWK sets are reloaded at adaptive intervals informed by Expires and Cache-Control header responses from the JWKS provider. The interval is bounded by a range, with defaults of 5 minutes to 5 days.
This change implements automatic reloading of PKC (Public Key Cryptography) JWK (JSON Web Key) sets from both file and URL sources, with configurable intervals.
Settings Configurations (
JwtRealmSettings.java)Added 4 new realm settings:
pkc_jwkset_reload.enabled- Enable/disable automatic reloading (default: false)pkc_jwkset_reload.file_interval- File check interval (default: 5 minutes)pkc_jwkset_reload.url_interval_min- Minimum URL reload interval (default: 60 minutes)pkc_jwkset_reload.url_interval_max- Maximum URL reload interval (default: 5 days)Core Refactoring (
JwkSetLoader.java)FilePkcJwkSetLoader- Monitors file changes usingFileWatcherUrlPkcJwkSetLoader- Schedules periodic HTTPS fetches with interval calculation based onExpiresheaderThreading Integration
JwtRealm.java: Now acceptsThreadPoolfor scheduling reload tasksJwtAuthenticator.java: Updated to passThreadPoolto signature validatorInternalRealms.java: InjectsThreadPoolinto JWT realm constructionHTTP Response Handling (
JwtUtil.java)readBytes()toreadResponse()returning newJwksResponserecordJwksResponserecord to capture both content andExpiresheaderparseExpires()to parse RFC 1123 HTTP date formatInterface Reorganization
PkcJwkSetReloadNotifierinterface fromJwtSignatureValidatorto its own filePkcJwtSignatureValidatorto remove reload notification responsibility (now in loader)