3:40 p.m.
The question is, should we really introduce another SPI for that?
Doesn't it mean uneccessary complexity? Also add the new options
directly to the client for the feature, which is likely interesting just
for quite limited amount of people?
IMO it's fine if this is implemented as protocolMapper?
Few thoughts:
- We can have abstract superclass like
AbstractPairwiseSubGeneratorMapper . The subclasses will just need to
implement method "generateSub" . We can have just one concrete impl,
which will use SHA-256( sector_identifier || local_sub || salt )
- The sector_identifier_uri will be a configuration option of this
protocolMapper implementation.
- If protocolMapper is not added to client, the client will just use the
public subjects. By default it's not added, which ensures backwards
compatibility and public subjects by default. Note that with this
approach, we don't even need subject_type option on the client.
- The salt can be generated lazily at the first time when mapper is used.
- The validation can be done at the moment, when protocolMapper is going
to be created/updated. Right now, we don't have validation callback
during protocolMapper creation/update. However Bill is going to add
support for that into generic componentModel. So if we're going to
refactor protocolMapper to use the new generic component model, we will
have validation callback available to protocolMapper SPI. The validation
will fail if array of redirect_uri from sector_identifier_uri doesn't
contain the uris from redirect_uri of particular client (including
wildcards etc).
- If client is updated and it's redirect_uri are changed, we won't be
able to catch this, however this is not strictly required per specs per
my understanding. At least the dynamic client registration specs sais [1]
"The values registered in redirect_uris MUST be included in the elements
of the array, or registration MUST fail. This MUST be validated at
registration time; there is no requirement for the OP to retain the
contents of this JSON file or to retrieve or revalidate its contents in
the future. "
[1]
http://openid.net/specs/openid-connect-registration-1_0.html#SectorIdenti...
Marek
On 22/08/16 15:50, Martin Hardselius wrote:
Ok, thanks for the clarification.
Where would it make sense to put the PairwiseSubGeneratorSpi? Which
package, that is?
On Mon, 22 Aug 2016 at 14:51 Stian Thorgersen <sthorger(a)redhat.com
<mailto:sthorger@redhat.com>> wrote:
On 22 August 2016 at 14:16, Martin Hardselius
<martin.hardselius(a)gmail.com <mailto:martin.hardselius@gmail.com>>
wrote:
"IMO it's sufficient to document the algorithm to create the
sub, which should make it clear that the origin in the
redirect uri will affect the sub."
Reasonable. :)
"One client could also have multiple redirect uris with
different origins so could get different sub's generated
depending on the redirect uri used."
That case is probably caught by
[...] If there are multiple hostnames in the
registeredredirect_uris, the Client MUST register
asector_identifier_uri. [...]
Yes, but I meant from a documentation perspective. It should be
clear from the documentation that is the case.
On Mon, 22 Aug 2016 at 10:42 Stian Thorgersen
<sthorger(a)redhat.com <mailto:sthorger@redhat.com>> wrote:
IMO it's sufficient to document the algorithm to create
the sub, which should make it clear that the origin in the
redirect uri will affect the sub. One client could also
have multiple redirect uris with different origins so
could get different sub's generated depending on the
redirect uri used.
On 22 August 2016 at 09:58, Martin Hardselius
<martin.hardselius(a)gmail.com
<mailto:martin.hardselius@gmail.com>> wrote:
Sounds fair enough.
What about the case where you don't provide a
sector_identifier_uri, set up a single redirect uri on
myhost and then later go on to change that redirect
uri to something on myotherhost? That would change the
sector_identifier and subsequently all the user subs.
Do we protect against such "mistakes" or do we warn
people (in the docs and/or admin gui) that it's not a
good idea to do that?
On Mon, 22 Aug 2016 at 09:38 Stian Thorgersen
<sthorger(a)redhat.com <mailto:sthorger@redhat.com>> wrote:
We need to follow the spec and verify that
sector_identifier_uri points to a URL that
contains the corresponding URIs. As an enhancement
we could support wildcards in the JSON returned
by sector_identifier_uri. For example if it returns:
[https://www.mydomain.com/*,
https://www.myotherdomain.com/* ]
A client with the redirect uri
'https://www.myotherdomain.com/myapp' would work
On 18 August 2016 at 15:09, Martin Hardselius
<martin.hardselius(a)gmail.com
<mailto:martin.hardselius@gmail.com>> wrote:
Speaking of subject_identifier_uri
From the spec:
"When asector_identifier_uriis provided, the
host component of that URL is used as the
Sector Identifier for the pairwise identifier
calculation. The value of
thesector_identifier_uriMUST be a URL using
thehttpsscheme that points to a JSON file
containing an array ofredirect_urivalues. The
values of the registeredredirect_urisMUST be
included in the elements of the array."
What's your stance on sanity/health checking
the sector_identifier_uri? URI validation is
one thing, but should we also make a request
to the uri in order to validate the response?
The spec also mentions that the
sector_identifier_uri isn't strictly required
if a client has all it's redirect_uris under
the same domain. How do we deal with changes
to clients if the sector_identifier_uri isn't
required for enabling pairwise subs?
Example:
I create a client, enabling pairwise subs.
Valid redirect_uris are [
https://www.mydomain.com/* ]. The
sector_identifier would be mydomain.
Later on, I update the redirect_uris to
[https://www.mydomain.com/*,
https://www.myotherdomain.com/* ] Do we
* keep the old sector_identifier, or
* reject the update, or
* allow the update if a valid
subject_identifier_uri is provided at mydomain, or
* just allow it and let the client devs deal
with the consequences, or
* take some other path you can think of ?
Having the sector_identifier_uri as a hard
requirement seems safer, but it's also means
more work implementing a client. Leaving it
optional is a lot more scary.
On Thu, 18 Aug 2016 at 10:45 Stian Thorgersen
<sthorger(a)redhat.com
<mailto:sthorger@redhat.com>> wrote:
Makes sense to make this pluggable. The
default should probably SHA-256(
sector_identifier || local_sub || salt ).
We would love a PR for this, but there's a
few bits required:
* OIDC clients need option for subject
type and sector_identifier_uri. If not set
it should default to public so existing
clients continue to work. These options
can just be set as client attributes so
there's no need to update db schema
* Admin console update for the above
* PairwiseSubGeneratorSpi and default
implementation
* Generation of salt for clients. This
should be done when a client sets subject
type to pairwise
* Tests and docs
I'd say the PairwiseSubGeneratorSpi
signature should probably be:
* public String getPairwiseSub(UserModel
user, ClientModel client)
It might even be an option to let the
default PairwiseSubGenerator provider
create the salt and store it as an
attribute on the client, making that part
pluggable as well.
On 17 August 2016 at 15:59, Martin
Hardselius <martin.hardselius(a)gmail.com
<mailto:martin.hardselius@gmail.com>> wrote:
I'm going to bump this, as I want to
continue the discussion/provide some
input.
Does it make sense to support more
than type of pairwise subject
identifier generator? E.g through a
PairwiseSubGeneratorSpi?
Let's say I want to generate the
pairwise sub as a salted hash: sub =
SHA-256( sector_identifier ||
local_sub || salt )
To me, it makes sense to allow for
per-client salts. These salts should
probably be generated and persisted
during client creation. Thoughts?
On Fri, 12 Aug 2016 at 09:57 Martin
Hardselius
<martin.hardselius(a)gmail.com
<mailto:martin.hardselius@gmail.com>>
wrote:
Thank you for your response. Did
not see that ticket before. Great
news!
I looked into using protocol
mappers to achieve this, and while
it would work I'm worried that
once KEYCLOAK-3422 has been
resolved and included in a proper
release we would run into
migration issues if the method
used for calculating "native"
pairwise subs is different from
what we implement. Clients could
loose / be forced to re-register
all their users if we decide to
switch. The example methods in the
spec are just that. Examples.
Maybe the method/alg for computing
the pairwise sub should be pluggable?
--
Martin
On Thu, 11 Aug 2016 at 17:15 Marek
Posolda <mposolda(a)redhat.com
<mailto:mposolda@redhat.com>> wrote:
Sorry for late response.
We have JIRA created for that.
You can possibly add yourself
as a watcher. See
https://issues.jboss.org/browse/KEYCLOAK-3422
Maybe an alternative for you
is to use protocolMappers.
That should allow you to
"construct" the token for
particular client exactly how
you want and also use the
different value for "sub" claim.
Another possibility is, to
handle this on adapter side.
We already have an adapter
option "principal-attribute",
which specifies that
application will see the
different attribute instead of
"sub" as subject. For example
when in appllication you call
"httpServletRequest.getRemoteUser()"
it will return "john" instead
of "123456-unique-johns-uuid"
. See
https://keycloak.gitbooks.io/securing-client-applications-guide/content/v...
Hopefully some of the options
can be useful for you?
Marek
On 02/08/16 14:13, Martin
Hardselius wrote:
> Me and my team are working
> towards getting Keycloak,
> customized for our needs,
> into production but we've
> identified the need for
> Pairwise Subject Identifiers
> as we don't want to expose
> internal user ids.
>
> Right now, the only
> subject_types_supported seems
> to be "public". Are there any
> near-future plans to include
> "pairwise"? Can we pitch in
> with a PR to make this happen
> as soon as possible?
>
> Links to relevant sections in
> the spec:
>
>
http://openid.net/specs/openid-connect-core-1_0.html#SubjectIDTypes
>
http://openid.net/specs/openid-connect-core-1_0.html#PairwiseAlg
>
> --
> Martin
>
>
>
_______________________________________________
> keycloak-dev mailing list
> keycloak-dev(a)lists.jboss.org
> <mailto:keycloak-dev@lists.jboss.org>
>
https://lists.jboss.org/mailman/listinfo/keycloak-dev
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org
<mailto:keycloak-dev@lists.jboss.org>
https://lists.jboss.org/mailman/listinfo/keycloak-dev