When an Identity Provider gives a different UserID for an existing Federated Identity, IdentityBrokerService creates a duplicate and fails #20677
Conversation
ghost
mentioned this pull request
pedroigor
force-pushed
the
bug/duplicate_federated_identity_in_identity_broker_service
branch
from
e83de22
to
1349764
Compare
|
@pedroigor I know this is normally your text but wouldnt a test be nice for this? |
|
@pedroigor We've run into this issue also, but I'm not sure updating the IdP link in place is quite right, either. The problem is the unique constraint is maybe not quite right. It's quite possibly that multiple users coming from the same identity provider link to one user in Keycloak. IdPs do not all share the same cardinality of user identity. You might have two distinct "users" (such as users in different tenants or contexts) in one IdP that federates with Keycloak, where in Keycloak you only have one user. Updating the link implies the old link should be removed, but that's not necessarily always the case. At least I would suggest this behavior can be controlled with a config option. Perhaps ideally we were not limited to a single user from one brokered IdP, but I believe changing that cardinality would require breaking changes to a number of APIs. (We are currently working around this by creating separate identity providers in Keycloak with duplicate configuration, so there is still only 1 link per identity provider as far as Keycloak thinks, even though they are all the same external identity provider.) |
|
@looorent @pedroigor -- Then we need to update the code where we query the federated identity with an additional WDYT @mposolda |
|
I called that out as a way to understand the semantics of the problem. It might not be realistic because changing the uniqueness constraint and cardinality of the relationship would be a breaking change. |
pedroigor
force-pushed
the
bug/duplicate_federated_identity_in_identity_broker_service
branch
from
1349764
to
6f77073
Compare
|
@looorent LGTM but we are still missing tests. I'm looking at how we can build a test scenario for it in our test suite. I think I understand the problem (mainly after reading #9209 (comment)) but not sure yet how to write the test. |
ghost
mentioned this pull request
|
@pedroigor Thanks. I sadly do not have time at the moment to write this test. |
|
Having the config option for control this as mentioned by @alechenninger might be good. I can imagine options like:
However for the time being, this PR helps with the problem - at least for some deployments. So I agree that we can go with the approach in this PR for now and then see if we need to add support for "Allow create duplicated federated link" as a follow-up (This would be probably a bigger task). WDYT? Regarding testing of this PR - I think it can be possible. We need to make sure that user, who has existing federatedLink in realm |
|
Hi @mposolda |
|
Hi I just finished the test.
Or should I create a new one? |
|
@lexcao I have added you as contributor. |
lexcao
force-pushed
the
bug/duplicate_federated_identity_in_identity_broker_service
branch
from
6f77073
to
9330ecf
Compare
|
Thanks, @looorent |
lexcao
force-pushed
the
bug/duplicate_federated_identity_in_identity_broker_service
branch
from
9330ecf
to
88dc2b9
Compare
lexcao
force-pushed
the
bug/duplicate_federated_identity_in_identity_broker_service
branch
2 times, most recently
from
2a3b2d0
to
0ff16c1
Compare
|
Hi @pedroigor |
|
@lexcao Glad to see you're back. Thanks. |
ghost
mentioned this pull request
…derated Identity, IdentityBrokerService creates a duplicate and fails. Closes keycloak#9209 Signed-off-by: Lex Cao <lexcao@foxmail.com>
lexcao
force-pushed
the
bug/duplicate_federated_identity_in_identity_broker_service
branch
from
0ff16c1
to
d194bed
Compare
ghost
mentioned this pull request
|
|
||
| federatedIdentity.setUserId(federatedIdentityModel.getUserId()); |
There was a problem hiding this comment.
Do we need to update the user ID? Why the ID will change during authentication?
I'm wondering if that will make it possible to take over accounts if you manage to somehow update the user ID after authenticating as some other user.
There was a problem hiding this comment.
Yes.
These are possible two different broker users when updating.
I think we should update the ID as well.
If we don't update the ID, the next time login via the same IDP user will continue the first-broker-login flow.
Because here can not find the federated user based on the IDP user ID.
I'm wondering if that will make it possible to take over accounts if you manage to somehow update the user ID after authenticating as some other user.
Before update all fields of the broker link, it makes sure the username or userId is matched.
There are two cases when updating:
case 1:
[before]
User A:
- username: user_a
- broker_user:
- username: user_a
- user_id: id_1
when the new broker_link comes
(This happens when the broker_user create a new account but use the previous username)
broker_user:
- username: user_a
- user_id: id_2
[after]
User A:
- username: user_a
- broker_user:
- username: user_a
- user_id: id_2
case 2:
[before]
User A:
- username: user_a
- broker_user:
- username: whatever
- user_id: id_1
when the new broker_link comes
(This happens when the broker_user just changed its username)
broker_user:
- username: user_b
- user_id: id_1
[after]
User A
- username: user_a
- broker_user:
- username: user_b
- user_id: id_1
The case 2 makes sure the ID not changed.
And I am not sure should we handle the case 1 to update the entire broker link.
Please help to check.
There was a problem hiding this comment.
I see. True, I did not consider this.
My concern here is basically to make sure, for the user, that he is authenticating from a different account at the IdP. Otherwise, we can (not 100% sure) enter a situation where an attacker can somehow trick the user into authenticating at some random IdP and have their account linked to the attacker.
To be safe, perhaps we should introduce a step during the flow to say "Hey, looks like you have an existing account linked to Foo IdP but using a different than the account you are now authenticating. Do you confirm using the new account instead?"
Something like that. I hope it makes sense.
There was a problem hiding this comment.
@pedroigor From what I recall, the problem arises post authentication via 'IdpConfirmLinkAuthenticator' (which pertains to the screen intended for user display). In my opinion, this issue seems to be accounted for already.
Regarding the scenario you've highlighted, it's plausible only if an attacker gains access to and can manipulate identities within the external IDP (already) linked to the Keycloak user account. It means an attacker cannot trigger this behavior from another random IDP, it has to be the same that is already linked to the keycloak account. In such a high-level compromise, the attacker could effectively impersonate anyone from the external IDP anyhow.
There was a problem hiding this comment.
To be safe, perhaps we should introduce a step during the flow to say "Hey, looks like you have an existing account linked to Foo IdP but using a different than the account you are now authenticating. Do you confirm using the new account instead?"
@pedroigor
Agree, this is what we added in our production, which verifying the code via the email sent to the Keycloak user.
the problem arises post authentication via 'IdpConfirmLinkAuthenticator' (which pertains to the screen intended for user display).
I think the confirming from IdpConfirmLinkAuthenticator is not safe enough.
Should we add such verification for the case 1 I mentioned above?
At that case, the attacker use the same username login from the IdP, it could override the broker link automatically without the real user noticing.
Or maybe it is difficult get the same username from the IdP for that he needs find the username of the user.
There was a problem hiding this comment.
I guess that now it would be easier as otherwise we would need to introduce new endpoint(s) to IdentityBrokerService for the whole shebang around showing custom screen with text "Hey, looks like you have an existing account linked to Foo IdP but using a different than the account you are now authenticating. Do you confirm using the new account instead?" if I understand correctly? But when using authenticator SPI, there won't be much customizations needed in the IdentityBrokerService .
There was a problem hiding this comment.
@mposolda
Sounds a good idea, I prefer adding a new first login flow Authenticator for this case rather than customizing IdentityBrokerService.
Maybe we need split into a new issue to track this and I will create a new PR to implement such feature,
close this one as well.
WDYT @pedroigor
There was a problem hiding this comment.
@lexcao +1 from me to your last suggestion
There was a problem hiding this comment.
Great, @pedroigor @mposolda
I have created a new issue tracking this.
Further discussion will be there.
We can close this as well.
Thank you.
#26201
|
I am closing this PR as we have different PR (see related issue #26201 ) with the other approach based on the discussion in this PR. |
Original issue
This PR closes #9209.
Description
The
IdentityBrokerService.afterFirstBrokerLoginmethod currently assumes that a Federated Identity must be created, even when it already exists. However, this assumption violates the uniqueness constraints and causes the function to fail.This breaks a unicity constraints and make the function fail. For example, with PostgreSQL (source: #9209):
To resolve this problem, I propose modifying the IdentityBrokerService.afterFirstBrokerLogin method to first check if the Federated Identity already exists before attempting to create it.
Example
Let's Keycloak defines a SAML2 identity provider that provides federated identity with Active Directory (AD), and the SubjectID is mapped to the email address claim sent by AD, here's how the situation unfolds:
hello.world@ad.com. A federated link is automatically associated with the Identity Providerad, usingusername=hello.world@ad.comandUserID=hello.world@ad.com.Hello.World@ad.com.Hello.World@ad.comconflicts with the existing federated link.Implementation
In
IdentityBrokerProvider.afterFirstBrokerLogin, we might update the Federated identity when it already exists.This part of the job is already done in
IdentityBrokerProvider.updateFederatedIdentity, depending on the IDPsyncMode, which looks good.However:
updateFederatedIdentityis called too lateusername, not theuserIdTherefore this PR implements:
a check for the existence of a Federated Identity in
IdentityBrokerProvider.afterFirstBrokerLogin: this fixes the unicity issue.a comparison the
userIdinupdateFederatedIdentity: it allows the Federated Identity to be updated when thesyncModeallows it.In the
IdentityBrokerProvider.afterFirstBrokerLoginmethod, a check is added to verify the existence of a Federated Identity. This resolves the uniqueness issue by preventing the creation of a new federated identity when one already exists.In the
IdentityBrokerProvider.updateFederatedIdentitymethod, the comparison is expanded to include theuserId. This ensures that the Federated Identity can be updated when the syncMode allows it.Versions affected
PR #9209 mentions version
15.0.2.21.1.1is still affected.