michaeljmarshall commented on pull request #11794: URL: https://github.com/apache/pulsar/pull/11794#issuecomment-912267604
Thanks for your response @eolivelli. > Probably having some high level description of this feature, would help in understanding why this feature is so useful and why we should add it to Pulsar core. Yes, I think we need more definition for the problem that @MathiasHaudgaard would like to solve with this PR. > Maybe it is just me, that I do not know **JWKS** and so I do not see the value in adding it to the core distribution, but I see more third party dependencies added to the core distribution. The `JWKs` data structure is defined in this RFC: https://datatracker.ietf.org/doc/html/rfc7517. The data structure can hold several kinds of cryptographic information, like public keys or private keys. Here is a helpful quote from the introduction: > JWKs and JWK Sets are used in the JSON Web Signature [JWS] and JSON Web Encryption [JWE] specifications. I don't know all of the use cases of the JWK and JWKs data structure. I know it is used in the OpenID Connect protocol, an extension of the OAuth2.0 protocol, to store public keys that can be used to verify the signature on a JWT. An Identity Provider implementing the OIDC protocol will issue JWTs for an authenticated entity. These tokens will be signed by the Identity Provider using a private key. The Identity Provider will serve the paired Public Keys at an endpoint on the server to make it easy to verify token signatures. The PR currently uses the JWKs endpoint to retrieve a single Identity Provider's public keys to verify the signature of the JWT. For reference, the current `AuthenticationProviderToken` class verifies a JWT in the same way that this new class verifies a token (although each uses different libraries). The `AuthenticationProviderToken` class relies on a public key that is available in to the broker's file system. This implementation is limited because it allows for only one active public key, which means that to rotate keys requires downtime: first to distribute new tokens signed by a new private key and then to update the paired public key in the broker. By adding support to retrieve a `JWKs` from an Identity Provider, we will gain the ability to have multiple valid public keys at the same time. That means it'll be possible to rotate keys without downtime. Further, because the public keys are discovered, operators will be able to remove public keys in the event that some part of the chain were compromised. Ultimately, this PR would make it much easier for end users to integrate with an independent Identity Provider service, which is a great feature. However, from my perspective, I think this PR does not go far enough to implement a specific authentication protocol. I know that the JWKS endpoint is used in OpenID Connect. I'm not sure if it is used in any other authentication protocols. I think I would prefer to see a complete implementation of the OpenID Connect protocol (or some other protocol) instead of a partial implementation. The main features that I currently see missing in this implementation (assuming we want to implement the OpenID Connect protocol): 1. Discover the issuer's `jwks_uri` by retrieving it at the issuer's `/.well-known/openid-configuration` endpoint (https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata) 2. Implement configurable caching of public keys (https://openid.net/specs/openid-connect-core-1_0.html#RotateEncKeys) 3. The trusted issuers should be required to use `https`, by default (it could be optional to override this requirement). This is technically required in the protocol: https://openid.net/specs/openid-connect-discovery-1_0.html. 4. (Optional) Allow for multiple issuers to be considered "allowed" or "trusted". This is convenient if you're running your identity provider in the same kubernetes cluster due to the way that k8s DNS works. (This component could be debatable, but in the event that we implement number 1, it'd be pretty easy to allow for this implementation too.) 5. (Optional) Allow for more algorithms than just `RS256`. Available algorithms are defined here: https://datatracker.ietf.org/doc/html/rfc7518#section-3.1. (This would be easy to add later, so it doesn't need to be a priority now.) If we do implement the OpenID Connect protocol (or even some other protocol), I think it would certainly deserve to be in the core distribution. @eolivelli - I think the question of which dependencies to use is a valid one that needs to be answered. I am not sure of the right answer, but I do think it'd be worth adding dependencies to get full support of the OpenID Connect protocol. The current 3rd party libraries from `auth0` do not support retrieving the the provider metadata from the `/.well-known/openid-configuration` endpoint. Further, the `auth0` library does not use nonblocking asynchronous IO. It'd be great if we could find a library that offers this. I'm not sure if one of these exists for OIDC. Finally, I think we should resolve the asynchronous authentication provider issue that I raised on the dev mailing list before we can merge this PR. Otherwise, any network calls made by this provider will block netty event loop threads. @MathiasHaudgaard - what do you think about my comments? Do you agree that this is meant to implement the OpenID Connect protocol or were you seeking to implement something different? I am not an expert on uses of the JWKS, so your insight will be helpful. @eolivelli - I hope my comment has helped explain some of the context for this PR and its significance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
