[keycloak-dev] Pairwise Subject Identifier

[Martin Hardselius]

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> 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> 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>
>> wrote:
>>
>>> On 22 August 2016 at 14:16, Martin Hardselius <
>>> 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 registered redirect_uris,
>>>> the Client MUST register a sector_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>
>>>> 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> 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>
>>>>>> 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> wrote:
>>>>>>>
>>>>>>>> Speaking of subject_identifier_uri
>>>>>>>>
>>>>>>>> From the spec:
>>>>>>>>
>>>>>>>> "When a sector_identifier_uri is provided, the host component of
>>>>>>>> that URL is used as the Sector Identifier for the pairwise identifier
>>>>>>>> calculation. The value of the sector_identifier_uri MUST be a URL
>>>>>>>> using the https scheme that points to a JSON file containing an
>>>>>>>> array of redirect_uri values. The values of the registered
>>>>>>>> redirect_uris MUST 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>
>>>>>>>> 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> 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> 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>
>>>>>>>>>>> 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 listkeycloak-dev at lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> keycloak-dev mailing list
>>>>>>>>>> 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/20160824/9a3729dd/attachment-0001.html