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.

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

ipa-sam: create the gidNumber attribute in the trusted domain entry #632

Closed
wants to merge 2 commits into from

Conversation

flo-renaud
Copy link
Contributor

@flo-renaud flo-renaud commented Mar 21, 2017

When a trusted domain entry is created, the uidNumber attribute is created
but not the gidNumber attribute. This causes samba to log
Failed to find a Unix account for DOM-AD$
because the samu structure does not contain a group_sid and is not put
in the cache.
The fix creates the gidNumber attribute in the trusted domain entry,
and initialises the group_sid field in the samu structure returned
by ldapsam_getsampwnam. This ensures that the entry is put in the cache.

Note that this is only a partial fix for 6660 as it does not prevent
_netr_ServerAuthenticate3 from failing with the log
_netr_ServerAuthenticate3: netlogon_creds_server_check failed. Rejecting auth request from client VM-AD machine account dom-ad.example.com.

https://pagure.io/freeipa/issue/6827

@flo-renaud
Copy link
Contributor Author

I updated the commit message with a different issue number, related to the "Failed to find a unix account" message.

Copy link
Contributor

@abbra abbra left a comment

Choose a reason for hiding this comment

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

I added some comments inline, please address them.

@@ -2419,6 +2419,8 @@ static NTSTATUS ipasam_set_trusted_domain(struct pdb_methods *methods,
if (entry == NULL || sid == NULL) {
smbldap_make_mod(priv2ld(ldap_state), entry, &mods,
LDAP_ATTRIBUTE_UIDNUMBER, IPA_MAGIC_ID_STR);
smbldap_make_mod(priv2ld(ldap_state), entry, &mods,
LDAP_ATTRIBUTE_GIDNUMBER, IPA_MAGIC_ID_STR);
Copy link
Contributor

Choose a reason for hiding this comment

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

@flo-renaud I think we need to change this. Autogeneration with DNA plugin wouldn't work here because a generated group ID does not mean the group object actually will exist. Given that TDO is created in the sub-tree that is not handled by the Managed Entries plugin, we should set gidNumber to a value of existing group.

Luckily, we have SMB Default Group for this. This is our fallback POSIX group for cases like this. Please fetch gidNumber of the SMB Default Group and use it instead here.

dn: cn=Default SMB Group,cn=groups,cn=accounts,dc=example,dc=com
cn: Default SMB Group
description: Fallback group for primary group RID, do not add users to this group
objectClass: top
objectClass: ipaobject
objectClass: posixgroup
objectClass: ipantgroupattrs
ipaUniqueID: 19ff163e-e60a-11e6-8794-001a4a62eb77
gidNumber: 1536000003
ipaNTSecurityIdentifier: S-1-5-21-570121326-3336757064-1157332047-1003

@@ -2884,6 +2893,23 @@ static bool init_sam_from_td(struct samu *user, struct pdb_trusted_domain *td,
}
talloc_free(u_sid);

tmp_ctx= talloc_init("init_sam_from_td");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not create empty context here. Create it off user object. It is guaranteed to be allocated with talloc too.

When a trusted domain entry is created, the uidNumber attribute is created
but not the gidNumber attribute. This causes samba to log
	Failed to find a Unix account for DOM-AD$
because the samu structure does not contain a group_sid and is not put
in the cache.
The fix creates the gidNumber attribute in the trusted domain entry,
and initialises the group_sid field in the samu structure returned
by ldapsam_getsampwnam. This ensures that the entry is put in the cache.

Note that this is only a partial fix for 6660 as it does not prevent
_netr_ServerAuthenticate3 from failing with the log
	_netr_ServerAuthenticate3: netlogon_creds_server_check failed. Rejecting auth request from client VM-AD machine account dom-ad.example.com.

https://pagure.io/freeipa/issue/6827
The trusted domain entries created in earlier versions are missing gidnumber.
During upgrade, a new plugin will read the gidnumber of the fallback group
cn=Default SMB Group and add this value to trusted domain entries which do
not have a gidNumber.

https://pagure.io/freeipa/issue/6827
@flo-renaud
Copy link
Contributor Author

Hi @abbra
thank you for the review. PR updated following your comments, and with an upgrade plugin to handle existing trusted domain objects.

@abbra
Copy link
Contributor

abbra commented Apr 3, 2017

Thanks. I read through the code and it looks good to me. I'm going to test it together with my work on ipasam_update_sam_account() tomorrow.

@abbra
Copy link
Contributor

abbra commented Apr 6, 2017

LGTM.

nltest /sc_verify:ipa.example.test works thanks to this pull request:

C:\Users\Administrator>nltest /sc_query:ipa.example.test
Flags: 30 HAS_IP  HAS_TIMESERV
Trusted DC Name \\master.ipa.example.test
Trusted DC Connection Status Status = 0 0x0 NERR_Success
The command completed successfully

@abbra abbra added the ack Pull Request approved, can be merged label Apr 6, 2017
@abbra abbra self-requested a review April 6, 2017 13:01
@MartinBasti
Copy link
Contributor

master:

  • e052c2d ipa-sam: create the gidNumber attribute in the trusted domain entry
  • 5405de5 Upgrade: add gidnumber to trusted domain entry

ipa-4-5:

  • 91d3694 ipa-sam: create the gidNumber attribute in the trusted domain entry
  • eddd29f Upgrade: add gidnumber to trusted domain entry

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Apr 7, 2017
@MartinBasti MartinBasti closed this Apr 7, 2017
@flo-renaud flo-renaud deleted the t6660 branch April 11, 2017 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants