Skip to content

Short circuit failure handling in OIDC flow #130618

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −22
Open

Short circuit failure handling in OIDC flow #130618

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 14 additions & 22 deletions ...src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,20 @@ static void handleUserinfoResponse(
if (httpResponse.getStatusLine().getStatusCode() == 200) {
if (ContentType.parse(contentHeader.getValue()).getMimeType().equals("application/json")) {
final JWTClaimsSet userInfoClaims = JWTClaimsSet.parse(contentAsString);
validateUserInfoResponse(userInfoClaims, verifiedIdTokenClaims.getSubject(), claimsListener);
String expectedSub = verifiedIdTokenClaims.getSubject();
if (userInfoClaims.getSubject().isEmpty()) {
Copy link
Contributor

@slobodanadamovic slobodanadamovic Jul 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's cover these error cases with unit tests in OpenIdConnectAuthenticatorTests. Also, I think we could potentially get a NullPointerException here when userInfoClaims.getSubject() is not returned at all (null).

Below is a simple test I could think of to test not matching subs. Not sure if there is a test version of action listener implementation which asserts that it was called only once:

    public void testHandleUserinfoValidationFailsOnNotMatchingSubject() throws Exception {
        final ProtocolVersion httpVersion = randomFrom(HttpVersion.HTTP_0_9, HttpVersion.HTTP_1_0, HttpVersion.HTTP_1_1);
        final HttpResponse response = new BasicHttpResponse(new BasicStatusLine(httpVersion, RestStatus.OK.getStatus(), "OK"));

        final String sub = randomAlphaOfLengthBetween(4, 36);
        final String inf = randomAlphaOfLength(12);
        final JWTClaimsSet infoClaims = new JWTClaimsSet.Builder().subject("it-is-a-different-subject").claim("inf", inf).build();
        final StringEntity entity = new StringEntity(infoClaims.toString(), ContentType.APPLICATION_JSON);
        if (randomBoolean()) {
            entity.setContentEncoding(
                randomFrom(StandardCharsets.UTF_8.name(), StandardCharsets.UTF_16.name(), StandardCharsets.US_ASCII.name())
            );
        }
        response.setEntity(entity);

        final String idx = randomAlphaOfLength(8);
        final JWTClaimsSet idClaims = new JWTClaimsSet.Builder().subject(sub).claim("idx", idx).build();
        final AtomicBoolean listenerCalled = new AtomicBoolean(false);
        final PlainActionFuture<JWTClaimsSet> future = new PlainActionFuture<>() {

            @Override
            public void onResponse(JWTClaimsSet result) {
                assertTrue("listener called more than once", listenerCalled.compareAndSet(false, true));
                super.onResponse(result);
            }

            @Override
            public void onFailure(Exception e) {
                assertTrue("listener called more than once", listenerCalled.compareAndSet(false, true));
                super.onFailure(e);
            }
        };

        this.authenticator = buildAuthenticator();
        OpenIdConnectAuthenticator.handleUserinfoResponse(response, idClaims, future);
        var e = expectThrows(ElasticsearchSecurityException.class, () -> future.actionGet());

        assertThat(
            e.getMessage(),
            equalTo(
                "Userinfo Response is not valid as it is for subject [it-is-a-different-subject] while the ID Token was for subject ["
                    + sub
                    + "]"
            )
        );

    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (userInfoClaims.getSubject().isEmpty()) {
if (userInfoClaims.getSubject() == null || userInfoClaims.getSubject().isEmpty()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks catching, I rushed on this 🤦

Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to your changes, but I think we should fix it as well.

claimsListener.onFailure(new ElasticsearchSecurityException("Userinfo Response did not contain a sub Claim"));
return;
} else if (userInfoClaims.getSubject().equals(expectedSub) == false) {
claimsListener.onFailure(
new ElasticsearchSecurityException(
"Userinfo Response is not valid as it is for " + "subject [{}] while the ID Token was for subject [{}]",
userInfoClaims.getSubject(),
expectedSub
)
);
return;
}
if (LOGGER.isTraceEnabled()) {
LOGGER.trace("Successfully retrieved user information: [{}]", userInfoClaims);
}
Expand Down Expand Up @@ -527,27 +540,6 @@ static void handleUserinfoResponse(
}
}

/**
* Validates that the userinfo response contains a sub Claim and that this claim value is the same as the one returned in the ID Token
*/
private static void validateUserInfoResponse(
JWTClaimsSet userInfoClaims,
String expectedSub,
ActionListener<JWTClaimsSet> claimsListener
) {
if (userInfoClaims.getSubject().isEmpty()) {
claimsListener.onFailure(new ElasticsearchSecurityException("Userinfo Response did not contain a sub Claim"));
} else if (userInfoClaims.getSubject().equals(expectedSub) == false) {
claimsListener.onFailure(
new ElasticsearchSecurityException(
"Userinfo Response is not valid as it is for " + "subject [{}] while the ID Token was for subject [{}]",
userInfoClaims.getSubject(),
expectedSub
)
);
}
}

/**
* Attempts to make a request to the Token Endpoint of the OpenID Connect provider in order to exchange an
* authorization code for an Id Token (and potentially an Access Token)
Expand Down