[keycloak-dev] Pairwise Subject Identifier
Marek Posolda
mposolda at redhat.com
Wed Aug 24 06:03:14 EDT 2016
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/20160824/a8367d3a/attachment-0001.html
More information about the keycloak-dev
mailing list