Fix freeipa userid or email logins creating different users#23723
Conversation
|
Do we need to deal with upgrades? Is it even possible? |
jrafanie
force-pushed
the
fix-freeipa-userid-email-creating-different-users
branch
2 times, most recently
from
af9e3a2 to
90321b1
Compare
Do you mean the multiple users with the same email address but different userids? I guess the question is how do we detect the incorrect user record? userid == email address? I could see domain matching email address so we might have false positives that users would need to review. I'm not sure. |
If it's available, we can expose the principal name as that gives a more consistent user lookup as sssd allows logins with email address or the freeipa/identity provider's username, leading to the username being not reliable for creating our internal user record based on it. This change is needed to support this: ManageIQ/manageiq#23723
The principal name provides a consistent username for creating user records since sssd supports logins with email or the username from the authentication system (freeipa). Using this consistent principal name prevents creating duplicate users created from both the free ipa username and from the configured email address. Related: ManageIQ/manageiq#23723 ManageIQ/manageiq-appliance#401
The principal name provides a consistent username for creating user records since sssd supports logins with email or the username from the authentication system (freeipa). Using this consistent principal name prevents creating duplicate users created from both the free ipa username and from the configured email address. Related: ManageIQ/manageiq#23723 ManageIQ/manageiq-appliance#401
The principal name provides a consistent username for creating user records since sssd supports logins with email or the username from the authentication system (freeipa). Using this consistent principal name prevents creating duplicate users created from both the free ipa username and from the configured email address. Related: ManageIQ/manageiq#23723 ManageIQ/manageiq-appliance#401
|
Is there a change to the config on the pods side as well? (Also can you verify if we need a downstream change?) |
app/models/authenticator/httpd.rb
Outdated
| User.in_my_region.where("userid LIKE ?", "%=#{user_attrs[:username]},%,#{dn_domain}").last | ||
| end | ||
|
|
||
| # TODO: remove this method if we have the principal name, we will already have it |
There was a problem hiding this comment.
I feel like there's a missing comma or something in this comment - my brain can't parse it.
There was a problem hiding this comment.
yeah, agreed, that was poorly written
app/models/authenticator/httpd.rb
Outdated
| # NOTE: X-REMOTE-USER-PRINCIPAL can be set in the headers by the sssd/apache configuration | ||
| # with a value of the krbPrincipalName. |
There was a problem hiding this comment.
Not sure if this comment is helpful - when we hit this part we don't care how the principal name was set. In fact, I'm considering we might push down any logic into other auth types as well and force them to also set a principal name even if it happens to be the username also.
There was a problem hiding this comment.
yeah, that makes sense... I'll update the comments to lean in that direction.
| expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire) | ||
| # Verify the principal was normalized from 'cheshire@EXAMPLE.COM' to 'cheshire@example.com' |
There was a problem hiding this comment.
Think this comment should come before the expectation? (otherwise it looks like a "TODO" type comment)
| expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire) | |
| # Verify the principal was normalized from 'cheshire@EXAMPLE.COM' to 'cheshire@example.com' | |
| # Verify the principal was normalized from 'cheshire@EXAMPLE.COM' to 'cheshire@example.com' | |
| expect(subject.lookup_by_identity('cheshire', request)).to eq(cheshire) |
There was a problem hiding this comment.
I simplified the tests so now this is rewritten.
|
|
||
| result = subject.send(:user_attrs_from_external_directory_via_dbus, 'jdoe') | ||
| expect(result).to eq(expected_jdoe_attrs) | ||
| expect(result["krbPrincipalName"]).to eq("jdoe@IPA.TEST") |
There was a problem hiding this comment.
This last line of the test is not necessary because we already checked it in the line before.
| allow(ifp_interface).to receive(:GetUserAttr).with('jdoe', requested_attrs).and_return(jdoe_attrs) | ||
| allow(MiqGroup).to receive(:get_httpd_groups_by_user).with('jdoe').and_return([]) | ||
|
|
||
| user_attrs, _groups = subject.send(:user_details_from_external_directory, 'jdoe') |
There was a problem hiding this comment.
This is testing a different method than the describe for this section (describe ".user_attrs_from_external_directory_via_dbus" do) - should this test be in a different section?
There was a problem hiding this comment.
Fixed, they shared common setup so I moved that up into a higher "with dbus mocks" context.
jrafanie
force-pushed
the
fix-freeipa-userid-email-creating-different-users
branch
3 times, most recently
from
29606f9 to
e4ffe4b
Compare
…name Because sssd supports logins with email or userids by default[1], we need to ensure we're using the same user for the same backend external auth account, such as freeipa. In this case, we need to fetch the krbPrincipalName user attribute or if coming from httpd request headers, the X-REMOTE-USER-PRINCIPAL, which under the covers also comes from the krbPrincipalName value. This krbPrincipalName is the backend account name with the kerberos domain and is consistent if you login with: first.last@example.com (email) flast@IPA.test(ipa userid) flast@ipa.test(case insensitive) flast(ipa userid without the domain) Previously, the email login would create a user with username of: first.last. Existing username normalization occurs in the authenticate method. Since we're overriding this behavior in the subclass for systems that have the krbPrincipalName user attribute or from httpd header with key X-REMOTE-USER-PRINCIPAL, we need to ensure these values are also normalized or we could have an existing user created with a downcased userid and a new user with uppercase characters in the userid. [1] Logins with either the userid or the email address is a feature of SSSD, the daemon for central access to the various identity providers in linux. You can see they updated the documentation to better describe this feature in: https://issues.redhat.com/browse/RHEL-1654 They released this doc in: https://access.redhat.com/errata/RHBA-2024:9351 There's a little more found here: https://www.github.com/SSSD/sssd/issues/7136 There is some configuration that can be done to disable this, see the last link. "If for some reason several users need to share the same email address then set this option to a nonexistent attribute name in order to disable user lookup/login by email."
jrafanie
force-pushed
the
fix-freeipa-userid-email-creating-different-users
branch
from
e4ffe4b to
3ffb8ab
Compare
|
Checked commits jrafanie/manageiq@3ffb8ab~...b0b827e with ruby 3.3.10, rubocop 1.56.3, haml-lint 0.69.0, and yamllint |
|
@jrafanie A couple more places might need changes.
|
jrafanie
deleted the
fix-freeipa-userid-email-creating-different-users
branch
We only set this for usage through sssd and httpd but this could be used as the de facto standard for all httpd based identity providers. Follow up to: ManageIQ/manageiq#23723 ManageIQ/manageiq-appliance#401 ManageIQ/guides#587
|
I don't think this is needed here since we have oidc/saml and other logins that don't currently go through sssd/httpd.
still to review.. still to review... I'm not sure how this is used to verify if it's needed but it's on the list here. Maybe I'll create a new issue to review this although I just don't know if we'll get to it. |
3 participants
This PRs will default to the existing behavior and depends on two other PRs to use the principal name for a more consistent username for creating unique users in our system based on ones in the external authentication system.
The dependent PRs are:
Use the downcased freeipa principal name, otherwise the provided username
Because sssd supports logins with email or userids by default[1], we need to
ensure we're using the same user for the same backend external auth account,
such as freeipa. In this case, we need to fetch the krbPrincipalName user
attribute or if coming from httpd request headers, the X-REMOTE-USER-PRINCIPAL,
which under the covers also comes from the krbPrincipalName value. This krbPrincipalName
is the backend account name with the kerberos domain and is consistent if you
login with:
first.last@example.com (email)
flast@IPA.test(ipa userid)
flast@ipa.test(case insensitive)
flast(ipa userid without the domain)
Previously, the email login would create a user with username of:
first.last.
Existing username normalization occurs in the authenticate method. Since we're
overriding this behavior in the subclass for systems that have the
krbPrincipalName user attribute or from httpd header with key
X-REMOTE-USER-PRINCIPAL, we need to ensure these values are also normalized or
we could have an existing user created with a downcased userid and a new user
with uppercase characters in the userid.
[1] Logins with either the userid or the email address is a feature of SSSD,
the daemon for central access to the various identity providers in linux.
You can see they updated the documentation to better describe this feature in:
https://issues.redhat.com/browse/RHEL-1654
They released this doc in:
https://access.redhat.com/errata/RHBA-2024:9351
There's a little more found here: https://www.github.com/SSSD/sssd/issues/7136
There is some configuration that can be done to disable this, see the last link.
"If for some reason several users need to share the same email address then set this
option to a nonexistent attribute name in order to disable user lookup/login by email."