Skip to content

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

Merged
merged 7 commits into from
May 1, 2025

Conversation

richard-dennehy
Copy link
Contributor

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

@richard-dennehy richard-dennehy added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team labels Apr 24, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @richard-dennehy, I've created a changelog YAML for you.

private static final Integer PORT = 8888;
private static final Integer TLS_PORT = 8889;
Copy link
Contributor Author

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 🤔

Copy link
Contributor

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.

Comment on lines +1 to +2
FROM c2id/c2id-server-demo:16.1.1 AS c2id
FROM openjdk:21-jdk-buster
Copy link
Contributor Author

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

@richard-dennehy richard-dennehy marked this pull request as ready for review April 24, 2025 15:34
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

Comment on lines 547 to 529
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)
);
Copy link
Contributor

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:

Suggested change
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
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

private static final Integer PORT = 8888;
private static final Integer TLS_PORT = 8889;
Copy link
Contributor

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.

@richard-dennehy richard-dennehy requested a review from a team as a code owner April 29, 2025 10:02
@richard-dennehy richard-dennehy added auto-backport Automatically create backport pull requests when merged v8.18.2 labels Apr 30, 2025
@@ -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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intentional?

Copy link
Contributor Author

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

Copy link
Contributor

@slobodanadamovic slobodanadamovic left a 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!

@richard-dennehy richard-dennehy merged commit e75e45b into elastic:main May 1, 2025
22 checks passed
@richard-dennehy richard-dennehy deleted the jwt-proxy-support branch May 1, 2025 07:57
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
9.0

richard-dennehy added a commit to richard-dennehy/elasticsearch that referenced this pull request May 1, 2025
http proxy support in JWT realm
richard-dennehy added a commit to richard-dennehy/elasticsearch that referenced this pull request May 1, 2025
http proxy support in JWT realm
elasticsearchmachine pushed a commit that referenced this pull request May 1, 2025
elasticsearchmachine pushed a commit that referenced this pull request May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v8.18.2 v9.0.2 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add proxy support for JWT realm.
3 participants