Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AD Provider: read sAMAccountName unconditionally #7497

Closed
wants to merge 2 commits into from
Closed

AD Provider: read sAMAccountName unconditionally #7497

wants to merge 2 commits into from

Conversation

ondrejv2
Copy link
Contributor

This is a follow up to pull request #7476 incorporating the changes as Simo requested.

@ondrejv2 ondrejv2 mentioned this pull request Jul 16, 2024
Closed
@andreboscatto andreboscatto requested a review from sumit-bose July 16, 2024 12:34
sumit-bose
sumit-bose reviewed Jul 22, 2024
View reviewed changes
src/providers/ldap/sdap_async_users.c Outdated
opts->user_map[SDAP_AT_USER_SAMACCOUNTNAME].sys_name, &samaccountname);
if (ret == EOK) {
ret = ENOENT;
realm = dp_opt_get_cstring(opts->basic, SDAP_KRB5_REALM);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I think it would be better to use realm = dom->realm here, opts->basic contain the global options of the backend and SDAP_KRB5_REALM is the realm of the domain you joined to. If the user is coming from a trusted domain this realm would be wrong.

bye,
Sumit

sumit-bose
sumit-bose reviewed Jul 22, 2024
View reviewed changes
src/providers/ldap/sdap_utils.c Outdated
return NULL;
}

realm = dp_opt_get_cstring(sdap_basic_opts, SDAP_KRB5_REALM);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
same here, SDAP_KRB5_REALM would be wrong for trusted domain. It works for get_enterprise_principal_string_filte() because with enterprise principals it is expected that the realm is from the local domain and the request is send to the local KDC which then will look at the part before the realm with the \@ and will try to figure out from which trusted realm the principal might be coming from and if it found wound it will tell the client to forward the request to this realm. But all of this is special for enterprise principals. But here the proper canonical principal for a user from a trusted domain is needed and hence the realm must match.

bye,
Sumit

sumit-bose
sumit-bose reviewed Jul 22, 2024
View reviewed changes
src/db/sysdb.h Outdated Show resolved Hide resolved
@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Jul 23, 2024
justin-stephenson
justin-stephenson reviewed Jul 23, 2024
View reviewed changes
src/providers/ldap/sdap_utils.c Outdated
/* we check the principal and if it contains our realm, then we drop it
* for the comparison with sAMAccountName */

char *principal_string_to_sAMAccountName(TALLOC_CTX *mem_ctx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch to all lowercase principal_string_to_samaccountname to follow C naming convention

@ondrejv2
Copy link
Contributor Author

Hi,
can you review again please?
Thanks.
Ondrej

@alexey-tikhonov
Copy link
Member

It doesn't build:

/usr/bin/ld: ./.libs/libsss_ldap_common.so: undefined reference to `principal_string_to_samaccountname'

@ondrejv2
Copy link
Contributor Author

ondrejv2 commented Aug 1, 2024

Hello, is the PR in good shape now? Can anyone confirm?

justin-stephenson
justin-stephenson approved these changes Aug 1, 2024
View reviewed changes
Copy link
Contributor

@justin-stephenson justin-stephenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack from me, thank you.

@sumit-bose
Copy link
Contributor

Hi,

thanks for the update. Can you squash the new patch into the first one and rebase your patches on top of the current master so that the patch SPEC: add new systemtap-sdt-dtrace to build deps is includes as well so that the CI runs for Fedora 41 and rawhide will work again?

bye,
Sumit

@alexey-tikhonov
Copy link
Member

Please 'rebase' instead of 'merge'

@ondrejv2
Copy link
Contributor Author

ondrejv2 commented Aug 22, 2024 •
edited
Loading

Hi,
Can you check now?

@sumit-bose
Copy link
Contributor

Hi, Can you check now?

Hi,

thank you, this looks better now. Codewise I'm fine, I will do some final testing.

bye,
Sumit

sumit-bose
sumit-bose approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for your patience, I have no further comments, ACK.

bye,
Sumit

@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Sep 4, 2024
@alexey-tikhonov
Copy link
Member

Pushed PR: #7497

  • master
    • 7a27e53 - AD: Construct UPN from the sAMAccountName
    • f05bd34 - AD provider: Read sAMAccountName attribute unconditionally

@ondrejv2 ondrejv2 deleted the samaccountname2 branch September 16, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sumit-bose sumit-bose sumit-bose approved these changes

@justin-stephenson justin-stephenson justin-stephenson approved these changes

Assignees

@justin-stephenson justin-stephenson

@sumit-bose sumit-bose

Labels
no-backport This should go to target branch only. Pushed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ondrejv2 @alexey-tikhonov @sumit-bose @justin-stephenson