JWT realm support for HTTPS URL in PKC JWKSet setting #84630
Conversation
justincr-elastic
added
:Security/Authentication
|
Pinging @elastic/es-security (Team:Security) |
|
Hi @justincr-elastic, I've created a changelog YAML for you. |
justincr-elastic
changed the title
|
#68967 talks about how we use HttpsServer for tests. |
…n:security:forbiddenApisMain error from es-server-signatures.txt.
|
@elasticmachine update branch |
|
merge conflict between base and head |
…feature/jwt-realm-https-pkc-jwkset
There was a problem hiding this comment.
I left a few comments, also:
- I don't think the PR should be labelled as
featurebecause it really is about tests - There are some complexities in how
JwtIssuer,JwtIssuerHttpsServeris created and managed. They work as is, but I'd personally prefer having them managed more transparently by the baseJwtTestCase. That said, these are all just test code and I don't have too strong opinions.
| } catch (Throwable t) { | ||
| this.close(); | ||
| throw t; | ||
| } |
There was a problem hiding this comment.
Shouldn't this belong to test code? e.g. invoke jwtRealm.close() in a @After method in test class?
There was a problem hiding this comment.
Answer my own question here. I think this is added to production code because the instantiation may fail in tests and there will no instance of jwtRealm to call close upon. Generally I don't like change production code for tests. But this seems to be minor enough and the behaviour is retrow anyway.
| final Registry<SchemeIOSessionStrategy> registry = RegistryBuilder.<SchemeIOSessionStrategy>create() | ||
| .register("https", new SSLIOSessionStrategy(clientContext, verifier)) | ||
| .build(); |
There was a problem hiding this comment.
Just for my own knowledge: we have decided to not support plain http in fetching JWKs?
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtUtil.java
Outdated
Show resolved
Hide resolved
...security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtRealmGenerateTests.java
Outdated
Show resolved
Hide resolved
.../security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtIssuerHttpsServer.java
Outdated
Show resolved
Hide resolved
.../security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtIssuerHttpsServer.java
Outdated
Show resolved
Hide resolved
.../security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtIssuerHttpsServer.java
Show resolved
Hide resolved
...ck/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtTestCase.java
Outdated
Show resolved
Hide resolved
justincr-elastic
added
>test
…roller.doPrivileged.
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
I am proposing to leave MockHttpServer in this PR.
I will add a task to track switching to MockWebServer later, which will be tried to PKC JWKSet reloading at runtime. |
Add HTTPS server to JWT issuer.
JwtRealmTestCase.createJwtIssuer() decides if public PKC JWKSet should be made available via local file or HTTPS server. If HTTPS, it passes settings to JwtIssuer to initialize a HTTPS server, and JwtIssuer.close() can be used to clean up the HTTPS server thread pool.
JwtRealmTestCase.createJwtRealm() checks if JwtIssuer has a HTTPS server. If yes, it uses the URL computed by the JwtIssuer server. If no, it puts the JWKSet in a temp file and points its config to it.