Re: [pve-devel] [PATCH openid 1/1] fix #4411: openid: add library code for openid groups support

[Thomas Skinner <thomas@atskinner.net>]

On Wed, Nov 13, 2024 at 6:46 AM Fabian Grünbichler
<f.gruenbichler@proxmox.com> wrote:
>
> this change actually does a lot more, right? it returns *all* claims,
> and also doesn't do anything groups specific at all, so the patch
> subject is not quite correct..

That's correct. I will update the commit message appropriately. It's
critical to return all of the claims so that the user can specify an
arbitrary claim name in the UI since a claim is not standardized
across IdPs for group names. This also allows Proxmox to be more
flexible in the future when supporting OIDC capabilities.

> I haven't tested this yet, but since all it does is allow arbitrary
> values instead of just empty ones, it should be fine. definitely would
> require some testing with different IdP implementations to ensure no
> regressions..

This change allows the ID token to contain more than just the core
claims prescribed in the openid CoreClaims struct. More importantly,
it allows any claims fields in the ID token to come through. I believe
this is not a breaking change because this is not removing any claims
from the ID token and any values that would have been passed through
OIDC would already be in some serializable format.

> On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  proxmox-openid/src/lib.rs | 55 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/proxmox-openid/src/lib.rs b/proxmox-openid/src/lib.rs
> > index fe65fded..bf8c650b 100644
> > --- a/proxmox-openid/src/lib.rs
> > +++ b/proxmox-openid/src/lib.rs
> > @@ -15,8 +15,11 @@ pub use auth_state::*;
> >  use openidconnect::{
> >      //curl::http_client,
> >      core::{
> > -        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreClient, CoreGenderClaim,
> > -        CoreIdTokenClaims, CoreIdTokenVerifier, CoreProviderMetadata,
> > +        CoreAuthDisplay, CoreAuthPrompt, CoreAuthenticationFlow, CoreErrorResponseType,
> > +        CoreGenderClaim, CoreIdTokenVerifier, CoreJsonWebKey, CoreJsonWebKeyType,
> > +        CoreJsonWebKeyUse, CoreJweContentEncryptionAlgorithm, CoreJwsSigningAlgorithm,
> > +        CoreProviderMetadata, CoreRevocableToken, CoreRevocationErrorResponse,
> > +        CoreTokenIntrospectionResponse, CoreTokenType,
> >      },
> >      AdditionalClaims,
> >      AuthenticationContextClass,
> > @@ -24,6 +27,9 @@ use openidconnect::{
> >      ClientId,
> >      ClientSecret,
> >      CsrfToken,
> > +    EmptyExtraTokenFields,
> > +    IdTokenClaims,
> > +    IdTokenFields,
> >      IssuerUrl,
> >      Nonce,
> >      OAuth2TokenResponse,
> > @@ -31,15 +37,47 @@ use openidconnect::{
> >      PkceCodeVerifier,
> >      RedirectUrl,
> >      Scope,
> > +    StandardErrorResponse,
> > +    StandardTokenResponse,
> >      UserInfoClaims,
> >  };
> >
> >  /// Stores Additional Claims into a serde_json::Value;
> > -#[derive(Debug, Deserialize, Serialize)]
> > +#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
> >  pub struct GenericClaims(Value);
> >  impl AdditionalClaims for GenericClaims {}
> >
> >  pub type GenericUserInfoClaims = UserInfoClaims<GenericClaims, CoreGenderClaim>;
> > +pub type GenericIdTokenClaims = IdTokenClaims<GenericClaims, CoreGenderClaim>;
> > +
> > +pub type GenericIdTokenFields = IdTokenFields<
> > +    GenericClaims,
> > +    EmptyExtraTokenFields,
> > +    CoreGenderClaim,
> > +    CoreJweContentEncryptionAlgorithm,
> > +    CoreJwsSigningAlgorithm,
> > +    CoreJsonWebKeyType,
> > +>;
> > +
> > +pub type GenericTokenResponse = StandardTokenResponse<GenericIdTokenFields, CoreTokenType>;
> > +
> > +pub type GenericClient = openidconnect::Client<
> > +    GenericClaims,
> > +    CoreAuthDisplay,
> > +    CoreGenderClaim,
> > +    CoreJweContentEncryptionAlgorithm,
> > +    CoreJwsSigningAlgorithm,
> > +    CoreJsonWebKeyType,
> > +    CoreJsonWebKeyUse,
> > +    CoreJsonWebKey,
> > +    CoreAuthPrompt,
> > +    StandardErrorResponse<CoreErrorResponseType>,
> > +    GenericTokenResponse,
> > +    CoreTokenType,
> > +    CoreTokenIntrospectionResponse,
> > +    CoreRevocableToken,
> > +    CoreRevocationErrorResponse,
> > +>;
> >
> >  #[derive(Debug, Deserialize, Serialize, Clone)]
> >  pub struct OpenIdConfig {
> > @@ -56,7 +94,7 @@ pub struct OpenIdConfig {
> >  }
> >
> >  pub struct OpenIdAuthenticator {
> > -    client: CoreClient,
> > +    client: GenericClient,
> >      config: OpenIdConfig,
> >  }
> >
> > @@ -120,8 +158,9 @@ impl OpenIdAuthenticator {
> >
> >          let provider_metadata = CoreProviderMetadata::discover(&issuer_url, http_client)?;
> >
> > -        let client = CoreClient::from_provider_metadata(provider_metadata, client_id, client_key)
> > -            .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
> > +        let client =
> > +            GenericClient::from_provider_metadata(provider_metadata, client_id, client_key)
> > +                .set_redirect_uri(RedirectUrl::new(String::from(redirect_url))?);
> >
> >          Ok(Self {
> >              client,
> > @@ -195,7 +234,7 @@ impl OpenIdAuthenticator {
> >          &self,
> >          code: &str,
> >          private_auth_state: &PrivateAuthState,
> > -    ) -> Result<(CoreIdTokenClaims, GenericUserInfoClaims), Error> {
> > +    ) -> Result<(GenericIdTokenClaims, GenericUserInfoClaims), Error> {
> >          let code = AuthorizationCode::new(code.to_string());
> >          // Exchange the code with a token.
> >          let token_response = self
> > @@ -206,7 +245,7 @@ impl OpenIdAuthenticator {
> >              .map_err(|err| format_err!("Failed to contact token endpoint: {}", err))?;
> >
> >          let id_token_verifier: CoreIdTokenVerifier = self.client.id_token_verifier();
> > -        let id_token_claims: &CoreIdTokenClaims = token_response
> > +        let id_token_claims: &GenericIdTokenClaims = token_response
> >              .extra_fields()
> >              .id_token()
> >              .expect("Server did not return an ID token")
> > --
> > 2.39.2
> >
> >
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> >
> >
> >
>

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel