[PATCH 01/11] Two small krb5_child fixes ssia
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }
[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote:
[PATCH 01/11] Two small krb5_child fixes ssia
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
Nack.
First impressions: unit tests are failing: Running suite(s): krb5_utils 86%: Checks: 15, Failures: 2, Errors: 0 ../src/tests/krb5_utils-tests.c:341:F:create_dir:test_illegal_patterns:0: cc_file_create allowed illegal pattern '/./' in filename [/dev/shm/tests_krb5_utils/priv_ccdir/abc/./ccfile]. ../src/tests/krb5_utils-tests.c:389:F:create_dir:test_cc_dir_create:0: Cannot remove /dev/shm/tests_krb5_utils/priv_ccdir: (null)
Also, the tests don't clean up after themselves, so subsequent reruns give back: Could not create empty directory [tests_krb5_utils]. Please remove [tests_krb5_utils]. FAIL: krb5-utils-tests
I'll continue looking at the patches themselves while this is addressed.
Sending the review in chunks.
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote:
[PATCH 01/11] Two small krb5_child fixes ssia
Nack. You only fixed the errno overwrite in one of the two debug messages you changed.
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
Nack.
Can we change the debug log message "Created ccache file:" to just "Created ccache:"? Since we're now printing the whole CCNAME, complete with type.
Speling misteak (it was there before, but we should fix it while we're touching the code: "successfull" should be "successful" in changepw_child()
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
Ack
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
Ack
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
Nack
Typo in help: secods -> seconds.
Typo in DEBUG: unkown -> unknown
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote: ...
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
Ack
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
Nack
Please add comments explaining that you're walking back from the end and removing any trailing slashes.
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
Nack.
Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't allocate anything atop it.
Otherwise I think this looks sane. It's a good approach to modularizing the approaches.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
Yes, this is as it should be. We're unlikely to need multiple TGTs for GSSAPI.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }
Maybe I'm confused, but shouldn't you be closing the cache before destroying it?
In general, I don't see anything obviously wrong with this code, so I'm probably okay with committing it for the beta and fixing any issues as they come up, provided that the testing I'm doing continues to meet my expectations.
I'll continue to test (and retest) until the requested revisions are complete.
[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
Ack
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
Nack
Instead of hard-coding the defaults, can we make this into a configure flag? Not all distributions are yet using systemd, so on those platforms we'll still want to default to /tmp.
Also, I'm going to open a bug to have pam_systemd.so create a subdirectory for us, so we're not polluting the /run/user/username directory with cryptically-named files.
On Wed, 2012-06-13 at 12:18 -0400, Stephen Gallagher wrote:
Also, I'm going to open a bug to have pam_systemd.so create a subdirectory for us, so we're not polluting the /run/user/username directory with cryptically-named files.
Why cryptically named files ?
We should just create one well know ccache dir. And we can create it ourselves, why should we have pam_systemd involved here ?
Simo.
On Wed, 2012-06-13 at 14:11 -0400, Simo Sorce wrote:
On Wed, 2012-06-13 at 12:18 -0400, Stephen Gallagher wrote:
Also, I'm going to open a bug to have pam_systemd.so create a subdirectory for us, so we're not polluting the /run/user/username directory with cryptically-named files.
Why cryptically named files ?
libkrb5 creates files named tktXXXXX (where XXXXX is random) and primary in the target directory. I'd like to keep this in a subdirectory of /run/user/username.
We should just create one well know ccache dir. And we can create it ourselves, why should we have pam_systemd involved here ?
/run/user/username is created automatically by pam_systemd during session start (and removed on last session exit).
If we decide to take on directory creation, we need to handle parent directories too (in case someone chooses to manually set a cache dir that's much deeper). And this still wouldn't play well with other libkrb5-using applications, since they would each need to grow this functionality as well.
So I opened https://bugzilla.redhat.com/show_bug.cgi?id=831738 to have pam_systemd just do this for us on session login so we don't have to worry about it for each and every potential consumer.
I also opened https://bugzilla.redhat.com/show_bug.cgi?id=831740 for the record (to change to /run/user/UID instead of /run/user/username). We'll see where that goes.
On Wed, 2012-06-13 at 14:21 -0400, Stephen Gallagher wrote:
On Wed, 2012-06-13 at 14:11 -0400, Simo Sorce wrote:
On Wed, 2012-06-13 at 12:18 -0400, Stephen Gallagher wrote:
Also, I'm going to open a bug to have pam_systemd.so create a subdirectory for us, so we're not polluting the /run/user/username directory with cryptically-named files.
Why cryptically named files ?
libkrb5 creates files named tktXXXXX (where XXXXX is random) and primary in the target directory. I'd like to keep this in a subdirectory of /run/user/username.
We should just create one well know ccache dir. And we can create it ourselves, why should we have pam_systemd involved here ?
/run/user/username is created automatically by pam_systemd during session start (and removed on last session exit).
If we decide to take on directory creation, we need to handle parent directories too (in case someone chooses to manually set a cache dir that's much deeper). And this still wouldn't play well with other libkrb5-using applications, since they would each need to grow this functionality as well.
No we don't need to handle parent directories creation. If the path up to the component before the cache does not exist we just fail.
this is because we MUST create the cache as the user not as root (this is mandatory) and the user will have no permission to create anything outside or /ryn/user/username
Plus I do not think we need to give the same kind of flexibility we have for FILE ccaches. Ie we should not allow arbitrary filenames.
Also we need to alignn with libkrb5 they will create the ccache dir if one does not exist in the next version, also there is work to change the default for libkrb5 to /run/user/username/ as you know, so allowing admins to change that randomly will probably be a bad idea for the DIR type.
So I opened https://bugzilla.redhat.com/show_bug.cgi?id=831738 to have pam_systemd just do this for us on session login so we don't have to worry about it for each and every potential consumer.
I think pam_systemd should have nothing to do with krb5, I would close it.
I also opened https://bugzilla.redhat.com/show_bug.cgi?id=831740 for the record (to change to /run/user/UID instead of /run/user/username). We'll see where that goes.
Yes we probably still want to push for this if it is not too late.
Simo.
I'm going to combine the replies into one (thanks for sending them separately, though).
The unit tests were fixed.
[PATCH 01/11] Two small krb5_child fixes ssia
Nack. You only fixed the errno overwrite in one of the two debug messages you changed.
Actually, one of three :-) Fixed.
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
Nack.
Can we change the debug log message "Created ccache file:" to just "Created ccache:"? Since we're now printing the whole CCNAME, complete with type.
It's only printed in create_ccache_file() which only created FILE: ccaches.
Speling misteak (it was there before, but we should fix it while we're touching the code: "successfull" should be "successful" in changepw_child()
Done
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
Ack
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
Ack
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
Nack
Typo in help: secods -> seconds.
Fixed
Typo in DEBUG: unkown -> unknown
Sorry, I don't see that?
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote: ...
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
Ack
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
Nack
Please add comments explaining that you're walking back from the end and removing any trailing slashes.
Done.
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
Nack.
Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't allocate anything atop it.
I used to, then I changed safe_remove_old_ccache_file() and forgot to remove the context. Fixed.
Otherwise I think this looks sane. It's a good approach to modularizing the approaches.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
Yes, this is as it should be. We're unlikely to need multiple TGTs for GSSAPI.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }Maybe I'm confused, but shouldn't you be closing the cache before destroying it?
It was my impression that krb5_cc_destroy() does both destroy and close and the krb5_ccache "object" is invalid after krb5_cc_close().
In general, I don't see anything obviously wrong with this code, so I'm probably okay with committing it for the beta and fixing any issues as they come up, provided that the testing I'm doing continues to meet my expectations.
I'll continue to test (and retest) until the requested revisions are complete.
There's one thing I forgot that I'm going to write a separate patch for - we should probably limit the substitutions used in the templates in case DIR is used. I think that if DIR is used, the last template must be "%d", the file-specific substitutions make no sense if DIR is used.
[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
Ack
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
Nack
Instead of hard-coding the defaults, can we make this into a configure flag? Not all distributions are yet using systemd, so on those platforms we'll still want to default to /tmp.
Done, patch #11 is a different one.
New patches are attached
Also, I'm going to open a bug to have pam_systemd.so create a subdirectory for us, so we're not polluting the /run/user/username directory with cryptically-named files.
On Wed, 2012-06-13 at 20:32 +0200, Jakub Hrozek wrote:
I'm going to combine the replies into one (thanks for sending them separately, though).
The unit tests were fixed.
[PATCH 01/11] Two small krb5_child fixes ssia
Nack. You only fixed the errno overwrite in one of the two debug messages you changed.
Actually, one of three :-) Fixed.
Ack.
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
Nack.
Can we change the debug log message "Created ccache file:" to just "Created ccache:"? Since we're now printing the whole CCNAME, complete with type.
It's only printed in create_ccache_file() which only created FILE: ccaches.
In that case, Ack.
Speling misteak (it was there before, but we should fix it while we're touching the code: "successfull" should be "successful" in changepw_child()
Done
Ack
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
Ack
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
Ack
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
Nack
Typo in help: secods -> seconds.
Fixed
Typo in DEBUG: unkown -> unknown
Sorry, I don't see that?
I can't find it anymore either. My eyes must be playing tricks on you.
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote: ...
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
Ack
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
Nack
Please add comments explaining that you're walking back from the end and removing any trailing slashes.
Done.
Nack
Your comment needs work: we only pass /some/path to find_ccdir_parent_data, not /some/path
That looks like the same thing to me :)
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
Nack.
Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't allocate anything atop it.
I used to, then I changed safe_remove_old_ccache_file() and forgot to remove the context. Fixed.
Ack
Otherwise I think this looks sane. It's a good approach to modularizing the approaches.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
Yes, this is as it should be. We're unlikely to need multiple TGTs for GSSAPI.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }Maybe I'm confused, but shouldn't you be closing the cache before destroying it?
It was my impression that krb5_cc_destroy() does both destroy and close and the krb5_ccache "object" is invalid after krb5_cc_close().
Ok, Ack.
In general, I don't see anything obviously wrong with this code, so I'm probably okay with committing it for the beta and fixing any issues as they come up, provided that the testing I'm doing continues to meet my expectations.
I'll continue to test (and retest) until the requested revisions are complete.
There's one thing I forgot that I'm going to write a separate patch for
- we should probably limit the substitutions used in the templates in
case DIR is used. I think that if DIR is used, the last template must be "%d", the file-specific substitutions make no sense if DIR is used.
Simo, Jakub and I had a long discussion on #sssd about how to handle this. The end result is that we're going to maintain the existing expansions (regardless of their usefulness).
We also decided that our plan would be to attempt to mkdir() the requested patch and fail if we could not do so. We would also fail if the path currently exists but has the wrong ownership and/or permissions.
[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
Ack
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
Nack
Instead of hard-coding the defaults, can we make this into a configure flag? Not all distributions are yet using systemd, so on those platforms we'll still want to default to /tmp.
Done, patch #11 is a different one.
New patches are attached
Nack
The specfile needs to be conditionalized to only set /run/user by default on F17 and higher. This will break our nightlies for F16, RHEL 5 and RHEL 6.
Building sssd-krb5.5 appears to be broken when building from a parallel build directory:
po4a --option doctype=docbook --package-name sssd-docs --variable builddir=/home/sgallagh/workspace/sssd/x86_64/src/man --package-version 1.8.92 --msgid-bugs-address sssd-devel@redhat.com --copyright-holder "Red Hat" --no-backups po/po4a.cfg po/po4a.cfg:13: The 'sssd-krb5.5.xml' master file does not exist.
On Wed, Jun 13, 2012 at 03:48:50PM -0400, Stephen Gallagher wrote:
On Wed, 2012-06-13 at 20:32 +0200, Jakub Hrozek wrote:
I'm going to combine the replies into one (thanks for sending them separately, though).
The unit tests were fixed.
[PATCH 01/11] Two small krb5_child fixes ssia
Nack. You only fixed the errno overwrite in one of the two debug messages you changed.
Actually, one of three :-) Fixed.
Ack.
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
Nack.
Can we change the debug log message "Created ccache file:" to just "Created ccache:"? Since we're now printing the whole CCNAME, complete with type.
It's only printed in create_ccache_file() which only created FILE: ccaches.
In that case, Ack.
Speling misteak (it was there before, but we should fix it while we're touching the code: "successfull" should be "successful" in changepw_child()
Done
Ack
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
Ack
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
Ack
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
Nack
Typo in help: secods -> seconds.
Fixed
Typo in DEBUG: unkown -> unknown
Sorry, I don't see that?
I can't find it anymore either. My eyes must be playing tricks on you.
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote: ...
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
Ack
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
Nack
Please add comments explaining that you're walking back from the end and removing any trailing slashes.
Done.
Nack
Your comment needs work: we only pass /some/path to find_ccdir_parent_data, not /some/path
That looks like the same thing to me :)
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
Nack.
Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't allocate anything atop it.
I used to, then I changed safe_remove_old_ccache_file() and forgot to remove the context. Fixed.
Ack
Otherwise I think this looks sane. It's a good approach to modularizing the approaches.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
Yes, this is as it should be. We're unlikely to need multiple TGTs for GSSAPI.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }Maybe I'm confused, but shouldn't you be closing the cache before destroying it?
It was my impression that krb5_cc_destroy() does both destroy and close and the krb5_ccache "object" is invalid after krb5_cc_close().
Ok, Ack.
In general, I don't see anything obviously wrong with this code, so I'm probably okay with committing it for the beta and fixing any issues as they come up, provided that the testing I'm doing continues to meet my expectations.
I'll continue to test (and retest) until the requested revisions are complete.
There's one thing I forgot that I'm going to write a separate patch for
- we should probably limit the substitutions used in the templates in
case DIR is used. I think that if DIR is used, the last template must be "%d", the file-specific substitutions make no sense if DIR is used.
Simo, Jakub and I had a long discussion on #sssd about how to handle this. The end result is that we're going to maintain the existing expansions (regardless of their usefulness).
We also decided that our plan would be to attempt to mkdir() the requested patch and fail if we could not do so. We would also fail if the path currently exists but has the wrong ownership and/or permissions.
[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
Ack
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
Nack
Instead of hard-coding the defaults, can we make this into a configure flag? Not all distributions are yet using systemd, so on those platforms we'll still want to default to /tmp.
Done, patch #11 is a different one.
New patches are attached
Nack
The specfile needs to be conditionalized to only set /run/user by default on F17 and higher. This will break our nightlies for F16, RHEL 5 and RHEL 6.
Done
Building sssd-krb5.5 appears to be broken when building from a parallel build directory:
po4a --option doctype=docbook --package-name sssd-docs --variable builddir=/home/sgallagh/workspace/sssd/x86_64/src/man --package-version 1.8.92 --msgid-bugs-address sssd-devel@redhat.com --copyright-holder "Red Hat" --no-backups po/po4a.cfg po/po4a.cfg:13: The 'sssd-krb5.5.xml' master file does not exist.
OK, I switched po4a.cfg to using absolute paths and sssd-krb5.xml is now referenced from builddir because it's generated.
New patches are attached.
On Thu, Jun 14, 2012 at 07:07:55PM +0200, Jakub Hrozek wrote:
On Wed, Jun 13, 2012 at 03:48:50PM -0400, Stephen Gallagher wrote:
On Wed, 2012-06-13 at 20:32 +0200, Jakub Hrozek wrote:
I'm going to combine the replies into one (thanks for sending them separately, though).
The unit tests were fixed.
[PATCH 01/11] Two small krb5_child fixes ssia
Nack. You only fixed the errno overwrite in one of the two debug messages you changed.
Actually, one of three :-) Fixed.
Ack.
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
Nack.
Can we change the debug log message "Created ccache file:" to just "Created ccache:"? Since we're now printing the whole CCNAME, complete with type.
It's only printed in create_ccache_file() which only created FILE: ccaches.
In that case, Ack.
Speling misteak (it was there before, but we should fix it while we're touching the code: "successfull" should be "successful" in changepw_child()
Done
Ack
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
Ack
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
Ack
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
Nack
Typo in help: secods -> seconds.
Fixed
Typo in DEBUG: unkown -> unknown
Sorry, I don't see that?
I can't find it anymore either. My eyes must be playing tricks on you.
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote: ...
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
Ack
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
Nack
Please add comments explaining that you're walking back from the end and removing any trailing slashes.
Done.
Nack
Your comment needs work: we only pass /some/path to find_ccdir_parent_data, not /some/path
That looks like the same thing to me :)
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
Nack.
Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't allocate anything atop it.
I used to, then I changed safe_remove_old_ccache_file() and forgot to remove the context. Fixed.
Ack
Otherwise I think this looks sane. It's a good approach to modularizing the approaches.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
Yes, this is as it should be. We're unlikely to need multiple TGTs for GSSAPI.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }Maybe I'm confused, but shouldn't you be closing the cache before destroying it?
It was my impression that krb5_cc_destroy() does both destroy and close and the krb5_ccache "object" is invalid after krb5_cc_close().
Ok, Ack.
In general, I don't see anything obviously wrong with this code, so I'm probably okay with committing it for the beta and fixing any issues as they come up, provided that the testing I'm doing continues to meet my expectations.
I'll continue to test (and retest) until the requested revisions are complete.
There's one thing I forgot that I'm going to write a separate patch for
- we should probably limit the substitutions used in the templates in
case DIR is used. I think that if DIR is used, the last template must be "%d", the file-specific substitutions make no sense if DIR is used.
Simo, Jakub and I had a long discussion on #sssd about how to handle this. The end result is that we're going to maintain the existing expansions (regardless of their usefulness).
We also decided that our plan would be to attempt to mkdir() the requested patch and fail if we could not do so. We would also fail if the path currently exists but has the wrong ownership and/or permissions.
[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
Ack
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
Nack
Instead of hard-coding the defaults, can we make this into a configure flag? Not all distributions are yet using systemd, so on those platforms we'll still want to default to /tmp.
Done, patch #11 is a different one.
New patches are attached
Nack
The specfile needs to be conditionalized to only set /run/user by default on F17 and higher. This will break our nightlies for F16, RHEL 5 and RHEL 6.
Done
Building sssd-krb5.5 appears to be broken when building from a parallel build directory:
po4a --option doctype=docbook --package-name sssd-docs --variable builddir=/home/sgallagh/workspace/sssd/x86_64/src/man --package-version 1.8.92 --msgid-bugs-address sssd-devel@redhat.com --copyright-holder "Red Hat" --no-backups po/po4a.cfg po/po4a.cfg:13: The 'sssd-krb5.5.xml' master file does not exist.
OK, I switched po4a.cfg to using absolute paths and sssd-krb5.xml is now referenced from builddir because it's generated.
New patches are attached.
As discussed on IRC, I reverted the changes that include the configure-time default. We will include those as part of https://fedorahosted.org/sssd/ticket/1378 because doing it right proved to be tricky and out of scope.
On Thu, 2012-06-14 at 21:22 +0200, Jakub Hrozek wrote:
On Thu, Jun 14, 2012 at 07:07:55PM +0200, Jakub Hrozek wrote:
On Wed, Jun 13, 2012 at 03:48:50PM -0400, Stephen Gallagher wrote:
On Wed, 2012-06-13 at 20:32 +0200, Jakub Hrozek wrote:
I'm going to combine the replies into one (thanks for sending them separately, though).
The unit tests were fixed.
[PATCH 01/11] Two small krb5_child fixes ssia
Nack. You only fixed the errno overwrite in one of the two debug messages you changed.
Actually, one of three :-) Fixed.
Ack.
[PATCH 02/11] Provide more debugging in krb5_child and ldap_child I started this patch before Nick did, maybe it would still be useful, at least the parts that get rid of level-9 DEBUG messages.
Nack.
Can we change the debug log message "Created ccache file:" to just "Created ccache:"? Since we're now printing the whole CCNAME, complete with type.
It's only printed in create_ccache_file() which only created FILE: ccaches.
In that case, Ack.
Speling misteak (it was there before, but we should fix it while we're touching the code: "successfull" should be "successful" in changepw_child()
Done
Ack
[PATCH 03/11] Allow redefining the KRB5_CHILD path The krb5-child-test will want to run the child from the build directory.
Ack
[PATCH 04/11] Split parse_krb5_child_response so it can be reused krb5-child-test will be another consumer. It also makes the code more readable by splitting a huge function.
Ack
[PATCH 05/11] Add a krb5_child test tool https://fedorahosted.org/sssd/ticket/1127
Nack
Typo in help: secods -> seconds.
Fixed
Typo in DEBUG: unkown -> unknown
Sorry, I don't see that?
I can't find it anymore either. My eyes must be playing tricks on you.
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote: ...
[PATCH 06/11] Residual util functions Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa tch adds a couple of utilities to support parsing if ccache locations, checking types etc.
Ack
[PATCH 07/11] Handle trailing slash in the ccname template With the DIR cache support, it's perfectly legal to specify a ccname directory that ends with a slash. The create_dir function did not han dle that situation correctly. The unit test is included in the DIR: cache patch, because it uses the cc_dir_create() function.
Nack
Please add comments explaining that you're walking back from the end and removing any trailing slashes.
Done.
Nack
Your comment needs work: we only pass /some/path to find_ccdir_parent_data, not /some/path
That looks like the same thing to me :)
[PATCH 08/11] Add a credential cache back end structure To be able to add support for new credential cache types easily, this patch creates a new structure sss_krb5_cc_be that defines common with a credential cache, such as create, check if used or remove.
Nack.
Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't allocate anything atop it.
I used to, then I changed safe_remove_old_ccache_file() and forgot to remove the context. Fixed.
Ack
Otherwise I think this looks sane. It's a good approach to modularizing the approaches.
[PATCH 09/11] Add support for storing credential caches in the DIR: back end https://fedorahosted.org/sssd/ticket/974
Please note that only the TGTs acquired by the krb5_child have changed, the ldap_child still puts its ccache into /var/lib/sss/db.
Yes, this is as it should be. We're unlikely to need multiple TGTs for GSSAPI.
The cc_dir_remove() function is a little odd, I tried to use the krb5 API directly, but I think I found a bug in libkrb5. For a subsidiary cache that does not exist (DIR::/no/such/path), the following code would segfault:
krb5_cc_resolve(context, location, &ccache); // returns EOK krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE if (krberr) { if (ccache) krb5_cc_close(context, ccache); // SIGSEGV }Maybe I'm confused, but shouldn't you be closing the cache before destroying it?
It was my impression that krb5_cc_destroy() does both destroy and close and the krb5_ccache "object" is invalid after krb5_cc_close().
Ok, Ack.
In general, I don't see anything obviously wrong with this code, so I'm probably okay with committing it for the beta and fixing any issues as they come up, provided that the testing I'm doing continues to meet my expectations.
I'll continue to test (and retest) until the requested revisions are complete.
There's one thing I forgot that I'm going to write a separate patch for
- we should probably limit the substitutions used in the templates in
case DIR is used. I think that if DIR is used, the last template must be "%d", the file-specific substitutions make no sense if DIR is used.
Simo, Jakub and I had a long discussion on #sssd about how to handle this. The end result is that we're going to maintain the existing expansions (regardless of their usefulness).
We also decided that our plan would be to attempt to mkdir() the requested patch and fail if we could not do so. We would also fail if the path currently exists but has the wrong ownership and/or permissions.
[PATCH 10/11] Use Kerberos context in KRB5_DEBUG Passing Kerberos context to sss_krb5_get_error_message will allow us to get better error messages. This patch technically belong earlier, but rebasing would be hard at this point.
Ack
[PATCH 11/11] Switch Kerberos cache default to DIR Just switches the defaults.
Nack
Instead of hard-coding the defaults, can we make this into a configure flag? Not all distributions are yet using systemd, so on those platforms we'll still want to default to /tmp.
Done, patch #11 is a different one.
New patches are attached
Nack
The specfile needs to be conditionalized to only set /run/user by default on F17 and higher. This will break our nightlies for F16, RHEL 5 and RHEL 6.
Done
Building sssd-krb5.5 appears to be broken when building from a parallel build directory:
po4a --option doctype=docbook --package-name sssd-docs --variable builddir=/home/sgallagh/workspace/sssd/x86_64/src/man --package-version 1.8.92 --msgid-bugs-address sssd-devel@redhat.com --copyright-holder "Red Hat" --no-backups po/po4a.cfg po/po4a.cfg:13: The 'sssd-krb5.5.xml' master file does not exist.
OK, I switched po4a.cfg to using absolute paths and sssd-krb5.xml is now referenced from builddir because it's generated.
New patches are attached.
As discussed on IRC, I reverted the changes that include the configure-time default. We will include those as part of https://fedorahosted.org/sssd/ticket/1378 because doing it right proved to be tricky and out of scope.
Patches compile and behave as expected.
Ack!
On Thu, 2012-06-14 at 15:49 -0400, Stephen Gallagher wrote:
On Thu, 2012-06-14 at 21:22 +0200, Jakub Hrozek wrote:
On Thu, Jun 14, 2012 at 07:07:55PM +0200, Jakub Hrozek wrote:
On Wed, Jun 13, 2012 at 03:48:50PM -0400, Stephen Gallagher wrote:
On Wed, 2012-06-13 at 20:32 +0200, Jakub Hrozek wrote:
I'm going to combine the replies into one (thanks for sending them separately, though).
The unit tests were fixed.
> [PATCH 01/11] Two small krb5_child fixes > ssia >
Nack. You only fixed the errno overwrite in one of the two debug messages you changed.
Actually, one of three :-) Fixed.
Ack.
> [PATCH 02/11] Provide more debugging in krb5_child and ldap_child > I started this patch before Nick did, maybe it would still be useful, at > least the parts that get rid of level-9 DEBUG messages. >
Nack.
Can we change the debug log message "Created ccache file:" to just "Created ccache:"? Since we're now printing the whole CCNAME, complete with type.
It's only printed in create_ccache_file() which only created FILE: ccaches.
In that case, Ack.
Speling misteak (it was there before, but we should fix it while we're touching the code: "successfull" should be "successful" in changepw_child()
Done
Ack
> [PATCH 03/11] Allow redefining the KRB5_CHILD path > The krb5-child-test will want to run the child from the build directory. >
Ack
> [PATCH 04/11] Split parse_krb5_child_response so it can be reused > krb5-child-test will be another consumer. It also makes the code more > readable by splitting a huge function. >
Ack
> [PATCH 05/11] Add a krb5_child test tool > https://fedorahosted.org/sssd/ticket/1127 >
Nack
Typo in help: secods -> seconds.
Fixed
Typo in DEBUG: unkown -> unknown
Sorry, I don't see that?
I can't find it anymore either. My eyes must be playing tricks on you.
On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote: ... > [PATCH 06/11] Residual util functions > Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa > tch adds a couple of utilities to support parsing if ccache locations, > checking types etc. >
Ack
> [PATCH 07/11] Handle trailing slash in the ccname template > With the DIR cache support, it's perfectly legal to specify a ccname > directory that ends with a slash. The create_dir function did not han dle > that situation correctly. The unit test is included in the DIR: cache > patch, because it uses the cc_dir_create() function. >
Nack
Please add comments explaining that you're walking back from the end and removing any trailing slashes.
Done.
Nack
Your comment needs work: we only pass /some/path to find_ccdir_parent_data, not /some/path
That looks like the same thing to me :)
> [PATCH 08/11] Add a credential cache back end structure > To be able to add support for new credential cache types easily, this > patch creates a new structure sss_krb5_cc_be that defines common with > a credential cache, such as create, check if used or remove. >
Nack.
Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't allocate anything atop it.
I used to, then I changed safe_remove_old_ccache_file() and forgot to remove the context. Fixed.
Ack
Otherwise I think this looks sane. It's a good approach to modularizing the approaches.
> [PATCH 09/11] Add support for storing credential caches in the DIR: back end > https://fedorahosted.org/sssd/ticket/974 > > Please note that only the TGTs acquired by the krb5_child have changed, > the ldap_child still puts its ccache into /var/lib/sss/db. >
Yes, this is as it should be. We're unlikely to need multiple TGTs for GSSAPI.
> The cc_dir_remove() function is a little odd, I tried to use the krb5 > API directly, but I think I found a bug in libkrb5. For a subsidiary > cache that does not exist (DIR::/no/such/path), the following code would > segfault: > > krb5_cc_resolve(context, location, &ccache); // returns EOK > krberr = krb5_cc_destroy(context, ccache); // KRB5_FCC_NOFILE > if (krberr) { > if (ccache) krb5_cc_close(context, ccache); // SIGSEGV > }
Maybe I'm confused, but shouldn't you be closing the cache before destroying it?
It was my impression that krb5_cc_destroy() does both destroy and close and the krb5_ccache "object" is invalid after krb5_cc_close().
Ok, Ack.
In general, I don't see anything obviously wrong with this code, so I'm probably okay with committing it for the beta and fixing any issues as they come up, provided that the testing I'm doing continues to meet my expectations.
I'll continue to test (and retest) until the requested revisions are complete.
There's one thing I forgot that I'm going to write a separate patch for
- we should probably limit the substitutions used in the templates in
case DIR is used. I think that if DIR is used, the last template must be "%d", the file-specific substitutions make no sense if DIR is used.
Simo, Jakub and I had a long discussion on #sssd about how to handle this. The end result is that we're going to maintain the existing expansions (regardless of their usefulness).
We also decided that our plan would be to attempt to mkdir() the requested patch and fail if we could not do so. We would also fail if the path currently exists but has the wrong ownership and/or permissions.
> > [PATCH 10/11] Use Kerberos context in KRB5_DEBUG > Passing Kerberos context to sss_krb5_get_error_message will allow us to > get better error messages. This patch technically belong earlier, but > rebasing would be hard at this point. >
Ack
> [PATCH 11/11] Switch Kerberos cache default to DIR > Just switches the defaults.
Nack
Instead of hard-coding the defaults, can we make this into a configure flag? Not all distributions are yet using systemd, so on those platforms we'll still want to default to /tmp.
Done, patch #11 is a different one.
New patches are attached
Nack
The specfile needs to be conditionalized to only set /run/user by default on F17 and higher. This will break our nightlies for F16, RHEL 5 and RHEL 6.
Done
Building sssd-krb5.5 appears to be broken when building from a parallel build directory:
po4a --option doctype=docbook --package-name sssd-docs --variable builddir=/home/sgallagh/workspace/sssd/x86_64/src/man --package-version 1.8.92 --msgid-bugs-address sssd-devel@redhat.com --copyright-holder "Red Hat" --no-backups po/po4a.cfg po/po4a.cfg:13: The 'sssd-krb5.5.xml' master file does not exist.
OK, I switched po4a.cfg to using absolute paths and sssd-krb5.xml is now referenced from builddir because it's generated.
New patches are attached.
As discussed on IRC, I reverted the changes that include the configure-time default. We will include those as part of https://fedorahosted.org/sssd/ticket/1378 because doing it right proved to be tricky and out of scope.
Patches compile and behave as expected.
Ack!
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Stephen Gallagher