[keycloak-dev] OIDC Discovery-enabled IdentityProvider

[Stian Thorgersen]

Adding a cache is not trivial as there's quite a few places it needs to be
added. I can't remember exactly where, but you should be able to find it by
simply searching for other caches that we define.

On Fri, 15 Feb 2019 at 03:33, James Campbell <james.p.campbell at gmail.com>
wrote:

> Thanks, Stian--the more detailed documentation inside the
> integration-arquillian folder helped understand the tests better.
>
> I've been able to read through a lot more on the tests, and I thought I
> had a picture of how to extend the current tests, but when I tried directly
> it seems that there must be configuration elements of the testing
> infrastructure itself that I'm not finding. Specifically, the code I added
> uses a new Infinispan cache to hold the configs, and I have been able to
> successfully test it via a manual build and run of keycloak. However,
> running inside the test harness gives me an error that the cache is not
> configured. I suspect I just don't know where the relevant XML and/or
> DeploymentProcessors are located for the testing framework.
>
> If there's a path to making the work I did useful, I'm game for it, but
> I'd need some more hints I'm sure to make it through the proper
> implementation of the tests.
>
> James
>
>
>
> On Wed, Feb 6, 2019 at 5:30 AM Stian Thorgersen <sthorger at redhat.com>
> wrote:
>
>>
>>
>> On Wed, 6 Feb 2019 at 02:27, James Campbell <james.p.campbell at gmail.com>
>> wrote:
>>
>>> 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).
>>>
>>
>> Is this from unit tests within the keycloak-model-infinispan module or
>> test in the Arquillian testsuite? If the latter it should pick up the
>> correct serializer. It has to be the fasterxml one otherwise the
>> annotations will be ignored.
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> 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.
>>>
>>
>> We have an Arquillian based testsuite that allows us to write functional
>> tests with all the benefits of unit tests. The testsuite allows running
>> tests from within the IDE (uses Keycloak with an embedded Undertow server)
>> and also run them against a proper built Keycloak on top of WildFly from
>> the command line.
>>
>> Some details are here:
>>
>> https://github.com/keycloak/keycloak/tree/master/testsuite/integration-arquillian
>>
>> To understand how to write the tests and run them try to run any test
>> inside testsuite/integration-arquillian/tests/base/src/test from the IDE.
>>
>> Identity brokering tests are under
>> testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker.
>> You should be able to relatively easily extend these tests to include
>> testing of OIDC well-known endpoints. We also have support in the testsuite
>> to run code directly on the server (embedded or external) to be able to
>> probe inside the server if needed. Take a look at
>> https://github.com/keycloak/keycloak/blob/master/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/runonserver/RunOnServerTest.java
>> as an example on how to do that.
>>
>>
>>
>>>
>>>
>>>
>>>>
>>>>> 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
>>>
>>