-
Notifications
You must be signed in to change notification settings - Fork 25.2k
http proxy support in JWT realm #127337
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
http proxy support in JWT realm #127337
Conversation
Hi @richard-dennehy, I've created a changelog YAML for you. |
...gin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java
Show resolved
Hide resolved
...gin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java
Outdated
Show resolved
Hide resolved
...ests/src/javaRestTest/java/org/elasticsearch/xpack/security/authc/jwt/JwtWithOidcAuthIT.java
Show resolved
Hide resolved
private static final Integer PORT = 8888; | ||
private static final Integer TLS_PORT = 8889; |
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.
Names are hard 😞
This isn't an SSL port, but it proxies to the C2ID SSL port 🤔
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'm blanking on a better name as well. Will have to think about it.
...idp-fixture/src/main/java/org/elasticsearch/test/fixtures/idp/OidcProviderTestContainer.java
Show resolved
Hide resolved
FROM c2id/c2id-server-demo:16.1.1 AS c2id | ||
FROM openjdk:21-jdk-buster |
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.
JDK 11 doesn't run properly on my kernel; C2ID 12 doesn't run properly on JDK 21
7347efa
to
9c2bafc
Compare
Pinging @elastic/es-security (Team:Security) |
...gin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java
Show resolved
Hide resolved
...gin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java
Outdated
Show resolved
Hide resolved
...gin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java
Show resolved
Hide resolved
key -> Setting.simpleString(key, "http", value -> { | ||
if (value.equals("http") == false && value.equals("https") == false) { | ||
throw new IllegalArgumentException("Invalid value [" + value + "] for [" + key + "]. Only `http` or `https` are allowed."); | ||
} | ||
}, Setting.Property.NodeScope) | ||
); |
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 can be simplified by calling SecuritySettingsUtil#verifyNonNullNotEmpty
, for example:
key -> Setting.simpleString(key, "http", value -> { | |
if (value.equals("http") == false && value.equals("https") == false) { | |
throw new IllegalArgumentException("Invalid value [" + value + "] for [" + key + "]. Only `http` or `https` are allowed."); | |
} | |
}, Setting.Property.NodeScope) | |
); | |
key -> Setting.simpleString( | |
key, | |
"http", | |
value -> verifyNonNullNotEmpty(key, value, List.of("http", "https")), | |
Setting.Property.NodeScope | |
) | |
); |
HTTP_MAX_ENDPOINT_CONNECTIONS, | ||
HTTP_PROXY_SCHEME, | ||
HTTP_PROXY_HOST, | ||
HTTP_PROXY_PORT |
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.
We also have to document these new settings. I'm fine if you prefer to do documentation update in a followup PR.
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.
Do we want to backport this? 9.0 docs are in a different repo, so I need to raise a different PR regardless
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, we should backport this to 8.18 at least. Makes total sense to handle the docs in a followup. The docs for 9.x are in different repo, but docs for 8.x are still in elastricsearch repo.
...idp-fixture/src/main/java/org/elasticsearch/test/fixtures/idp/OidcProviderTestContainer.java
Show resolved
Hide resolved
private static final Integer PORT = 8888; | ||
private static final Integer TLS_PORT = 8889; |
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'm blanking on a better name as well. Will have to think about it.
020564a
to
d06d2cd
Compare
@@ -6,10 +6,18 @@ | |||
<scope name="Production" level="ERROR" enabled="false" /> | |||
<scope name="Production minus fixtures" level="ERROR" enabled="true" /> | |||
</inspection_tool> | |||
<inspection_tool class="MalformedFormatString" enabled="true" level="WARNING" enabled_by_default="true"> |
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.
Was this change intentional?
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 was messing with Intellij settings; I'm used to these project files being in the .gitignore
- I'll remove it
9c3b8b9
to
a8b028d
Compare
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 👍
Thanks for the persistence on this one. Nice job!
http proxy support in JWT realm
http proxy support in JWT realm
Resolves #114956
Adds
http.proxy.*
settings to JWT realm configuration, to enable JWKS lookup to use a HTTP proxy.It's worth stressing that this proxy must use HTTP rather than HTTPS, due to #100264