I'm sending two patches solving selection of appropriate principal for GSSAPI authentication from keytab file.
A part of the first patch is a fix of an error present in the documentation. I did that early in the development phase of the patch and I didn't want to tamper with the finished patch any more. Sorry for this inconsistency.
Jan
Jan Zelený jzeleny@redhat.com wrote:
I'm sending two patches solving selection of appropriate principal for GSSAPI authentication from keytab file.
A part of the first patch is a fix of an error present in the documentation. I did that early in the development phase of the patch and I didn't want to tamper with the finished patch any more. Sorry for this inconsistency.
Jan
self-nack
I just found that this patch and my 007-2 patch have some bits mixed up, which causes them to work together, but the code is at the wrong place.
Jan
Jan Zelený jzeleny@redhat.com wrote:
I'm sending two patches solving selection of appropriate principal for GSSAPI authentication from keytab file.
A part of the first patch is a fix of an error present in the documentation. I did that early in the development phase of the patch and I didn't want to tamper with the finished patch any more. Sorry for this inconsistency.
Jan
This is updated version without the code mixup. Also the documentation update from my patch 007 has been sqashed to this one, so all related changes are in one patch.
Jan
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/29/2011 03:48 AM, Jan Zelený wrote:
Jan Zelený jzeleny@redhat.com wrote:
I'm sending two patches solving selection of appropriate principal for GSSAPI authentication from keytab file.
A part of the first patch is a fix of an error present in the documentation. I did that early in the development phase of the patch and I didn't want to tamper with the finished patch any more. Sorry for this inconsistency.
Jan
This is updated version without the code mixup. Also the documentation update from my patch 007 has been sqashed to this one, so all related changes are in one patch.
Nack.
If the SDAP_SASL_AUTHID has been explicitly set, but the SDAP_SASL_REALM hasn't, why are you overriding SDAP_SASL_AUTHID with select_principal_from_keytab()?
It would be nice to have an optional return value from select_principal_from_keytab() that was the complete string, so that in ldap_child_get_tgt_sync() we can just ask for that instead of the two final_* variables. (The _primary and _realm arguments should also be optional. It should be possible to pass NULL to them and not have the results talloc_strdup()ed into them)
krb_ctx should be initialized to NULL (in case we ever put a 'goto done' before krb5_init_context())
As mentioned in other recent reviews, instead of parsing on @, please use krb5_unparse_name_flags() and krb5_principal_get_realm() to return the primary and realm components.
And as mentioned above, it would be nice to be able to return principal_string directly if requested.
In the documentation: s/canvenient/convenient
"Priority of chosen principal is this:" should be "Priority of the chosen principal is as follows:"
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
Hi, I'm sending corrected patches. All your suggestions and objections have been addressed except maybe for this:
If the SDAP_SASL_AUTHID has been explicitly set, but the SDAP_SASL_REALM hasn't, why are you overriding SDAP_SASL_AUTHID with select_principal_from_keytab()?
I agree with you that the code made a little sense before. I did a little modification, so the SDAP_SASL_AUTHID isn't changed if possible. Here I'd like to know your opinion. We might want to prioritize the configuration entered by admin. My current approach prioritizes an actual content of the keytab if either SDAP_SASL_AUTHID or SDAP_SASL_REALM isn't entered. That means in case keytab doesn't contain principal matching the desired one, another one (based on the preference in select_principal_from_keytab()) is selected.
If the user configuration had absolute priority, there would be a comparison right after the best principal is selected by select_principal_from_keytab() and in case the selected principal doesn't correspond to the configured one, an error should be raised.
What do you think the best approach is for this?
Jan
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/30/2011 04:07 AM, Jan Zelený wrote:
Hi, I'm sending corrected patches. All your suggestions and objections have been addressed except maybe for this:
If the SDAP_SASL_AUTHID has been explicitly set, but the SDAP_SASL_REALM hasn't, why are you overriding SDAP_SASL_AUTHID with select_principal_from_keytab()?
I agree with you that the code made a little sense before. I did a little modification, so the SDAP_SASL_AUTHID isn't changed if possible. Here I'd like to know your opinion. We might want to prioritize the configuration entered by admin. My current approach prioritizes an actual content of the keytab if either SDAP_SASL_AUTHID or SDAP_SASL_REALM isn't entered. That means in case keytab doesn't contain principal matching the desired one, another one (based on the preference in select_principal_from_keytab()) is selected.
If the user configuration had absolute priority, there would be a comparison right after the best principal is selected by select_principal_from_keytab() and in case the selected principal doesn't correspond to the configured one, an error should be raised.
What do you think the best approach is for this?
If SDAP_SASL_AUTHID is specified, then ONLY this auth ID is allowable. If the keytab doesn't contain it, we need to fail.
If SDAP_SASL_REALM is specified, then only the REALM portion is mandatory (if we have no entries for this realm in the keytab, we need to fail).
And for the code review:
Nack. If the talloc_strdup() or talloc_asprintf() fails to create the return values in select_principal_from_keytab(), this should be an ENOMEM failure. We should not proceed with a value of NULL.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
Stephen Gallagher sgallagh@redhat.com wrote:
If SDAP_SASL_AUTHID is specified, then ONLY this auth ID is allowable. If the keytab doesn't contain it, we need to fail.
If SDAP_SASL_REALM is specified, then only the REALM portion is mandatory (if we have no entries for this realm in the keytab, we need to fail).
Yep, that's basically what I thought. In the new patch these conditions should be met
And for the code review:
Nack. If the talloc_strdup() or talloc_asprintf() fails to create the return values in select_principal_from_keytab(), this should be an ENOMEM failure. We should not proceed with a value of NULL.
I suspected so. Corrected.
Thanks Jan
Jan Zelený jzeleny@redhat.com wrote:
Stephen Gallagher sgallagh@redhat.com wrote:
If SDAP_SASL_AUTHID is specified, then ONLY this auth ID is allowable. If the keytab doesn't contain it, we need to fail.
If SDAP_SASL_REALM is specified, then only the REALM portion is mandatory (if we have no entries for this realm in the keytab, we need to fail).
Yep, that's basically what I thought. In the new patch these conditions should be met
And for the code review:
Nack. If the talloc_strdup() or talloc_asprintf() fails to create the return values in select_principal_from_keytab(), this should be an ENOMEM failure. We should not proceed with a value of NULL.
I suspected so. Corrected.
Thanks Jan
Just a reminder that this patch needs a review.
Thanks Jan
On Wed, 2011-03-30 at 15:41 +0200, Jan Zelený wrote:
Stephen Gallagher sgallagh@redhat.com wrote:
If SDAP_SASL_AUTHID is specified, then ONLY this auth ID is allowable. If the keytab doesn't contain it, we need to fail.
If SDAP_SASL_REALM is specified, then only the REALM portion is mandatory (if we have no entries for this realm in the keytab, we need to fail).
Yep, that's basically what I thought. In the new patch these conditions should be met
And for the code review:
Nack. If the talloc_strdup() or talloc_asprintf() fails to create the return values in select_principal_from_keytab(), this should be an ENOMEM failure. We should not proceed with a value of NULL.
I suspected so. Corrected.
Nack.
You still have unchecked talloc_strdup() calls in this patch in select_principal_from_keytab().
Otherwise I think this looks good.
On Thu, 2011-04-21 at 12:30 +0200, Jan Zelený wrote:
Nack.
You still have unchecked talloc_strdup() calls in this patch in select_principal_from_keytab().
I don't think so, all talloc_strdup() calls in that function are checked for NULL output. But there was a tiny glitch, which is fixed in attached patch.
Jan
Sorry, I wrote talloc_strdup() and really meant talloc_asprintf(). Mea culpa.
Could you please elaborate on the "tiny glitch" as well?
On Thu, 2011-04-21 at 12:30 +0200, Jan Zelený wrote:
Nack.
You still have unchecked talloc_strdup() calls in this patch in select_principal_from_keytab().
I don't think so, all talloc_strdup() calls in that function are checked for NULL output. But there was a tiny glitch, which is fixed in attached patch.
Jan
Sorry, I wrote talloc_strdup() and really meant talloc_asprintf(). Mea culpa.
Ah, that explains why I didn't find it, I looked at the output part of the function. Thanks for clarifying.
Could you please elaborate on the "tiny glitch" as well?
Nothing serious, I just added code which frees already allocated outputs if one of them fails.
Jan
On Thu, 2011-04-21 at 14:13 +0200, Jan Zelený wrote:
On Thu, 2011-04-21 at 12:30 +0200, Jan Zelený wrote:
Nack.
You still have unchecked talloc_strdup() calls in this patch in select_principal_from_keytab().
I don't think so, all talloc_strdup() calls in that function are checked for NULL output. But there was a tiny glitch, which is fixed in attached patch.
Jan
Sorry, I wrote talloc_strdup() and really meant talloc_asprintf(). Mea culpa.
Ah, that explains why I didn't find it, I looked at the output part of the function. Thanks for clarifying.
Could you please elaborate on the "tiny glitch" as well?
Nothing serious, I just added code which frees already allocated outputs if one of them fails.
Ack
On Mon, 2011-04-25 at 08:04 -0400, Stephen Gallagher wrote:
On Thu, 2011-04-21 at 14:13 +0200, Jan Zelený wrote:
On Thu, 2011-04-21 at 12:30 +0200, Jan Zelený wrote:
Nack.
You still have unchecked talloc_strdup() calls in this patch in select_principal_from_keytab().
I don't think so, all talloc_strdup() calls in that function are checked for NULL output. But there was a tiny glitch, which is fixed in attached patch.
Jan
Sorry, I wrote talloc_strdup() and really meant talloc_asprintf(). Mea culpa.
Ah, that explains why I didn't find it, I looked at the output part of the function. Thanks for clarifying.
Could you please elaborate on the "tiny glitch" as well?
Nothing serious, I just added code which frees already allocated outputs if one of them fails.
Ack
Pushed to master
sssd-devel@lists.fedorahosted.org
-
Jan Zelený -
Stephen Gallagher