Skip to content
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

Support OIDC Client JWT Bearer authentication #38541

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

@sberyozkin sberyozkin requested a review from pedroigor February 1, 2024 19:37
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/oidc labels Feb 1, 2024
Copy link

github-actions bot commented Feb 1, 2024

🙈 The PR is closed and the preview is expired.

@sberyozkin sberyozkin force-pushed the oidc_client_jwt_bearer_auth branch from 95ac52d to 27e68da Compare February 2, 2024 00:31
@sberyozkin sberyozkin force-pushed the oidc_client_jwt_bearer_auth branch from 27e68da to 5f8d63f Compare February 6, 2024 14:08
@sberyozkin sberyozkin marked this pull request as ready for review February 6, 2024 14:24
Copy link

quarkus-bot bot commented Feb 6, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 5f8d63f.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other check runs running, make sure you don't need to wait for their status before merging.

Copy link

quarkus-bot bot commented Feb 6, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5f8d63f.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Copy link
Contributor

@pedroigor pedroigor left a comment

Choose a reason for hiding this comment

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

LGTM, @sberyozkin.

@sberyozkin
Copy link
Member Author

Thanks @pedroigor, that should help with picking up other dynamically generated sensitive data the secrets you mentioned etc

@sberyozkin sberyozkin merged commit 44672eb into quarkusio:main Feb 12, 2024
39 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 12, 2024
@sberyozkin sberyozkin deleted the oidc_client_jwt_bearer_auth branch February 12, 2024 14:50
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Feb 12, 2024
@Sopka
Copy link

Sopka commented Feb 15, 2024

I've tested the current 999-SNAPSHOT version and it works, but only If I add the client id again...
I do think that the OidcClientImpl class should add

body.add(OidcConstants.CLIENT_ID, oidcConfig.clientId.get());

at the end of new clode block for } else if (jwtBearerAuthentication) { after line 149

My current workaround is to override my custom filter method like so:

  @Override
  protected Map<String, String> additionalParameters() {
      return Map.of(OidcConstants.CLIENT_ASSERTION, getFederatedToken(),
          OidcConstants.CLIENT_ID, this.clientId);
  }

BTW: Without setting the client-id again in the additionalParameters Map, Azure was giving me the error AADSTS90023: Unexpected client assertion type. This is really misleading, because the assertion type was correct all the time, but actually the client-id was missing.

@Sopka
Copy link

Sopka commented Feb 20, 2024

There is still another problem with the current snapshot implementation.

After a while, I guess when the token gets renewed after it expired, the filter passes multiple times the client_assertion_type parameter. Especially the OIDC provider at Azure only expects the parameter exactly once.

The response from the OIDC provider is than correctly:

{"error":"invalid_request","error_description":"AADSTS90023: Unexpected client assertion type. ...}

Since the actual requests to the OIDC provider cannot be logged by enablng some application.properties I've added another custom Filter to debug what actually got sent to the provider:

import org.eclipse.microprofile.config.inject.ConfigProperty;
import org.jboss.logging.Logger;

import io.quarkus.arc.Unremovable;
import io.quarkus.oidc.common.OidcRequestContextProperties;
import io.quarkus.oidc.common.OidcRequestFilter;
import io.vertx.mutiny.core.buffer.Buffer;
import jakarta.enterprise.context.ApplicationScoped;

@ApplicationScoped
@Unremovable
public class OidcRequestDumpFilter implements OidcRequestFilter {

  private static final Logger LOG = Logger.getLogger(OidcRequestDumpFilter.class);

  @ConfigProperty(name = "oidc.request.dump.enabled")
  boolean requestDumpEnabled;

  @Override
  public void filter(io.vertx.mutiny.ext.web.client.HttpRequest<Buffer> request, Buffer requestBody,
      OidcRequestContextProperties contextProperties) {
    if (requestDumpEnabled) {
      LOG.info(dumpRequest(request.headers().entries(), requestBody == null ? "" : requestBody.toString()));
    }
  }

  String dumpRequest(final List<Map.Entry<String, String>> headers, final String requestBody) {
    final StringBuilder dumpBuilder = new StringBuilder();
    dumpBuilder.append("Headers:\n");
    headers.forEach(
        header -> dumpBuilder.append("  ").append(header.getKey()).append(": ").append(header.getValue()).append("\n"));
    dumpBuilder.append("Body:\n  ").append(requestBody);
    return dumpBuilder.toString();
  }

}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Support OIDC Client JWT Bearer authentication
3 participants