-
Notifications
You must be signed in to change notification settings - Fork 296
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
Add support for OpenId Connect userinfo_endpoint #4649
base: main
Are you sure you want to change the base?
Changes from 19 commits
1a3e5d9
1c0344a
e300d7d
7593421
3b9ef8c
6df0d02
9115585
6497003
2f4f065
e0c5c17
8450932
435c099
1684db1
b601a92
c837dcf
3fbf595
fd8dd4c
8e25e32
ec73fd2
ca57141
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,289 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
* | ||
* Modifications Copyright OpenSearch Contributors. See | ||
* GitHub history for details. | ||
*/ | ||
|
||
package com.amazon.dlic.auth.http.jwt.keybyoidc; | ||
|
||
import java.io.IOException; | ||
import java.net.URI; | ||
import java.net.URISyntaxException; | ||
import java.nio.file.Path; | ||
import java.text.ParseException; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
|
||
import org.apache.logging.log4j.LogManager; | ||
import org.apache.logging.log4j.Logger; | ||
|
||
import org.opensearch.OpenSearchSecurityException; | ||
import org.opensearch.common.settings.Settings; | ||
import org.opensearch.common.util.concurrent.ThreadContext; | ||
import org.opensearch.security.auth.HTTPAuthenticator; | ||
import org.opensearch.security.filter.SecurityRequest; | ||
import org.opensearch.security.filter.SecurityResponse; | ||
import org.opensearch.security.user.AuthCredentials; | ||
|
||
import com.amazon.dlic.auth.http.jwt.AbstractHTTPJwtAuthenticator; | ||
import com.amazon.dlic.util.SettingsBasedSSLConfigurator; | ||
import com.nimbusds.common.contenttype.ContentType; | ||
import com.nimbusds.jwt.JWTClaimsSet; | ||
import com.nimbusds.jwt.SignedJWT; | ||
import com.nimbusds.oauth2.sdk.http.HTTPRequest; | ||
import com.nimbusds.oauth2.sdk.http.HTTPResponse; | ||
import com.nimbusds.oauth2.sdk.token.AccessToken; | ||
import com.nimbusds.oauth2.sdk.token.AccessTokenType; | ||
import com.nimbusds.oauth2.sdk.util.StringUtils; | ||
import com.nimbusds.openid.connect.sdk.UserInfoRequest; | ||
import com.nimbusds.openid.connect.sdk.UserInfoResponse; | ||
|
||
import static org.apache.hc.core5.http.HttpHeaders.AUTHORIZATION; | ||
import static com.amazon.dlic.auth.http.jwt.keybyoidc.OpenIdConstants.CLIENT_ID; | ||
import static com.amazon.dlic.auth.http.jwt.keybyoidc.OpenIdConstants.ISSUER_ID_URL; | ||
import static com.amazon.dlic.auth.http.jwt.keybyoidc.OpenIdConstants.SUB_CLAIM; | ||
|
||
public class HTTPOpenIdAuthenticator implements HTTPAuthenticator { | ||
|
||
private final static Logger log = LogManager.getLogger(HTTPOpenIdAuthenticator.class); | ||
private final Settings settings; | ||
private final Path configPath; | ||
private final int requestTimeoutMs = 10000; | ||
private final SettingsBasedSSLConfigurator.SSLConfig sslConfig; | ||
private final String userInfoEndpoint; | ||
private volatile HTTPJwtKeyByOpenIdConnectAuthenticator openIdJwtAuthenticator; | ||
|
||
public HTTPOpenIdAuthenticator(Settings settings, Path configPath) throws Exception { | ||
this.settings = settings; | ||
this.configPath = configPath; | ||
this.sslConfig = getSSLConfig(settings, configPath); | ||
userInfoEndpoint = settings.get("userinfo_endpoint"); | ||
openIdJwtAuthenticator = new HTTPJwtKeyByOpenIdConnectAuthenticator(settings, configPath); | ||
} | ||
|
||
public HTTPJwtKeyByOpenIdConnectAuthenticator getOpenIdJwtAuthenticator() { | ||
return openIdJwtAuthenticator; | ||
} | ||
|
||
public String getType() { | ||
return "oidc"; | ||
} | ||
|
||
private static SettingsBasedSSLConfigurator.SSLConfig getSSLConfig(Settings settings, Path configPath) throws Exception { | ||
return new SettingsBasedSSLConfigurator(settings, configPath, "openid_connect_idp").buildSSLConfig(); | ||
} | ||
|
||
@Override | ||
public AuthCredentials extractCredentials(SecurityRequest request, ThreadContext context) throws OpenSearchSecurityException { | ||
if (this.userInfoEndpoint != null && !this.userInfoEndpoint.isBlank()) { | ||
return extractCredentials0(request, context); | ||
} | ||
return (this.openIdJwtAuthenticator.extractCredentials(request, context)); | ||
} | ||
|
||
@Override | ||
public Optional<SecurityResponse> reRequestAuthentication(SecurityRequest request, AuthCredentials credentials) { | ||
return Optional.empty(); | ||
} | ||
|
||
/** | ||
* This method performs the logic required for making use of the userinfo_endpoint OIDC feature. | ||
* Per the spec: https://openid.net/specs/openid-connect-core-1_0.html#UserInfo there are 10 verification steps we must perform | ||
* 1. Validate the OP is correct via TLS certificate check | ||
* 2. Validate response is OK | ||
* 3. Validate application type is either JSON or JWT | ||
* 4. Validate response is one of: just signed, just encrypted, or signed then encrypted (encryption MUST NOT occur before signing) | ||
* 5. If response is signed then validate the signature via JWS and confirm the response has "iss" and "aud" claims | ||
* 6. Validate "iss" claim is equal to the issuer ID url | ||
* 7. Validate the "aud" claim is equal to the client ID | ||
* 8. If the client provides a userinfo_encrypted_response_alg value decrypt the response using the keys from registration | ||
* 9. Validate "sub" claim is always present | ||
* 10. Validate "sub" claim matches the ID token | ||
* @param request The SecurityRequest to perform auth on | ||
* @param context The active thread context | ||
* @return AuthCredentials formed through querying the userinfo_endpoint | ||
* @throws OpenSearchSecurityException On failure to extract credentials from the request | ||
*/ | ||
public AuthCredentials extractCredentials0(SecurityRequest request, ThreadContext context) throws OpenSearchSecurityException { | ||
|
||
try { | ||
|
||
URI userInfoEndpointURI = new URI(this.userInfoEndpoint); | ||
|
||
String bearerHeader = request.getHeaders().get(AUTHORIZATION).getFirst(); | ||
stephen-crawford marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!StringUtils.isBlank(bearerHeader)) { | ||
if (bearerHeader.contains("Bearer ")) { | ||
bearerHeader = bearerHeader.substring(7); | ||
} | ||
} | ||
|
||
String finalBearerHeader = bearerHeader; | ||
|
||
AccessToken accessToken = new AccessToken(AccessTokenType.BEARER, finalBearerHeader) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI Nimbus has a class called BearerAccessToken, would that be usable here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I didn't realize that. |
||
@Override | ||
public String toAuthorizationHeader() { | ||
return "Bearer " + finalBearerHeader; | ||
} | ||
}; | ||
|
||
UserInfoRequest userInfoRequest = new UserInfoRequest(userInfoEndpointURI, accessToken); | ||
|
||
HTTPRequest httpRequest = userInfoRequest.toHTTPRequest(); | ||
|
||
HTTPResponse httpResponse = httpRequest.send(); | ||
if (httpResponse.getStatusCode() < 200 || httpResponse.getStatusCode() >= 300) { | ||
throw new AuthenticatorUnavailableException( | ||
"Error while getting " + this.userInfoEndpoint + ": " + httpResponse.getStatusMessage() | ||
); | ||
} | ||
|
||
try { | ||
|
||
UserInfoResponse userInfoResponse = UserInfoResponse.parse(httpResponse); | ||
|
||
if (!userInfoResponse.indicatesSuccess()) { | ||
throw new AuthenticatorUnavailableException( | ||
"Error while getting " + this.userInfoEndpoint + ": " + userInfoResponse.toErrorResponse() | ||
Check warning on line 151 in src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPOpenIdAuthenticator.java
|
||
); | ||
} | ||
|
||
String contentType = String.valueOf(httpResponse.getHeaderValues("content-type")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extract out the UserInfoSuccessResponse here and then call on UserInfoSuccessResponse.getEntityContentType to get the content type from the request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do |
||
|
||
JWTClaimsSet claims; | ||
boolean isSigned = contentType.contains(ContentType.APPLICATION_JWT.toString()); | ||
if (isSigned) { // We don't need the userinfo_encrypted_response_alg since the | ||
// selfRefreshingKeyProvider has access to the keys | ||
claims = openIdJwtAuthenticator.getJwtClaimsSetFromInfoContent( | ||
userInfoResponse.toSuccessResponse().getUserInfoJWT().getParsedString() | ||
); | ||
} else { | ||
claims = JWTClaimsSet.parse(userInfoResponse.toSuccessResponse().getUserInfo().toString()); | ||
} | ||
|
||
String id = openIdJwtAuthenticator.getJwtClaimsSet(request).getSubject(); | ||
String missing = validateResponseClaims(claims, id, isSigned); | ||
if (!missing.isBlank()) { | ||
throw new AuthenticatorUnavailableException( | ||
"Error while getting " + this.userInfoEndpoint + ": Missing or invalid required claims in response: " + missing | ||
); | ||
} | ||
|
||
final String subject = openIdJwtAuthenticator.extractSubject(claims); | ||
if (subject == null) { | ||
log.error("No subject found in JWT token"); | ||
return null; | ||
Check warning on line 179 in src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPOpenIdAuthenticator.java
|
||
} | ||
|
||
final String[] roles = openIdJwtAuthenticator.extractRoles(claims); | ||
|
||
AuthCredentials ac = new AuthCredentials(subject, roles); | ||
|
||
for (Map.Entry<String, Object> claim : claims.getClaims().entrySet()) { | ||
ac.addAttribute("attr.jwt." + claim.getKey(), String.valueOf(claim.getValue())); | ||
} | ||
|
||
return ac.markComplete(); | ||
} catch (ParseException e) { | ||
throw new RuntimeException(e); | ||
Check warning on line 192 in src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPOpenIdAuthenticator.java
|
||
} | ||
} catch (IOException | URISyntaxException | com.nimbusds.oauth2.sdk.ParseException e) { | ||
throw new AuthenticatorUnavailableException("Error while getting " + this.userInfoEndpoint + ": " + e, e); | ||
Check warning on line 195 in src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPOpenIdAuthenticator.java
|
||
} | ||
} | ||
|
||
private String validateResponseClaims(JWTClaimsSet claims, String id, boolean isSigned) { | ||
|
||
String missing = ""; | ||
stephen-crawford marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (claims.getClaim(SUB_CLAIM) == null || claims.getClaim(SUB_CLAIM).toString().isBlank() || !claims.getClaim("sub").equals(id)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also support the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not the same. The userinfo endpoint has to have the 'sub' field specifically. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "The sub (subject) Claim MUST always be returned in the UserInfo Response. NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used." https://openid.net/specs/openid-connect-core-1_0.html#UserInfo |
||
missing = missing.concat(SUB_CLAIM); | ||
} | ||
|
||
if (isSigned) { | ||
if (claims.getIssuer() == null || claims.getIssuer().isBlank() || !claims.getIssuer().equals(settings.get(ISSUER_ID_URL))) { | ||
missing = missing.concat("iss"); | ||
} | ||
if (claims.getAudience() == null | ||
|| claims.getAudience().toString().isBlank() | ||
|| !claims.getAudience().contains(settings.get(CLIENT_ID))) { | ||
missing = missing.concat("aud"); | ||
} | ||
} | ||
|
||
return missing; | ||
} | ||
|
||
private final class HTTPJwtKeyByOpenIdConnectAuthenticator extends AbstractHTTPJwtAuthenticator { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All above is new. This internal class previously existed as the separate HttpJwtKeyByOpenIdConnectAuthenticator. I nested the class since the userinfo logic directly relies on it and the previous naming was out of convention. |
||
|
||
public HTTPJwtKeyByOpenIdConnectAuthenticator(Settings settings, Path configPath) { | ||
super(settings, configPath); | ||
} | ||
|
||
protected KeyProvider initKeyProvider(Settings settings, Path configPath) throws Exception { | ||
int idpRequestTimeoutMs = settings.getAsInt("idp_request_timeout_ms", 5000); | ||
int idpQueuedThreadTimeoutMs = settings.getAsInt("idp_queued_thread_timeout_ms", 2500); | ||
|
||
int refreshRateLimitTimeWindowMs = settings.getAsInt("refresh_rate_limit_time_window_ms", 10000); | ||
int refreshRateLimitCount = settings.getAsInt("refresh_rate_limit_count", 10); | ||
String jwksUri = settings.get("jwks_uri"); | ||
KeySetRetriever keySetRetriever; | ||
|
||
if (jwksUri != null && !jwksUri.isBlank()) { | ||
keySetRetriever = new KeySetRetriever( | ||
getSSLConfig(settings, configPath), | ||
settings.getAsBoolean("cache_jwks_endpoint", false), | ||
jwksUri | ||
); | ||
} else { | ||
keySetRetriever = new KeySetRetriever( | ||
settings.get("openid_connect_url"), | ||
getSSLConfig(settings, configPath), | ||
settings.getAsBoolean("cache_jwks_endpoint", false) | ||
); | ||
} | ||
|
||
keySetRetriever.setRequestTimeoutMs(idpRequestTimeoutMs); | ||
|
||
SelfRefreshingKeySet selfRefreshingKeySet = new SelfRefreshingKeySet(keySetRetriever); | ||
|
||
selfRefreshingKeySet.setRequestTimeoutMs(idpRequestTimeoutMs); | ||
selfRefreshingKeySet.setQueuedThreadTimeoutMs(idpQueuedThreadTimeoutMs); | ||
selfRefreshingKeySet.setRefreshRateLimitTimeWindowMs(refreshRateLimitTimeWindowMs); | ||
selfRefreshingKeySet.setRefreshRateLimitCount(refreshRateLimitCount); | ||
|
||
return selfRefreshingKeySet; | ||
} | ||
|
||
private JWTClaimsSet getJwtClaimsSet(SecurityRequest request) throws OpenSearchSecurityException { | ||
String parsedToken = super.getJwtTokenString(request); | ||
return getJwtClaimsSetFromInfoContent(parsedToken); | ||
} | ||
|
||
private JWTClaimsSet getJwtClaimsSetFromInfoContent(String userInfoContent) throws OpenSearchSecurityException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is new--how I split it out from the extract method--but the rest of the nested class was previously implemented. |
||
SignedJWT jwt; | ||
JWTClaimsSet claimsSet; | ||
try { | ||
jwt = super.jwtVerifier.getVerifiedJwtToken(userInfoContent); | ||
claimsSet = jwt.getJWTClaimsSet(); | ||
} catch (OpenSearchSecurityException | ParseException | BadCredentialsException e) { | ||
throw new RuntimeException(e); | ||
} | ||
return claimsSet; | ||
} | ||
|
||
@Override | ||
public AuthCredentials extractCredentials(SecurityRequest request, ThreadContext context) throws OpenSearchSecurityException { | ||
return super.extractCredentials(request, context); | ||
} | ||
|
||
@Override | ||
public String getType() { | ||
return "jwt-key-by-oidc"; | ||
Check warning on line 286 in src/main/java/com/amazon/dlic/auth/http/jwt/keybyoidc/HTTPOpenIdAuthenticator.java
|
||
} | ||
} | ||
} |
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.
wdyt about moving the logic that calls the userinfo endpoint into a new method?
In AbstractHttpJwtAuthenticator would it make sense to add a new method called:
That way this line can be replaced: https://github.com/opensearch-project/security/blob/main/src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java#L131
In the OpenID authenticator can we then subclass that and either return
jwt.getJWTClaimsSet()
(if userinfo endpoint is not configured) or if userinfo is configured then follow the logic that you have below.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 am not sure what the benefit of further moving things would be. We already have separation of the userinfo vs. non-userinfo logic inside this handler. If you really prefer it that way, I can move things around, but don't see how
is better than
These are functionally identical in my opinion and make the code harder to follow I feel. You can decide what you prefer though.
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 class would not need to override
extractCredentials
, it would completely re-useextractCredentials
from AbstractHTTPJwtAuthenticator, but override the implementation ofgetJWTClaimsSet
.