Inline...
On Tue, Feb 5, 2019 at 3:45 AM Stian Thorgersen <sthorger(a)redhat.com> wrote:
On Mon, 4 Feb 2019 at 05:14, James Campbell <james.p.campbell(a)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(a)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(a)redhat.com>
>> wrote:
>>
>>> JWKs used by identity brokering and client authentication.
>>>
>>> On Sat, 19 Jan 2019 at 23:29, James Campbell <
>>> james.p.campbell(a)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(a)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(a)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(a)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-conne...
>>>>>> >
>>>>>> >
>>>>>> > ----- 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(a)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(a)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