Re: [keycloak-dev] Pairwise Subject Identifier

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(a)redhat.com&gt; 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> <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... > . 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/o... > ) > > 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(a)redhat.com&gt; 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#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&gt; >> wrote: >> >>> On 22 August 2016 at 14:16, Martin Hardselius < >>> martin.hardselius(a)gmail.com&gt; 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(a)redhat.com&gt; >>>> 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&gt; 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&gt; >>>>>> 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&gt; 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(a)redhat.com&gt; >>>>>>>> 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&gt; 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&gt; 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&gt; >>>>>>>>>>> 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 listkeycloak-dev@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> _______________________________________________ >>>>>>>>>> keycloak-dev mailing list >>>>>>>>>> keycloak-dev(a)lists.jboss.org >>>>>>>>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>> >>>>> >> >