[SSSD] [PATCHES] PAC responder and client

Jan Zelený jzeleny at redhat.com
Mon Jun 11 11:04:14 UTC 2012


Sumit sent me his set of patches last Friday. I've done some merges, filled 
commit messages and other cosmetic adjustments. I've also done initial review 
and I'm sending my comments:

get_rid() doesn't need to be in the header file, it can be static in the module

Defining idmap_talloc() and idmap_free() twice, please remove one of them.

The condition if (pac_ctx->idmap_ctx == NULL) in domsid_rid_to_uid() will be 
always false, won't it? If it was NULL, the entire responder would fail way 
earlier.

Missing debug_level initialization in main() in PAC responder

pac_save_memberships_next() is missing static in its definition (declaration is 
ok)

pac_save_memberships_recv() is missing static

you are not assigning the result to *_pwd in get_pwd_from_pac()

Is the TODO in save_pac_user() valid and important? Do we need to address this 
before pushing the patch set?

Nitpick: please use initialization instead of "g = 0;" in get_gids_from_pac()
Typo: Remote users are members ... (comment in pac_save_memberships_send())

Please remove the sysdb_search_group_by_gid() call in 
pac_save_memberships_next(), it is redundant there and can be replaced by 
checking the return code after calling pac_store_memberships().

You are not freeing pr_ctx in some error cases. The only place pr_ctx is 
free'd is pac_save_memberships_done(), i.e. only when everything went ok.

Please add some talloc_free() calls where necessary. My first guess is that 
pac_get_domains_done() and pac_add_pac_user() are the only two places where it 
is necessary.

In function local_sid_to_id(): I think another check should be in place:
if (rid-remote_min+local_min > local_max) return ERANGE;

Sumit, please contact me if you need help with anything.

Thanks
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-PAC-responder-add-basic-infrastructure.patch
Type: text/x-patch
Size: 15851 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-PAC-responder-add-some-utility-functions.patch
Type: text/x-patch
Size: 4060 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-PAC-responder-add-the-core-functionality.patch
Type: text/x-patch
Size: 28654 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-PAC-responder-support-in-spec-file.patch
Type: text/x-patch
Size: 1098 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-PAC-responder-test-suite.patch
Type: text/x-patch
Size: 4841 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-PAC-client-add-basic-support-in-common-client-code.patch
Type: text/x-patch
Size: 3138 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-PAC-client-add-krb5-authdata-plugin.patch
Type: text/x-patch
Size: 19838 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120611/1cdf038a/attachment.sig>


More information about the sssd-devel mailing list