[keycloak-dev] OIDC Discovery-enabled IdentityProvider

Stian Thorgersen sthorger at redhat.com
Tue Feb 5 03:45:38 EST 2019 

On Mon, 4 Feb 2019 at 05:14, James Campbell <james.p.campbell at gmail.com>
wrote:

> All--
>
> Thanks to Stian's recommendations for similar features in the codebase,
> I've developed a proposal for how to implement this feature, and sketched
> out an initial implementation. As he suggested, I'd like to get feedback
> before opening up a PR.
>
> 1. We introduce a new subclass of the OIDCIdentityProvider / Factory
> called OIDCDiscoveryIdentityProvider. The new class adds a discoverConfig
> method that can be used to discover and set all relevant endpoints in a
> configuration except the issuer. Issuer then becomes the sole required
> element of the config. (So the GoogleIdentityProvider, for example, will
> then just call discoverConfig with its hard-coded issuer).
>    -> During the parseConfig call, the Factory will raise a
> RuntimeException if the config cannot be obtained, since if the config can
> never be obtained there is no way to have a valid configuration at all.
>    -> During the discoverConfig call, we return the cached value, and
> optionally refresh it based on when we last obtained the configuration.
>    -> In the event of a failure in the *refresh* I'm imagining the best
> behavior is to log a warning but then return the cached config (last known
> good config).
>

This should just be added to the OIDCIdentityProvider directly rather than
a subclass. Perhaps we should have an option to configure how frequently
the configuration is reloaded.


>
> 2. We introduce an SPI and interface called
> OIDCDiscoveryRepresentationProvider / Factory and implementations called
> Infinispan... which obtains OIDCConfigurationRepresentations using the
> /.well-known/openid-configuration endpoint on the issuer.
>
> I have a couple questions:
>
> 1. I began an implementation of this, but the @JsonProperty annotations on
> the existing OIDCCOnfigurationRepresentation class seem to be ignored when
> I try to have it read from the openid-configuration docuement, e.g.:
>   - OIDCConfigurationRepresentation rep = SimpleHttp.doGet(issuer +
> "/.well-known/openid-configuration",
> session).asJson(OIDCConfigurationRepresentation.class); // FAILS with
> "Unrecognized field ..." Perhaps a mismatch of the jackson annotation
> version in the keycloak-model-infinispan submodule?
>

Is the unknown field actually mapped or is the ignore unknown fields option
enabled? The ignore unknown fields should probably be enabled as there may
be unknown elements in the config.


>
> 2. Assuming the design above makes sense, what is the right way to plug
> this into the testing framework? Is one of the core developers interested
> in tackling that aspect if I push a PR with the above features implemented?
>

We are very reluctant to receive PRs from the community that do not include
tests. Think about it, would you as a developer like to implement tests for
other developers? We can help give you recommendations on how to test it
though.


>
> James
>
> On Sun, Jan 27, 2019 at 11:15 AM James Campbell <
> james.p.campbell at gmail.com> wrote:
>
>> Got it; I checked that code out and it does look like a pretty direct
>> model for the refreshing feature. I'll begin working on a PR.
>>
>> On Mon, Jan 21, 2019 at 3:25 AM Stian Thorgersen <sthorger at redhat.com>
>> wrote:
>>
>>> JWKs used by identity brokering and client authentication.
>>>
>>> On Sat, 19 Jan 2019 at 23:29, James Campbell <james.p.campbell at gmail.com>
>>> wrote:
>>>
>>>> Stian--
>>>>
>>>> Thanks for confirming. Which keys are you referring to--I'll take a
>>>> look at that in the code and try to follow that pattern closely.
>>>>
>>>> James
>>>>
>>>> On Wed, Jan 16, 2019 at 1:52 AM Stian Thorgersen <sthorger at redhat.com>
>>>> wrote:
>>>>
>>>>> It would be good to get these changes included. We do something
>>>>> similar for the keys today and they are cached in an Infinispan local
>>>>> cache. Could do something similar for the OIDC discovery/config.
>>>>>
>>>>> On Tue, 15 Jan 2019 at 17:44, James Campbell <
>>>>> james.p.campbell at gmail.com> wrote:
>>>>>
>>>>>> Tomas--
>>>>>>
>>>>>> Thanks--it certainly seems close, you're right! It looks, however
>>>>>> like an
>>>>>> OIDC provider still uses a static configuration even though it loads
>>>>>> it
>>>>>> from the discovery URL--that is, once it's loaded at configuraiton
>>>>>> time, it
>>>>>> doesn't discover new changes, and there isn't an option to
>>>>>> refresh/store
>>>>>> that discovery endpoint outside of configuration.
>>>>>>
>>>>>> It's not clear to me how important that feature is--on one hand, it
>>>>>> seems
>>>>>> unlikely that we should expect frequent changes; on the other, in the
>>>>>> short
>>>>>> time since I started exploring this setup, I have encountered three
>>>>>> changes
>>>>>> in google's OIDC endpoints between what was hard-coded into keycloak,
>>>>>> what
>>>>>> is in their documentation, and what their current discovery endpoint
>>>>>> provides. (And, the google docs specifically suggest refreshing from
>>>>>> the
>>>>>> endpoint periodically).
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>> James
>>>>>>
>>>>>>
>>>>>> On Tue, Jan 15, 2019 at 10:31 AM Tomas Kyjovsky <tkyjovsk at redhat.com>
>>>>>> wrote:
>>>>>>
>>>>>> > Please disregard my previous message.
>>>>>> >
>>>>>> > Looking at the docs [1] and the Admin Console UI this should be
>>>>>> already
>>>>>> > possible with the OIDC identity provider.
>>>>>> > When creating a OIDC identity provider in the Admin Console there
>>>>>> is an
>>>>>> > option at the bottom of the page to import OIDC configuration
>>>>>> metadata from
>>>>>> > URL.
>>>>>> >
>>>>>> > Does this cover your use case?
>>>>>> >
>>>>>> >
>>>>>> > Regards,
>>>>>> > Tomas
>>>>>> >
>>>>>> > [1]
>>>>>> >
>>>>>> https://www.keycloak.org/docs/latest/server_admin/index.html#openid-connect-v1-0-identity-providers
>>>>>> >
>>>>>> >
>>>>>> > ----- Original Message -----
>>>>>> > > Hello James,
>>>>>> > >
>>>>>> > > See my 2 cents inline..
>>>>>> > >
>>>>>> > > ----- Original Message -----
>>>>>> > > > All--
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > After observing that the Google Social Identity Provider in
>>>>>> Keycloak
>>>>>> > was
>>>>>> > > > using a deprecated userprofile endpoint [
>>>>>> > > > <
>>>>>> https://issues.jboss.org/projects/KEYCLOAK/issues/KEYCLOAK-9179>
>>>>>> > > > Keycloak-9179,  <https://issues.jboss.org/browse/KEYCLOAK-9169>
>>>>>> > > > Keycloak-9169], I wanted to propose the creation of an
>>>>>> IdentityProvider
>>>>>> > > > that
>>>>>> > > > will use the OIDC Discovery mechanism to dynamically build a
>>>>>> config [
>>>>>> > > > <https://issues.jboss.org/browse/KEYCLOAK-9194> Keycloak-9194].
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > I see a few decision points along the way that I wanted to ask
>>>>>> about
>>>>>> > before
>>>>>> > > > an implementation, since I'm very new to keycloak and just
>>>>>> starting to
>>>>>> > > > understand the codebase. In particular, I wondered if this
>>>>>> group could
>>>>>> > > > share
>>>>>> > > > insight into these couple issues so I can make a more informed
>>>>>> design:
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > 1.    It looks to me like the actual IdentityProviders are
>>>>>> instantiated
>>>>>> > > > just
>>>>>> > > > as they're being used, but that the models are persisted in the
>>>>>> > RealmModel.
>>>>>> > > > It's not clear to me where the separation of concerns is
>>>>>> supposed to be
>>>>>> > > > between the IdentityProvider and the IdentityProviderModel-in
>>>>>> > particular
>>>>>> > > > since the GoogeIdentityProvider, say, immediately sets
>>>>>> endpoints in its
>>>>>> > > > constructor. Normatively, where *should* social identity
>>>>>> providers'
>>>>>> > model
>>>>>> > > > configuration be set (and, e.g., where are the models first
>>>>>> added to
>>>>>> > the
>>>>>> > > > RealmModel)?
>>>>>> > >
>>>>>> > > Provider classes are being instantiated per transaction by their
>>>>>> > > corresponding ProviderFactories and then left to be
>>>>>> garbage-collected
>>>>>> > after
>>>>>> > > Provider.close() is called.
>>>>>> > > The Provider class is given its configuration
>>>>>> (IdentityProviderModel in
>>>>>> > this
>>>>>> > > case) by its factory which I believe loads it from cache/jpa
>>>>>> layer. Any
>>>>>> > > class extending AbstractIdentityProvider should then be able to
>>>>>> access
>>>>>> > its
>>>>>> > > config via getConfig() method but I don't think it will be able to
>>>>>> > > update/persist it back. The provider configuration/model itself is
>>>>>> > managed
>>>>>> > > by the IdentityProviderResource (REST endpoint accessible via
>>>>>> REST or
>>>>>> > admin
>>>>>> > > console UI) in the keycloak/services module so I think the
>>>>>> > > auto-configuration logic would have to be placed somewhere there.
>>>>>> > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > 2.    I see that there is logic to parse OIDC Discovery
>>>>>> configuration
>>>>>> > as
>>>>>> > > > part of configuring Keycloak itself as an OIDC provider /
>>>>>> implementer
>>>>>> > of
>>>>>> > > > OIDC protocol (including building and parsing the .well-known
>>>>>> config
>>>>>> > > > elements), but that logic seems not to be used in any setting
>>>>>> > currently as
>>>>>> > > > a
>>>>>> > > > client. Should I plan to reuse, say, the
>>>>>> > OIDCConfigurationRepresentation
>>>>>> > > > and
>>>>>> > > > OIDCWellKnownProvider classes for their logic in handling such
>>>>>> configs?
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > Currently, I'm imagining something along the lines of extending
>>>>>> > > > OIDCIdentityProvider with a new OIDCDiscoveryIdentityProvider
>>>>>> that
>>>>>> > adds a
>>>>>> > > > discoverConfig method which can be used by an implementing
>>>>>> class (such
>>>>>> > as
>>>>>> > > > GoogleIdentityProvider) to discover and cache endpoints such
>>>>>> that they
>>>>>> > are
>>>>>> > > > not hard-coded into the implementing class. Each implementing
>>>>>> class
>>>>>> > would
>>>>>> > > > then have a public static final DISCOVERY_URL that it passes to
>>>>>> > > > discoverConfig.
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > My main hangup, as suggested above, is that to implement the
>>>>>> caching, I
>>>>>> > > > want
>>>>>> > > > to ensure that the model configuration is stored/set in the
>>>>>> right
>>>>>> > place.
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > Thanks for bearing with me as I come up to speed on this!
>>>>>> > > >
>>>>>> > > >
>>>>>> > > >
>>>>>> > > > James
>>>>>> > > >
>>>>>> > > > _______________________________________________
>>>>>> > > > keycloak-dev mailing list
>>>>>> > > > keycloak-dev at lists.jboss.org
>>>>>> > > > https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>>> > > >
>>>>>> > >
>>>>>> > >
>>>>>> > > Regards,
>>>>>> > > Tomas
>>>>>> > >
>>>>>> >
>>>>>>
>>>>>>
>>>>>> --
>>>>>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.- /
>>>>>> -...
>>>>>> . / ..-. .. -
>>>>>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>>>>> _______________________________________________
>>>>>> keycloak-dev mailing list
>>>>>> keycloak-dev at lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>>>
>>>>>
>>>>
>>>> --
>>>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.- /
>>>> -... . / ..-. .. -
>>>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>>>
>>>
>>
>> --
>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.- /
>> -... . / ..-. .. -
>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>
>
>
> --
> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.- / -...
> . / ..-. .. -
> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>

More information about the keycloak-dev mailing list