[keycloak-dev] Pairwise Subject Identifier
Marek Posolda
mposolda at redhat.com
Wed Aug 31 08:14:50 EDT 2016
Yes, Thanks for sharing.
Marek
On 31/08/16 13:29, Martin Hardselius wrote:
> Just had the chance to start looking at this. Wanted to check with you
> if this is the direction you had in mind?
> public abstract class AbstractPairwiseSubGeneratorMapperextends AbstractOIDCProtocolMapperimplements OIDCAccessTokenMapper, OIDCIDTokenMapper{
>
> @Override
> public IDToken transformIDToken(IDToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, ClientSessionModel clientSession) {
> token.setSubject(generateSub(mappingModel, userSession.getUser().getId()));
> return token;
> }
>
> @Override
> public AccessToken transformAccessToken(AccessToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, ClientSessionModel clientSession) {
> token.setSubject(generateSub(mappingModel, userSession.getUser().getId()));
> return token;
> }
>
> public abstract String generateSub(ProtocolMapperModel mappingModel, String localSub);
> }
>
> On Wed, 24 Aug 2016 at 12:03 Marek Posolda <mposolda at redhat.com
> <mailto:mposolda at redhat.com>> wrote:
>
> Sounds great. Thanks!
>
>
> Marek
>
>
> On 24/08/16 11:08, Martin Hardselius wrote:
>> Sounds good. I'll look into it and see what I can do. Possibly
>> during next week. :)
>>
>> On Tue, 23 Aug 2016 at 15:38 Marek Posolda <mposolda at redhat.com
>> <mailto:mposolda at redhat.com>> wrote:
>>
>> We discussed it with Stian a bit and we discussed to:
>>
>> - Implement as protocolMapper
>>
>> - There will be possibility to configure "salt" as parameter
>> to protocolMapper. It may not be mandatory parameter. If it's
>> used, then client will use it as salt, otherwise salt will be
>> generated. This allows the use-case you mentioned. You can
>> use same salt for clients x,y,z but different salt for
>> clients m,n. If you let generate salt, then the subject
>> identifiers will be really unique just to the particular client.
>>
>> - We need to validate sector_identifier_uri during
>> create/update of protocolMapper, but also during update of
>> client. Otherwise someone can register sector_identifier_uri
>> and register the client with valid redirect_uris (for example
>> URLs "http://good-host/url1" <http://good-host/url1>
>> "http://good-host/url2" <http://good-host/url2> , which are
>> both specified under sector_identifier_uri ) but then later
>> update just the client redirect_uris to
>> "http://evil-host/url1" <http://evil-host/url1> . Basically
>> he will be able to update redirect_uris to anything he wants
>> regardless of redirect_uris under sector_identifier_uri, so
>> the whole validation will be bypassed.
>>
>> For updating client, I've just sent PR, which adds
>> RealmModel.ClientUpdatedEvent . You can register to that
>> event similarly like
>> https://github.com/keycloak/keycloak/blob/master/server-spi/src/main/java/org/keycloak/protocol/AbstractLoginProtocolFactory.java#L39
>> . The creation/update listener for protocolMapper doesn't yet
>> exists, however we are likely going to refactor
>> protocolMapper to generic component, which will have
>> validation support. But that will be likely available in few
>> weeks, so for now, we can add something temporary for
>> protocolMappers similar to what is available for example for
>> UserFederationMapperFactory (See
>> https://github.com/keycloak/keycloak/blob/master/services/src/main/java/org/keycloak/services/resources/admin/UserFederationProviderResource.java#L437-L440
>> )
>>
>> Martin, will you have some time to eventually look into this
>> and send PR ?
>>
>>
>> Marek
>>
>>
>> On 23/08/16 14:25, Martin Hardselius wrote:
>>> Let's say we want to force all of our client to use pairwise
>>> subs, but a single "merchant" needs to implement several
>>> clients where subs should remain the same for all those clients.
>>>
>>> Merchant A
>>> - client x
>>> - client y
>>> - client z
>>>
>>> Merchant B
>>> - client m
>>> - client n
>>>
>>> You can assume the sector_identifiers are the same across
>>> all clients owned by a merchant
>>>
>>> It should not be possible to correlate activities between
>>> Customer A and Customer B (at least not from their side).
>>> They should, however, be able to correlate user activities
>>> between their own clients.
>>>
>>> Which implementation of pairwise subs is better suited for
>>> supporting this scenario? I'm leaning towards the protocol
>>> mapper solution. It should be easier to create custom
>>> mappers with merchant-wide configuration (e.g salts).
>>>
>>> On Mon, 22 Aug 2016 at 22:40 Marek Posolda
>>> <mposolda at redhat.com <mailto:mposolda at redhat.com>> wrote:
>>>
>>> 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#SectorIdentifierValidation
>>>
>>>
>>> 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 at redhat.com <mailto:sthorger at redhat.com>> wrote:
>>>>
>>>> On 22 August 2016 at 14:16, Martin Hardselius
>>>> <martin.hardselius at gmail.com
>>>> <mailto:martin.hardselius at 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 at redhat.com
>>>> <mailto:sthorger at 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 at gmail.com
>>>> <mailto:martin.hardselius at 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 at redhat.com
>>>> <mailto:sthorger at 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 at gmail.com
>>>> <mailto:martin.hardselius at 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 at redhat.com
>>>> <mailto:sthorger at 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 at gmail.com <mailto:martin.hardselius at 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 at gmail.com
>>>> <mailto:martin.hardselius at 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 at redhat.com <mailto:mposolda at 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/2.1/topics/oidc/java/java-adapter-config.html
>>>>
>>>> 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 at lists.jboss.org
>>>>> <mailto:keycloak-dev at lists.jboss.org>
>>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>
>>>>
>>>> _______________________________________________
>>>> keycloak-dev mailing list
>>>> keycloak-dev at lists.jboss.org
>>>> <mailto:keycloak-dev at lists.jboss.org>
>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/keycloak-dev/attachments/20160831/d8251902/attachment-0001.html
More information about the keycloak-dev
mailing list