[feat] PIP-257: Add AuthenticationProviderOpenID

[michaeljmarshall]

PIP: #19771

Motivation

This is the primary PR for PIP 257 (#19771). It adds an OpenID Conenct AuthenticationProvider implementation. The implementation is intended to be compliant with the OpenID Specs defined here: https://openid.net/developers/specs/. We specifically implement the discovery and these two:

• OpenID Connect Core – Defines the core OpenID Connect functionality: authentication built on top of OAuth 2.0 and the use of claims to communicate information about the End-User
• OpenID Connect Discovery – Defines how clients dynamically discover information about OpenID Providers

Modifications

• Add new module pulsar-broker-auth-oidc
• Add implementation that relies on auth0 client libraries to verify the signature and claims of the JWT
• Use async http client for all http requests
• Cache the provider metadata and the JWKS results
• Support different types of FallbackDiscoveryModes, as documented in the code. Essentially, this setting allows users to more easily integrate with k8s. We need this coupling with kubernetes to deal with some of the nuances of the k8s implementation. Note that this part of the code is experimental and is subject to change as requirements and cloud provider implementations change. One important reason we use the k8s client is because the API Server requires special configuration for authentication and TLS. Since these do not appear to be generic requirements, the K8s client simplifies this integration. Here is a reference to the decision that requires authentication by default for getting OIDC info Provide OIDC discovery for service account token issuer kubernetes/kubernetes#80724. That discussion also indicate to me that this is an isolated design decision in k8s. If we find that authentication is a generic requirement, we should easily be able to expand the existing feature at a later time.
• Add metrics to help quantify success and failure. (I had thought I would add audit logging, but that is an independent feature that we can add to the Pulsar framework. It seems outside the scope of an Authentication Provider implementation to implement this feature.)

Verifying this change

There are many new tests to cover this new implementation. Some of the tests are unit tests while others are integration tests that rely on Wire Mock to return the public key information.

Documentation

• doc-required

This feature will need new docs.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#35

[lhotari]

Good work! I added some comments about dependencies and java packages.

[EronWright]

I would suggest that this validation not be performed, because the Kubernetes OpenID Discovery feature is not 100% spec-compliant. In particular, the actual issuer value may be different from the configured value. For example, here's the disco document from a GKE cluster:

$ curl -k https://kubernetes.default.svc/.well-known/openid-configuration
{
    "issuer": "https://container.googleapis.com/v1/projects/xyz/locations/us-central1/clusters/test-oidc",
    "jwks_uri": "https://35.223.170.12:443/openid/v1/jwks",
    "response_types_supported": [
        "id_token"
    ],
    "subject_types_supported": [
        "public"
    ],
    "id_token_signing_alg_values_supported": [
        "RS256"
    ]
}

Note that, when an ID token arrives at the broker, the iss claim will point to the actual value (https://container.googleapis.com/v1/projects/xyz/....).

[michaeljmarshall]

That's an interesting point of divergence. I noticed somewhat similar issues with keycloak where the iss claim on the token would change depending the DNS name you used to retrieve the token. In that case, I just added multiple issuers to ensure all possible names were trusted.

Note that, when an ID token arrives at the broker, the iss claim will point to the actual value (https://container.googleapis.com/v1/projects/xyz/....).

If the ID token's iss claim matches the issuer on the discovery document, do we need to worry that Kubernetes provides another way to get the discovery document?

[EronWright]

I take your comment to be about whether to allow for the broker configuration to simply use the well-known value of https://kubernetes.default.svc/.well-known/openid-configuration as an issuer URL, as opposed to the dynamic value as seen in the metadata doc. I would advocate for supporting the well-known value for simplicity's sake. I'd love to have a reusable broker config with in-built Kubernetes authentication.

[michaeljmarshall]

I agree that using https://kubernetes.default.svc/.well-known/openid-configuration enhances simplicity. I decided to prototype it using the kubernetes ApiClient since that takes it a step further and discovers the URL, the relevant root CA, and the appropriate authentication.

[EronWright]

Note that the value of jwt.getIssuer() here would match the actual value of the issuer field in the metadata document. It would not match the configured issuer URL; see the GKE example elsewhere in this review.

[michaeljmarshall]

To clarify, this potential discrepancy is only a problem if we remove the equality check in the verifyIssuer method, right?

[EronWright]

I agree but do think it reasonable to use the discovered issuer URL as the expected value of the iss claim.

[michaeljmarshall]

I agree but do think it reasonable to use the discovered issuer URL as the expected value of the iss claim.

This is now the case when the openIDKubernetesDiscoveryMode is DISCOVER_TRUSTED_ISSUER or DISCOVER_PUBLIC_KEYS.

I provide more explanation in my recent commits and I'll add some comments at the end of this PR.

[EronWright]

By whatever means, please use the actual value of OpenIDProviderMetadata::getIssuer() as the key into the cache, as opposed to the configured issuer value.

[michaeljmarshall]

I modified the cache a bit to suit our needs more. It probably still needs tweaking though.

[EronWright]

I didn't notice any checks around the intended audience of the token. Maybe I missed it. I believe that the Pulsar cluster should have an associated audience that all tokens would be aimed at.

[michaeljmarshall]

I didn't notice any checks around the intended audience of the token. Maybe I missed it. I believe that the Pulsar cluster should have an associated audience that all tokens would be aimed at.

@EronWright - This is present AuthenticationProviderOpenID class. Operators will configure openIDAllowedAudiences in the conf file and then the class uses that set of audiences when validating the token with this code:

        JWTVerifier verifier = JWT.require(alg)
                .acceptLeeway(acceptedTimeLeewaySeconds)
                .withAnyOfAudience(allowedAudiences)
                .withClaimPresence(RegisteredClaims.ISSUED_AT)
                .withClaimPresence(RegisteredClaims.EXPIRES_AT)
                .withClaimPresence(RegisteredClaims.NOT_BEFORE)
                .withClaimPresence(RegisteredClaims.SUBJECT)
                .build();

The auth0 library used above offers withAnyOfAudience and withAllOfAudience. I am not too opinionated about which is correct, but it seems like we want operators to specify the set of audiences they will accept and then the token should have one of those audiences.

Note that I verify the allowedAudiences set with this logic:

    /**
     * Validate the configured allow list of allowedAudiences. The allowedAudiences must be set because
     * JWT must have an audience claim.
     * See https://openid.net/specs/openid-connect-basic-1_0.html#IDTokenValidation.
     * @param allowedAudiences
     * @return the validated audiences
     */
    String[] validateAllowedAudiences(Set<String> allowedAudiences) {
        if (allowedAudiences == null || allowedAudiences.isEmpty()) {
            throw new IllegalArgumentException("Missing configured value for: " + ALLOWED_AUDIENCES);
        }
        return allowedAudiences.toArray(new String[0]);
    }

Essentially, an operator will have to supply an audience in order to use the authentication provider.

(All code snippets are from the PR in its current form. I copied them to simplify our discussion.)

[michaeljmarshall]

/pulsarbot rerun-failure-checks

[michaeljmarshall]

@lhotari @EronWright - this is ready for re-review.

[codecov-commenter]

Codecov Report

Merging #19849 (f4b18a0) into master (4f6b5fe) will increase coverage by 42.32%.
The diff coverage is 75.00%.

@@              Coverage Diff              @@
##             master   #19849       +/-   ##
=============================================
+ Coverage     30.55%   72.88%   +42.32%     
- Complexity      293    31736    +31443     
=============================================
  Files          1679     1866      +187     
  Lines        126916   137845    +10929     
  Branches      13855    15133     +1278     
=============================================
+ Hits          38783   100470    +61687     
+ Misses        82295    29376    -52919     
- Partials       5838     7999     +2161     

Flag	Coverage Δ
inttests	24.21% <1.06%> (-0.34%)	⬇️
systests	24.88% <1.06%> (-0.30%)	⬇️
unittests	72.17% <75.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rg/apache/pulsar/broker/service/BrokerService.java	80.63% <ø> (+40.44%)	⬆️
...e/pulsar/broker/authentication/oidc/JwksCache.java	56.62% <56.62%> (ø)
...hentication/oidc/AuthenticationProviderOpenID.java	72.35% <72.35%> (ø)
...thentication/oidc/OpenIDProviderMetadataCache.java	74.39% <74.39%> (ø)
...r/delayed/bucket/BucketDelayedDeliveryTracker.java	82.90% <80.00%> (+82.90%)	⬆️
.../pulsar/broker/delayed/bucket/ImmutableBucket.java	92.52% <81.81%> (+92.52%)	⬆️
...pulsar/broker/authentication/oidc/ConfigUtils.java	83.72% <83.72%> (ø)
...authentication/oidc/AuthenticationStateOpenID.java	95.00% <95.00%> (ø)
...e/bookkeeper/mledger/impl/LedgerMetadataUtils.java	100.00% <100.00%> (+21.05%)	⬆️
...thentication/oidc/AuthenticationExceptionCode.java	100.00% <100.00%> (ø)
... and 8 more

... and 1484 files with indirect coverage changes

[EronWright]

I really admire the extra effort here to be compatible with Kubernetes.

[EronWright]

For better interoperability with Azure, I recall that the issuer should be normalized in a non-standard way, that is by trimming any trailing slash, and then appending /.well-known/openid-configuration.

I found RFC 8414 that seems to be an evolution of the openid connect discovery spec.

This specification generalizes the metadata format defined by "OpenID
Connect Discovery 1.0" [OpenID.Discovery] in a way that is compatible
with OpenID Connect Discovery while being applicable to a wider set
of OAuth 2.0 use cases.

Section 3.1 gives some advice about manipulating the path, might influence your thinking. See also Section 5.

If the issuer identifier value contains a path component, any
terminating "/" MUST be removed before inserting "/.well-known/" and
the well-known URI suffix between the host component and the path
component.

[michaeljmarshall]

I've been thinking about this, and I think it makes sense to change it to just drop the / when needed. For what it's worth, the call to normalize() properly maps // to /. However, since the spec took the time to spell this out exactly, it makes sense to me to follow it exactly.

[michaeljmarshall]

Note that the OIDC discovery spec indicates the same rules in Section 4.1:

If the Issuer value contains a path component, any terminating / MUST be removed before appending /.well-known/openid-configuration. The RP would make the following request to the Issuer https://example.com/issuer1 to obtain its Configuration information, since the Issuer contains a path component:

[michaeljmarshall]

One counterpoint is that we already use this normalization method here in the client:

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/protocol/DefaultMetadataResolver.java

Lines 105 to 111 in ec102fb

	public static URL getWellKnownMetadataUrl(URL issuerUrl) {
	try {
	return URI.create(issuerUrl.toExternalForm() + "/.well-known/openid-configuration").normalize().toURL();
	} catch (MalformedURLException e) {
	throw new IllegalArgumentException(e);
	}
	}

[EronWright]

I'm finding it difficult to distinguish these cases. Why is the metadata fetched and then not used in the second case?

[michaeljmarshall]

Why is the metadata fetched and then not used in the second case?

The metadata is fetched to verify that the token's issuer is the issuer on the discovery doc provided by the k8s API Server. This verification helps identify if the token properly qualifies for the fallback issuer or if it should be rejected before verifying the token's signature.

The subsequent call to get the public keys is to the API Server's /openid/v1/jwks endpoint. This works for AKS and GKE, but not for EKS. Because the call is to the API Server, we need to use authentication and a custom ca cert. Using the k8s client is the easiest way to discover those details without requiring the user to configure anything extra.

[EronWright]

I notice that code elsewhere is verifying that the token has a sub claim, yet uses the configured role claim name at runtime. Maybe that's OK but shouldn't it verify that the role claim is present, at an earlier stage?

[michaeljmarshall]

Great catch. I'll add this as a step to token validation.

[EronWright]

It seems too laborious for a deployer to know which variant to choose between these two. I have to wonder if these could be combined into one. For example, in KUBERNETES fallback mode, the plugin would attempt to get the jwks from metadata, and if it fails then it falls back to getting the keys from the kube client.

Frankly I don't recall in which situation you'd need to use KUBERNETES_DISCOVER_PUBLIC_KEYS.

[michaeljmarshall]

It seems too laborious for a deployer to know which variant to choose between these two.

I plan to provide clear documentation explaining which to choose which setting. Because we are diverging from the OIDC discovery spec, I think it is important to provide users with information on the implementation so they can evaluate if the design meets their needs.

Essentially, it'll be a flow chart for how your k8s API Server works. EKS requires KUBERNETES_DISCOVER_TRUSTED_ISSUER right now, AKS requires KUBERNETES_DISCOVER_PUBLIC_KEYS, and GKE can do either. AWS does not work for KUBERNETES_DISCOVER_PUBLIC_KEYS because the API server does not serve the public keys.

It is possible that AWS will eventually support serving the public keys, so I don't want to make the configuration mode EKS specific. However, since the docs can easily be updated, I'll document it for now and then we can change it as the relevant options change.

Frankly I don't recall in which situation you'd need to use KUBERNETES_DISCOVER_PUBLIC_KEYS.

This is needed when we're getting the public keys from the API Server. As we discussed off line, you could avoid some of this by letting users configure their own authentication and trusted ca cert for an issuer. However, I don't see any other implementation that requires authentication to retrieve the discovery doc or the jwks. It looks like k8s only requires authentication for the OIDC endpoints because of other issues they faced: kubernetes/kubernetes#80724 (comment).

[lhotari]

LGTM, outstanding work, @michaeljmarshall

[Anonymitaet]

@michaeljmarshall thanks for bringing this great feature! I see this PR is labeled with doc-required, would you like to contribute docs? So that users will know what it is and how to use it. Feel free to ping me if you need a review, thanks!

[michaeljmarshall]

Docs added by apache/pulsar-site#555