[keycloak-dev] OIDC Discovery-enabled IdentityProvider

James Campbell james.p.campbell at gmail.com
Tue Feb 5 20:27:07 EST 2019 

Inline...

On Tue, Feb 5, 2019 at 3:45 AM Stian Thorgersen <sthorger at redhat.com> wrote:

>
>
> 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.
>

Makes sense, thank you.


>
>> 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.
>

The unknown field in this case was authorization_endpoint (so yes, mapped,
and in code that I did not change). I wondered whether perhaps the issue is
that I was doing this from inside the keycloak-model-infinispan module, and
a conflicting JSON parser dependency is in play? I did try adding
the @JsonIgnoreProperties annotation as well, to no avail. My suspicion is
just based on (1) the current class correctly supporting deserialization in
existing tests, and (2) a search of common causes for this error (which
I've not encountered before).


>
>
>>
>> 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.
>
>

Definitely makes sense; I just didn't see any documentation on the testing
framework at all, whereas there is documentation on the core server
development components. Happy to either take a look at docs you flag, or to
work from any recommendations you have (I certainly wasn't thinking a PR
would be ready without tests!). The current testing system is pretty opaque
to me.



>
>> 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
>>
>

-- 
-... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.- / -...
. / ..-. .. -
07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70

More information about the keycloak-dev mailing list