Re: [Freeipa-devel] Final OTP Review
On 05/14/2013 07:12 PM, Martin Kosek wrote: On 05/14/2013 03:53 PM, Nathaniel McCallum wrote: On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote: On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote: On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Ok, but this does not answer my question. We definitely need to *always* run our other preop plugin as we do sanity checks like verifying if the user is enabled/disabled etc... Also we need to understand how to deal with migrating password auth when OTP is enabeld. TBH I think we should not have a separate OTP-auth plugin but we should probably have a single plugin that handles authentication and the 2 should be merged. Keeping them separate is going to cause more harm than good with unexpected interactions. We could still have 2 plugins and simply move the prebind action currently don in ipa-pwd-extop to the otp plugin by making some more code common. But it is probably easier to just merge OTP into ipa-pwd-extop right now than try to do a huge refactoring. We can always refactor the ipa-pwd-extop plugin later. The attached patches encompass an initial review of the companion daemon and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp into ipa-pwd-extop appears to have broken something during install, but I don't have enough familiarity with 389 to understand what I've broken. If I upgrade after an install, it appears to work. An RPM with the patches is available here: http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935 Nathan / Rob / Simo, could you take a look and see what might be broken in ipa-pwd-extop? While I'm not quite sure what the problem was, I do know it appeared on the stock 3.2 F19 RPMs. I also fixed it by accident. I am certain it is unrelated to these patches. I have now tested install and upgrade with the six patches in the previous email and everything is in order, including permissions. At this point, we just need reviews/ACKs. Nathaniel I tested IPA server upgrades, new installs and also adding 3.2+OTP replica for F18 3.1.4 IPA master. Everything seemed to work fine (when I added my patch 407 fixing the replication), I did not see any breakage. Issues I found with too much logging I reported should now be fixed on github, so this should be OK. So it is an ACK from my side if Rob does not discover some blocking issue. Martin We have all the acks now (some went off-list). Pushed to master, ipa-3-2. Thanks! Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On 05/17/2013 09:42 AM, Martin Kosek wrote: On 05/14/2013 07:12 PM, Martin Kosek wrote: On 05/14/2013 03:53 PM, Nathaniel McCallum wrote: On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote: On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote: On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Ok, but this does not answer my question. We definitely need to *always* run our other preop plugin as we do sanity checks like verifying if the user is enabled/disabled etc... Also we need to understand how to deal with migrating password auth when OTP is enabeld. TBH I think we should not have a separate OTP-auth plugin but we should probably have a single plugin that handles authentication and the 2 should be merged. Keeping them separate is going to cause more harm than good with unexpected interactions. We could still have 2 plugins and simply move the prebind action currently don in ipa-pwd-extop to the otp plugin by making some more code common. But it is probably easier to just merge OTP into ipa-pwd-extop right now than try to do a huge refactoring. We can always refactor the ipa-pwd-extop plugin later. The attached patches encompass an initial review of the companion daemon and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp into ipa-pwd-extop appears to have broken something during install, but I don't have enough familiarity with 389 to understand what I've broken. If I upgrade after an install, it appears to work. An RPM with the patches is available here: http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935 Nathan / Rob / Simo, could you take a look and see what might be broken in ipa-pwd-extop? While I'm not quite sure what the problem was, I do know it appeared on the stock 3.2 F19 RPMs. I also fixed it by accident. I am certain it is unrelated to these patches. I have now tested install and upgrade with the six patches in the previous email and everything is in order, including permissions. At this point, we just need reviews/ACKs. Nathaniel I tested IPA server upgrades, new installs and also adding 3.2+OTP replica for F18 3.1.4 IPA master. Everything seemed to work fine (when I added my patch 407 fixing the replication), I did not see any breakage. Issues I found with too much logging I reported should now be fixed on github, so this should be OK. So it is an ACK from my side if Rob does not discover some blocking issue. Martin We have all the acks now (some went off-list). Pushed to master, ipa-3-2. Thanks! Martin Just a note: With these patches, master no longer builds on Fedora 18. (no krb5-devel 1.11) -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
- Original Message - We have all the acks now (some went off-list). Pushed to master, ipa-3-2. Thanks! Martin Just a note: With these patches, master no longer builds on Fedora 18. (no krb5-devel 1.11) If you want to still build on F18 you can simply do this: yum install --releasever=19 krb5-devel It will not drag in any large dependency set and will work just fine for clients as the ABI is compatible. However older FreeIPA Server builds will stop to work as the KDC driver needs a rebuild as the 1.11 vtable has changed slightly. Simo. -- Simo Sorce * Red Hat, Inc. * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote: On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote: On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Ok, but this does not answer my question. We definitely need to *always* run our other preop plugin as we do sanity checks like verifying if the user is enabled/disabled etc... Also we need to understand how to deal with migrating password auth when OTP is enabeld. TBH I think we should not have a separate OTP-auth plugin but we should probably have a single plugin that handles authentication and the 2 should be merged. Keeping them separate is going to cause more harm than good with unexpected interactions. We could still have 2 plugins and simply move the prebind action currently don in ipa-pwd-extop to the otp plugin by making some more code common. But it is probably easier to just merge OTP into ipa-pwd-extop right now than try to do a huge refactoring. We can always refactor the ipa-pwd-extop plugin later. The attached patches encompass an initial review of the companion daemon and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp into ipa-pwd-extop appears to have broken something during install, but I don't have enough familiarity with 389 to understand what I've broken. If I upgrade after an install, it appears to work. An RPM with the patches is available here: http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935 Nathan / Rob / Simo, could you take a look and see what might be broken in ipa-pwd-extop? While I'm not quite sure what the problem was, I do know it appeared on the stock 3.2 F19 RPMs. I also fixed it by accident. I am certain it is unrelated to these patches. I have now tested install and upgrade with the six patches in the previous email and everything is in order, including permissions. At this point, we just need reviews/ACKs. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On 05/14/2013 03:53 PM, Nathaniel McCallum wrote: On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote: On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote: On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Ok, but this does not answer my question. We definitely need to *always* run our other preop plugin as we do sanity checks like verifying if the user is enabled/disabled etc... Also we need to understand how to deal with migrating password auth when OTP is enabeld. TBH I think we should not have a separate OTP-auth plugin but we should probably have a single plugin that handles authentication and the 2 should be merged. Keeping them separate is going to cause more harm than good with unexpected interactions. We could still have 2 plugins and simply move the prebind action currently don in ipa-pwd-extop to the otp plugin by making some more code common. But it is probably easier to just merge OTP into ipa-pwd-extop right now than try to do a huge refactoring. We can always refactor the ipa-pwd-extop plugin later. The attached patches encompass an initial review of the companion daemon and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp into ipa-pwd-extop appears to have broken something during install, but I don't have enough familiarity with 389 to understand what I've broken. If I upgrade after an install, it appears to work. An RPM with the patches is available here: http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935 Nathan / Rob / Simo, could you take a look and see what might be broken in ipa-pwd-extop? While I'm not quite sure what the problem was, I do know it appeared on the stock 3.2 F19 RPMs. I also fixed it by accident. I am certain it is unrelated to these patches. I have now tested install and upgrade with the six patches in the previous email and everything is in order, including permissions. At this point, we just need reviews/ACKs. Nathaniel I tested IPA server upgrades, new installs and also adding 3.2+OTP replica for F18 3.1.4 IPA master. Everything seemed to work fine (when I added my patch 407 fixing the replication), I did not see any breakage. Issues I found with too much logging I reported should now be fixed on github, so this should be OK. So it is an ACK from my side if Rob does not discover some blocking issue. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote: On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote: On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Ok, but this does not answer my question. We definitely need to *always* run our other preop plugin as we do sanity checks like verifying if the user is enabled/disabled etc... Also we need to understand how to deal with migrating password auth when OTP is enabeld. TBH I think we should not have a separate OTP-auth plugin but we should probably have a single plugin that handles authentication and the 2 should be merged. Keeping them separate is going to cause more harm than good with unexpected interactions. We could still have 2 plugins and simply move the prebind action currently don in ipa-pwd-extop to the otp plugin by making some more code common. But it is probably easier to just merge OTP into ipa-pwd-extop right now than try to do a huge refactoring. We can always refactor the ipa-pwd-extop plugin later. The attached patches encompass an initial review of the companion daemon and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp into ipa-pwd-extop appears to have broken something during install, but I don't have enough familiarity with 389 to understand what I've broken. If I upgrade after an install, it appears to work. An RPM with the patches is available here: http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935 Nathan / Rob / Simo, could you take a look and see what might be broken in ipa-pwd-extop? how did it fail ? Do you have an install log file that shows the error ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Continuing with otp.c: - what does 'egress' mean ? (can you just use 'done' as a standard label for exceptions ?) Egress: Noun - The action of going out of or leaving a place: direct means of access and egress. Verb - Go out of or leave (a place). In short: ingress means to enter and egress means to exit. I have changed all 'egress' labels to 'done'. - Is it ok to call PK11_DestroyContext() if ctx is NULL ? Can't find much documentation but see #276314 / #276311 on bugzilla.mozilla.org I added if's for all of these just to be defensive. - Can you please add a comment that describe which HMAC algorithm are you using (or a reference to a RFC of it ?) Unfortunately NSS makes thins a lot more cryptic than it should :( Also adding comments before the various NSS invocation to explain what they do would help, this code is obscure. Unfortunately, that codes is mostly copy/paste from an NSS example of how to do HMAC. It is just as unclear to me as it is to you. I have added a link to the example with a note about me not understanding how it works... We should have Bob Relyea (cc'd) from the NSS development team take a look at it. -NGK The good news is that it passes all the unit tests which use values defined in the RFC. Also, valgrind reports no leaks or other errors. So even if I don't know *how* it works, I do know that it does, in fact, work. - Why do we have ipa_otp_hotp if we implement only totp ? TOTP is a specialized case of HOTP. It is simply an alternative mechanism for calculating the counter input to HOTP. Note that ipa_otp_totp() is basically a one-liner. Since you *have* to implement HOTP to get TOTP, you might as well expose the HOTP implementation for future use. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On Thu, 2013-05-02 at 22:29 -0400, Rob Crittenden wrote:
Simo Sorce wrote:
On Thu, 2013-05-02 at 17:57 -0400, Rob Crittenden wrote:
Simo Sorce wrote:
On Thu, 2013-05-02 at 15:24 -0400, Nathaniel McCallum wrote:
On Thu, 2013-05-02 at 12:18 -0400, Nathaniel McCallum wrote:
Attached are the patches from the ongoing OTP review with rcrit. We
believe these to be ready to merge. Please review. The first two patches
just add the required schema. The third patch adds support for OTP to
kdb. The fourth adds ipa-otpd, the otp companion daemon. The fifth, adds
the 389DS bind plugin. The sixth patch is cosmetic (.gitignore).
Code for managing tokens (CLI or GUI) remains to be written, though I do
have a rudimentary script for adding tokens for testing.
KNOWN ISSUES
1. ipa-otpd runs as root. This trade-off exists to permit autobinding
for this PoC. Ideally, ipa-otpd would run as its own unprivileged user.
I'd like to address this for the N+1 release.
2. krb5 currently requires the top three patches here in order to
properly trigger the otp code path:
https://github.com/greghudson/krb5/commits/keycheck. These should
hopefully be merged upstream soon and will be backported to krb5 1.11 in
Fedora 19 shortly.
3. krb5 tickets can't be issued. This is due to an upstream ticket
issuance bug that was discovered on Monday. This occurs *after* the OTP
has already been validated. We are working on a fix for this.
rcrit noticed that I wasn't using pkgconfig in patch #5, which I fixed.
He also merged patch #6. Attached are the five remaining patches.
Nathaniel
Will do one patch at a time as these are huge.
I think you should have separate mail threads per patch so that each can
be independently tracked in patchwork.
These patches are so huge I am going to have to write separate mails for
each anyway.
Patch 1 NACK:
- I see GPLv2 boiler plate, but we should use v3.
- Bad indentation with 2 spaces indent in several places.
- not required, but I find much better to use braces around single line
ifs, not only for pure stylistic reasons but for defensive programming,
it's very common to make mistakes later when modifying existing code and
forget to add braces when adding lines to the if statement.
- we do not do a new line after a the return type when declaring
functions.
Not ok:
int
fn_name()
{
ok:
int fn_name()
{
- Constructs like the following are not good:
if (slapi_entry_attr_find(e, type, attr) != 0 || !attr)
Makes debugging hard and reading hard. It should be:
ret = slapi_entry_attr_find(e, type, attr);
if (ret != 0 || attr) {
do something;
}
In general it is not ok to call a function and test its value within an
if statement except for trivial ones that return booleans.
always:
ret = fn()
if (ret == ?) { }
- (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
(although I know slapi_ch_malloc() currently just aborts on failure, I
think that is plain wrong which is why I would prefer using
malloc/strdup, but well, I guess this is not something I am going to
care too much about for now).
- Please do not use if (1) { ... } constructs, it makes no sense, simply
remove the if statement and leave the contents.
- Please if you can use 'done' as a label to get out of a function
- token_decode(), credentials_parse() and ipa_otp_do_otp_auth() are
effectively returning a boolean state, please make them return bool and
true for success, false for failure.
(ps: I see this all over, please use bool everywhere you return
effectively a boolean state, not going to point out every single
function from now on)
- indentation issues at lines 524 and 527 of the patch, both case should
align after the previous line '('
- another bad testing pattern:
do not do things like:
ret = foo() == 0
if (ret) { ... }
do:
ret = fn()
if (ret == 0) { ... }
- Using a single ipa_otp_postop() function instead of one function per
operation makes things less clear, as you have to create the boilerplate
for each function seaprately anyway and then most of the function is in
the switch case statements which are completely separate. The only
common code is the initial checks that you have already split off in
_stared()/_oktodo() functions anyway.
Having separate function per operation would be preferable I think.
- bad indentation line 1054,1055,1065,1074,1092 and so on ... they
should be indented after the preceding line(s) '('
Also this is often the case with slapi_log_error() functions too, please
indent after the opening '(' on previous lines, not at a random
indentation.
- Is the logic with auth_type_used correct ?
At least the name of the variable sounds misleading, because you set it
only if the authentication was successful but not set if it was 'used'
but was unsuccessful. Made me
Re: [Freeipa-devel] Final OTP Review
On Fri, 2013-05-03 at 01:27 -0400, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... Continuing with otp.c: - what does 'egress' mean ? (can you just use 'done' as a standard label for exceptions ?) Egress: Noun - The action of going out of or leaving a place: direct means of access and egress. Verb - Go out of or leave (a place). In short: ingress means to enter and egress means to exit. I have changed all 'egress' labels to 'done'. - Is it ok to call PK11_DestroyContext() if ctx is NULL ? Can't find much documentation but see #276314 / #276311 on bugzilla.mozilla.org I added if's for all of these just to be defensive. - Can you please add a comment that describe which HMAC algorithm are you using (or a reference to a RFC of it ?) Unfortunately NSS makes thins a lot more cryptic than it should :( Also adding comments before the various NSS invocation to explain what they do would help, this code is obscure. Unfortunately, that codes is mostly copy/paste from an NSS example of how to do HMAC. It is just as unclear to me as it is to you. I have added a link to the example with a note about me not understanding how it works... The good news is that it passes all the unit tests which use values defined in the RFC. Also, valgrind reports no leaks or other errors. So even if I don't know *how* it works, I do know that it does, in fact, work. Ok I took a deper look and now understand what it is doing. I think it is implementing RFC 6234 HMAC, but can't say for sure. The first NSS call creates a key container, the second initializes the context and tells NSS which HMAC algorythm to use and what is the key. Then the 3 calls simply (1) start the hmac calculation, (2) add in the data to be signed and (3) extracts the final signature on the data to be returned. - Why do we have ipa_otp_hotp if we implement only totp ? TOTP is a specialized case of HOTP. It is simply an alternative mechanism for calculating the counter input to HOTP. Note that ipa_otp_totp() is basically a one-liner. Since you *have* to implement HOTP to get TOTP, you might as well expose the HOTP implementation for future use. Yeah I've seen that, looks a bit weird but makes perfect sense. I do not have any more concerns on patch 1, so it's an ACK from me for that one. I haven't yet gone through the whole companion daemon patch :/ The otp ACIs one I think is wrong though, so still no full ack on the whole patchset. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Ok, but this does not answer my question. We definitely need to *always* run our other preop plugin as we do sanity checks like verifying if the user is enabled/disabled etc... Also we need to understand how to deal with migrating password auth when OTP is enabeld. TBH I think we should not have a separate OTP-auth plugin but we should probably have a single plugin that handles authentication and the 2 should be merged. Keeping them separate is going to cause more harm than good with unexpected interactions. We could still have 2 plugins and simply move the prebind action currently don in ipa-pwd-extop to the otp plugin by making some more code common. But it is probably easier to just merge OTP into ipa-pwd-extop right now than try to do a huge refactoring. We can always refactor the ipa-pwd-extop plugin later. Continuing with otp.c: - what does 'egress' mean ? (can you just use 'done' as a standard label for exceptions ?) Egress: Noun - The action of going out of or leaving a place: direct means of access and egress. Verb - Go out of or leave (a place). In short: ingress means to enter and egress means to exit. I have changed all 'egress' labels to 'done'. - Is it ok to call PK11_DestroyContext() if ctx is NULL ? Can't find much documentation but see #276314 / #276311 on bugzilla.mozilla.org I added if's for all of these just to be defensive. - Can you please add a comment that describe which HMAC algorithm are you using (or a reference to a RFC of it ?) Unfortunately NSS makes thins a lot more cryptic than it should :( Also adding comments before the various NSS invocation to explain what they do would help, this code is obscure. Unfortunately, that codes is mostly copy/paste from an NSS example of how to do HMAC. It is just as unclear to me as it is to you. I have added a link to the example with a note about me not understanding how it works... We should have Bob Relyea (cc'd) from the NSS development team take a look at it. I took a second stab afresh this morning and grokked it. No more concerns from me. NSS'a API does look *very* ugly... Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
On Fri, 2013-05-03 at 12:00 -0400, Nathaniel McCallum wrote: On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote: On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote: On 05/02/2013 10:27 PM, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... We would need to set the precedence if you want a predictable/guaranteed execution order. If a pre-BIND plug-in callback returns non-zero (which you should do when the plug-in sends the result to the client directly), it will cause other pre-bind plug-ins to not be called. This is actually how all pre-op plug-ins work. If a pre-op callback returns an error, we don't call the rest of the pre-op plug-ins in the list. Ok, but this does not answer my question. We definitely need to *always* run our other preop plugin as we do sanity checks like verifying if the user is enabled/disabled etc... Also we need to understand how to deal with migrating password auth when OTP is enabeld. TBH I think we should not have a separate OTP-auth plugin but we should probably have a single plugin that handles authentication and the 2 should be merged. Keeping them separate is going to cause more harm than good with unexpected interactions. We could still have 2 plugins and simply move the prebind action currently don in ipa-pwd-extop to the otp plugin by making some more code common. But it is probably easier to just merge OTP into ipa-pwd-extop right now than try to do a huge refactoring. We can always refactor the ipa-pwd-extop plugin later. +1. Can we do this after 3.2? This is an experimental feature after all... You must assure ipa-pwd-extop is always invoked in all bind cases. I am not welded on how you do it. However *merging* plugins later will be messy as you have to deal with configuration changes as the plugin .so will disappear, so I would rather do it now. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Final OTP Review
Simo Sorce wrote: On Fri, 2013-05-03 at 01:27 -0400, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... Continuing with otp.c: - what does 'egress' mean ? (can you just use 'done' as a standard label for exceptions ?) Egress: Noun - The action of going out of or leaving a place: direct means of access and egress. Verb - Go out of or leave (a place). In short: ingress means to enter and egress means to exit. I have changed all 'egress' labels to 'done'. - Is it ok to call PK11_DestroyContext() if ctx is NULL ? Can't find much documentation but see #276314 / #276311 on bugzilla.mozilla.org I added if's for all of these just to be defensive. - Can you please add a comment that describe which HMAC algorithm are you using (or a reference to a RFC of it ?) Unfortunately NSS makes thins a lot more cryptic than it should :( Also adding comments before the various NSS invocation to explain what they do would help, this code is obscure. Unfortunately, that codes is mostly copy/paste from an NSS example of how to do HMAC. It is just as unclear to me as it is to you. I have added a link to the example with a note about me not understanding how it works... The good news is that it passes all the unit tests which use values defined in the RFC. Also, valgrind reports no leaks or other errors. So even if I don't know *how* it works, I do know that it does, in fact, work. Ok I took a deper look and now understand what it is doing. I think it is implementing RFC 6234 HMAC, but can't say for sure. The first NSS call creates a key container, the second initializes the context and tells NSS which HMAC algorythm to use and what is the key. Then the 3 calls simply (1) start the hmac calculation, (2) add in the data to be signed and (3) extracts the final signature on the data to be returned. - Why do we have ipa_otp_hotp if we implement only totp ? TOTP is a specialized case of HOTP. It is simply an alternative mechanism for calculating the counter input to HOTP. Note that ipa_otp_totp() is basically a one-liner. Since you *have* to implement HOTP to get TOTP, you might as well expose the HOTP implementation for future use. Yeah I've seen that, looks a bit weird but makes perfect sense. I do not have any more concerns on patch 1, so it's an ACK from me for that one. I haven't yet gone through the whole companion daemon patch :/ The otp ACIs one I think is wrong though, so still no full ack on the whole patchset. Simo. This patch should fix things up. rob From 0072e0e7d2f43e775a257de04149eb31d8cccf03 Mon Sep 17 00:00:00 2001 From: Rob Crittenden rcrit...@redhat.com Date: Fri, 3 May 2013 15:27:13 -0400 Subject: [PATCH] fix access control --- install/share/default-aci.ldif| 6 - install/share/otp.ldif| 14 install/updates/40-otp.update | 8 --- ipaserver/install/plugins/update_anonymous_aci.py | 27 --- 4 files changed, 24 insertions(+), 31 deletions(-) diff --git a/install/share/default-aci.ldif b/install/share/default-aci.ldif index f173f79718e406e103cfe96026da041701382f7a..dcdd21ccd0ba19b15ef1ef0126da1c5392113994 100644 --- a/install/share/default-aci.ldif +++ b/install/share/default-aci.ldif @@ -3,7 +3,7 @@ dn: $SUFFIX changetype: modify add: aci -aci: (target != ldap:///idnsname=*,cn=dns,$SUFFIX;)(targetattr != userPassword ||
Re: [Freeipa-devel] Final OTP Review
Rob Crittenden wrote: Simo Sorce wrote: On Fri, 2013-05-03 at 01:27 -0400, Nathaniel McCallum wrote: All issues fixed unless noted below... The attached patches are tested to work. On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote: - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure (although I know slapi_ch_malloc() currently just aborts on failure, I think that is plain wrong which is why I would prefer using malloc/strdup, but well, I guess this is not something I am going to care too much about for now). Not fixed. - Is the logic with auth_type_used correct ? At least the name of the variable sounds misleading, because you set it only if the authentication was successful but not set if it was 'used' but was unsuccessful. Made me look at it multiple times to reconstuct the logic. The var name could be better, however I also want a comment that explain exactly how we are going to manage authentication and fallbacks here as that logic is critical and is useful for anyone that is going to have to change this code later in order not to break it. The variable is now gone. I re-factored the section to make the logic clearer and put a nice big comment up top. - General question: how does this PRE_BIND plugin interact with ipapwd_pre_bind() in the ipa-pwd-extop() plugin ? Are you going to cause that plugin not to run by returning a result directly in this function ? Or is this plugin configured to run only after the previous one went through ? I ask because I do not see any ordering information in the cn=config plugin configuration so it is not immediately clear to me. That is a good question for Nathan since he wrote this part of the code... Continuing with otp.c: - what does 'egress' mean ? (can you just use 'done' as a standard label for exceptions ?) Egress: Noun - The action of going out of or leaving a place: direct means of access and egress. Verb - Go out of or leave (a place). In short: ingress means to enter and egress means to exit. I have changed all 'egress' labels to 'done'. - Is it ok to call PK11_DestroyContext() if ctx is NULL ? Can't find much documentation but see #276314 / #276311 on bugzilla.mozilla.org I added if's for all of these just to be defensive. - Can you please add a comment that describe which HMAC algorithm are you using (or a reference to a RFC of it ?) Unfortunately NSS makes thins a lot more cryptic than it should :( Also adding comments before the various NSS invocation to explain what they do would help, this code is obscure. Unfortunately, that codes is mostly copy/paste from an NSS example of how to do HMAC. It is just as unclear to me as it is to you. I have added a link to the example with a note about me not understanding how it works... The good news is that it passes all the unit tests which use values defined in the RFC. Also, valgrind reports no leaks or other errors. So even if I don't know *how* it works, I do know that it does, in fact, work. Ok I took a deper look and now understand what it is doing. I think it is implementing RFC 6234 HMAC, but can't say for sure. The first NSS call creates a key container, the second initializes the context and tells NSS which HMAC algorythm to use and what is the key. Then the 3 calls simply (1) start the hmac calculation, (2) add in the data to be signed and (3) extracts the final signature on the data to be returned. - Why do we have ipa_otp_hotp if we implement only totp ? TOTP is a specialized case of HOTP. It is simply an alternative mechanism for calculating the counter input to HOTP. Note that ipa_otp_totp() is basically a one-liner. Since you *have* to implement HOTP to get TOTP, you might as well expose the HOTP implementation for future use. Yeah I've seen that, looks a bit weird but makes perfect sense. I do not have any more concerns on patch 1, so it's an ACK from me for that one. I haven't yet gone through the whole companion daemon patch :/ The otp ACIs one I think is wrong though, so still no full ack on the whole patchset. Simo. This patch should fix things up. rob The ACIs to let a user manage their own OTP needed some tweaking. It was loading in the wrong place for new installs and in both cases it lacked read access to objectclass so nothing was actually being granted. rob From 614e46cbce0672fe55ab23e1e95ef712a4749db4 Mon Sep 17 00:00:00 2001 From: Rob Crittenden rcrit...@redhat.com Date: Fri, 3 May 2013 15:27:13 -0400 Subject: [PATCH] Fix access control for OTP --- install/share/default-aci.ldif| 10 - install/share/otp.ldif| 14 install/updates/40-otp.update | 11 ++--- ipaserver/install/plugins/update_anonymous_aci.py | 27 --- 4 files changed, 30 insertions(+), 32 deletions(-) diff --git a/install/share/default-aci.ldif b/install/share/default-aci.ldif index
Re: [Freeipa-devel] Final OTP Review
On Thu, 2013-05-02 at 15:24 -0400, Nathaniel McCallum wrote:
On Thu, 2013-05-02 at 12:18 -0400, Nathaniel McCallum wrote:
Attached are the patches from the ongoing OTP review with rcrit. We
believe these to be ready to merge. Please review. The first two patches
just add the required schema. The third patch adds support for OTP to
kdb. The fourth adds ipa-otpd, the otp companion daemon. The fifth, adds
the 389DS bind plugin. The sixth patch is cosmetic (.gitignore).
Code for managing tokens (CLI or GUI) remains to be written, though I do
have a rudimentary script for adding tokens for testing.
KNOWN ISSUES
1. ipa-otpd runs as root. This trade-off exists to permit autobinding
for this PoC. Ideally, ipa-otpd would run as its own unprivileged user.
I'd like to address this for the N+1 release.
2. krb5 currently requires the top three patches here in order to
properly trigger the otp code path:
https://github.com/greghudson/krb5/commits/keycheck. These should
hopefully be merged upstream soon and will be backported to krb5 1.11 in
Fedora 19 shortly.
3. krb5 tickets can't be issued. This is due to an upstream ticket
issuance bug that was discovered on Monday. This occurs *after* the OTP
has already been validated. We are working on a fix for this.
rcrit noticed that I wasn't using pkgconfig in patch #5, which I fixed.
He also merged patch #6. Attached are the five remaining patches.
Nathaniel
Will do one patch at a time as these are huge.
I think you should have separate mail threads per patch so that each can
be independently tracked in patchwork.
These patches are so huge I am going to have to write separate mails for
each anyway.
Patch 1 NACK:
- I see GPLv2 boiler plate, but we should use v3.
- Bad indentation with 2 spaces indent in several places.
- not required, but I find much better to use braces around single line
ifs, not only for pure stylistic reasons but for defensive programming,
it's very common to make mistakes later when modifying existing code and
forget to add braces when adding lines to the if statement.
- we do not do a new line after a the return type when declaring
functions.
Not ok:
int
fn_name()
{
ok:
int fn_name()
{
- Constructs like the following are not good:
if (slapi_entry_attr_find(e, type, attr) != 0 || !attr)
Makes debugging hard and reading hard. It should be:
ret = slapi_entry_attr_find(e, type, attr);
if (ret != 0 || attr) {
do something;
}
In general it is not ok to call a function and test its value within an
if statement except for trivial ones that return booleans.
always:
ret = fn()
if (ret == ?) { }
- (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
(although I know slapi_ch_malloc() currently just aborts on failure, I
think that is plain wrong which is why I would prefer using
malloc/strdup, but well, I guess this is not something I am going to
care too much about for now).
- Please do not use if (1) { ... } constructs, it makes no sense, simply
remove the if statement and leave the contents.
- Please if you can use 'done' as a label to get out of a function
- token_decode(), credentials_parse() and ipa_otp_do_otp_auth() are
effectively returning a boolean state, please make them return bool and
true for success, false for failure.
(ps: I see this all over, please use bool everywhere you return
effectively a boolean state, not going to point out every single
function from now on)
- indentation issues at lines 524 and 527 of the patch, both case should
align after the previous line '('
- another bad testing pattern:
do not do things like:
ret = foo() == 0
if (ret) { ... }
do:
ret = fn()
if (ret == 0) { ... }
- Using a single ipa_otp_postop() function instead of one function per
operation makes things less clear, as you have to create the boilerplate
for each function seaprately anyway and then most of the function is in
the switch case statements which are completely separate. The only
common code is the initial checks that you have already split off in
_stared()/_oktodo() functions anyway.
Having separate function per operation would be preferable I think.
- bad indentation line 1054,1055,1065,1074,1092 and so on ... they
should be indented after the preceding line(s) '('
Also this is often the case with slapi_log_error() functions too, please
indent after the opening '(' on previous lines, not at a random
indentation.
- Is the logic with auth_type_used correct ?
At least the name of the variable sounds misleading, because you set it
only if the authentication was successful but not set if it was 'used'
but was unsuccessful. Made me look at it multiple times to reconstuct
the logic. The var name could be better, however I also want a comment
that explain exactly how we are going to manage authentication and
fallbacks here as that logic is critical and is useful for anyone that
is going to have to change this code later in order not to break it.
- What about the
Re: [Freeipa-devel] Final OTP Review
Simo Sorce wrote:
On Thu, 2013-05-02 at 15:24 -0400, Nathaniel McCallum wrote:
On Thu, 2013-05-02 at 12:18 -0400, Nathaniel McCallum wrote:
Attached are the patches from the ongoing OTP review with rcrit. We
believe these to be ready to merge. Please review. The first two patches
just add the required schema. The third patch adds support for OTP to
kdb. The fourth adds ipa-otpd, the otp companion daemon. The fifth, adds
the 389DS bind plugin. The sixth patch is cosmetic (.gitignore).
Code for managing tokens (CLI or GUI) remains to be written, though I do
have a rudimentary script for adding tokens for testing.
KNOWN ISSUES
1. ipa-otpd runs as root. This trade-off exists to permit autobinding
for this PoC. Ideally, ipa-otpd would run as its own unprivileged user.
I'd like to address this for the N+1 release.
2. krb5 currently requires the top three patches here in order to
properly trigger the otp code path:
https://github.com/greghudson/krb5/commits/keycheck. These should
hopefully be merged upstream soon and will be backported to krb5 1.11 in
Fedora 19 shortly.
3. krb5 tickets can't be issued. This is due to an upstream ticket
issuance bug that was discovered on Monday. This occurs *after* the OTP
has already been validated. We are working on a fix for this.
rcrit noticed that I wasn't using pkgconfig in patch #5, which I fixed.
He also merged patch #6. Attached are the five remaining patches.
Nathaniel
Will do one patch at a time as these are huge.
I think you should have separate mail threads per patch so that each can
be independently tracked in patchwork.
These patches are so huge I am going to have to write separate mails for
each anyway.
Patch 1 NACK:
- I see GPLv2 boiler plate, but we should use v3.
- Bad indentation with 2 spaces indent in several places.
- not required, but I find much better to use braces around single line
ifs, not only for pure stylistic reasons but for defensive programming,
it's very common to make mistakes later when modifying existing code and
forget to add braces when adding lines to the if statement.
- we do not do a new line after a the return type when declaring
functions.
Not ok:
int
fn_name()
{
ok:
int fn_name()
{
- Constructs like the following are not good:
if (slapi_entry_attr_find(e, type, attr) != 0 || !attr)
Makes debugging hard and reading hard. It should be:
ret = slapi_entry_attr_find(e, type, attr);
if (ret != 0 || attr) {
do something;
}
In general it is not ok to call a function and test its value within an
if statement except for trivial ones that return booleans.
always:
ret = fn()
if (ret == ?) { }
- (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
(although I know slapi_ch_malloc() currently just aborts on failure, I
think that is plain wrong which is why I would prefer using
malloc/strdup, but well, I guess this is not something I am going to
care too much about for now).
- Please do not use if (1) { ... } constructs, it makes no sense, simply
remove the if statement and leave the contents.
- Please if you can use 'done' as a label to get out of a function
- token_decode(), credentials_parse() and ipa_otp_do_otp_auth() are
effectively returning a boolean state, please make them return bool and
true for success, false for failure.
(ps: I see this all over, please use bool everywhere you return
effectively a boolean state, not going to point out every single
function from now on)
- indentation issues at lines 524 and 527 of the patch, both case should
align after the previous line '('
- another bad testing pattern:
do not do things like:
ret = foo() == 0
if (ret) { ... }
do:
ret = fn()
if (ret == 0) { ... }
- Using a single ipa_otp_postop() function instead of one function per
operation makes things less clear, as you have to create the boilerplate
for each function seaprately anyway and then most of the function is in
the switch case statements which are completely separate. The only
common code is the initial checks that you have already split off in
_stared()/_oktodo() functions anyway.
Having separate function per operation would be preferable I think.
- bad indentation line 1054,1055,1065,1074,1092 and so on ... they
should be indented after the preceding line(s) '('
Also this is often the case with slapi_log_error() functions too, please
indent after the opening '(' on previous lines, not at a random
indentation.
- Is the logic with auth_type_used correct ?
At least the name of the variable sounds misleading, because you set it
only if the authentication was successful but not set if it was 'used'
but was unsuccessful. Made me look at it multiple times to reconstuct
the logic. The var name could be better, however I also want a comment
that explain exactly how we are going to manage authentication and
fallbacks here as that logic is critical and is useful for anyone that
is going to have to change this code later in order not to break it.
- What about the comments labeled NGK ?
Re: [Freeipa-devel] Final OTP Review
On Thu, 2013-05-02 at 17:57 -0400, Rob Crittenden wrote:
Simo Sorce wrote:
On Thu, 2013-05-02 at 15:24 -0400, Nathaniel McCallum wrote:
On Thu, 2013-05-02 at 12:18 -0400, Nathaniel McCallum wrote:
Attached are the patches from the ongoing OTP review with rcrit. We
believe these to be ready to merge. Please review. The first two patches
just add the required schema. The third patch adds support for OTP to
kdb. The fourth adds ipa-otpd, the otp companion daemon. The fifth, adds
the 389DS bind plugin. The sixth patch is cosmetic (.gitignore).
Code for managing tokens (CLI or GUI) remains to be written, though I do
have a rudimentary script for adding tokens for testing.
KNOWN ISSUES
1. ipa-otpd runs as root. This trade-off exists to permit autobinding
for this PoC. Ideally, ipa-otpd would run as its own unprivileged user.
I'd like to address this for the N+1 release.
2. krb5 currently requires the top three patches here in order to
properly trigger the otp code path:
https://github.com/greghudson/krb5/commits/keycheck. These should
hopefully be merged upstream soon and will be backported to krb5 1.11 in
Fedora 19 shortly.
3. krb5 tickets can't be issued. This is due to an upstream ticket
issuance bug that was discovered on Monday. This occurs *after* the OTP
has already been validated. We are working on a fix for this.
rcrit noticed that I wasn't using pkgconfig in patch #5, which I fixed.
He also merged patch #6. Attached are the five remaining patches.
Nathaniel
Will do one patch at a time as these are huge.
I think you should have separate mail threads per patch so that each can
be independently tracked in patchwork.
These patches are so huge I am going to have to write separate mails for
each anyway.
Patch 1 NACK:
- I see GPLv2 boiler plate, but we should use v3.
- Bad indentation with 2 spaces indent in several places.
- not required, but I find much better to use braces around single line
ifs, not only for pure stylistic reasons but for defensive programming,
it's very common to make mistakes later when modifying existing code and
forget to add braces when adding lines to the if statement.
- we do not do a new line after a the return type when declaring
functions.
Not ok:
int
fn_name()
{
ok:
int fn_name()
{
- Constructs like the following are not good:
if (slapi_entry_attr_find(e, type, attr) != 0 || !attr)
Makes debugging hard and reading hard. It should be:
ret = slapi_entry_attr_find(e, type, attr);
if (ret != 0 || attr) {
do something;
}
In general it is not ok to call a function and test its value within an
if statement except for trivial ones that return booleans.
always:
ret = fn()
if (ret == ?) { }
- (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
(although I know slapi_ch_malloc() currently just aborts on failure, I
think that is plain wrong which is why I would prefer using
malloc/strdup, but well, I guess this is not something I am going to
care too much about for now).
- Please do not use if (1) { ... } constructs, it makes no sense, simply
remove the if statement and leave the contents.
- Please if you can use 'done' as a label to get out of a function
- token_decode(), credentials_parse() and ipa_otp_do_otp_auth() are
effectively returning a boolean state, please make them return bool and
true for success, false for failure.
(ps: I see this all over, please use bool everywhere you return
effectively a boolean state, not going to point out every single
function from now on)
- indentation issues at lines 524 and 527 of the patch, both case should
align after the previous line '('
- another bad testing pattern:
do not do things like:
ret = foo() == 0
if (ret) { ... }
do:
ret = fn()
if (ret == 0) { ... }
- Using a single ipa_otp_postop() function instead of one function per
operation makes things less clear, as you have to create the boilerplate
for each function seaprately anyway and then most of the function is in
the switch case statements which are completely separate. The only
common code is the initial checks that you have already split off in
_stared()/_oktodo() functions anyway.
Having separate function per operation would be preferable I think.
- bad indentation line 1054,1055,1065,1074,1092 and so on ... they
should be indented after the preceding line(s) '('
Also this is often the case with slapi_log_error() functions too, please
indent after the opening '(' on previous lines, not at a random
indentation.
- Is the logic with auth_type_used correct ?
At least the name of the variable sounds misleading, because you set it
only if the authentication was successful but not set if it was 'used'
but was unsuccessful. Made me look at it multiple times to reconstuct
the logic. The var name could be
Re: [Freeipa-devel] Final OTP Review
Simo Sorce wrote:
On Thu, 2013-05-02 at 17:57 -0400, Rob Crittenden wrote:
Simo Sorce wrote:
On Thu, 2013-05-02 at 15:24 -0400, Nathaniel McCallum wrote:
On Thu, 2013-05-02 at 12:18 -0400, Nathaniel McCallum wrote:
Attached are the patches from the ongoing OTP review with rcrit. We
believe these to be ready to merge. Please review. The first two patches
just add the required schema. The third patch adds support for OTP to
kdb. The fourth adds ipa-otpd, the otp companion daemon. The fifth, adds
the 389DS bind plugin. The sixth patch is cosmetic (.gitignore).
Code for managing tokens (CLI or GUI) remains to be written, though I do
have a rudimentary script for adding tokens for testing.
KNOWN ISSUES
1. ipa-otpd runs as root. This trade-off exists to permit autobinding
for this PoC. Ideally, ipa-otpd would run as its own unprivileged user.
I'd like to address this for the N+1 release.
2. krb5 currently requires the top three patches here in order to
properly trigger the otp code path:
https://github.com/greghudson/krb5/commits/keycheck. These should
hopefully be merged upstream soon and will be backported to krb5 1.11 in
Fedora 19 shortly.
3. krb5 tickets can't be issued. This is due to an upstream ticket
issuance bug that was discovered on Monday. This occurs *after* the OTP
has already been validated. We are working on a fix for this.
rcrit noticed that I wasn't using pkgconfig in patch #5, which I fixed.
He also merged patch #6. Attached are the five remaining patches.
Nathaniel
Will do one patch at a time as these are huge.
I think you should have separate mail threads per patch so that each can
be independently tracked in patchwork.
These patches are so huge I am going to have to write separate mails for
each anyway.
Patch 1 NACK:
- I see GPLv2 boiler plate, but we should use v3.
- Bad indentation with 2 spaces indent in several places.
- not required, but I find much better to use braces around single line
ifs, not only for pure stylistic reasons but for defensive programming,
it's very common to make mistakes later when modifying existing code and
forget to add braces when adding lines to the if statement.
- we do not do a new line after a the return type when declaring
functions.
Not ok:
int
fn_name()
{
ok:
int fn_name()
{
- Constructs like the following are not good:
if (slapi_entry_attr_find(e, type, attr) != 0 || !attr)
Makes debugging hard and reading hard. It should be:
ret = slapi_entry_attr_find(e, type, attr);
if (ret != 0 || attr) {
do something;
}
In general it is not ok to call a function and test its value within an
if statement except for trivial ones that return booleans.
always:
ret = fn()
if (ret == ?) { }
- (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
(although I know slapi_ch_malloc() currently just aborts on failure, I
think that is plain wrong which is why I would prefer using
malloc/strdup, but well, I guess this is not something I am going to
care too much about for now).
- Please do not use if (1) { ... } constructs, it makes no sense, simply
remove the if statement and leave the contents.
- Please if you can use 'done' as a label to get out of a function
- token_decode(), credentials_parse() and ipa_otp_do_otp_auth() are
effectively returning a boolean state, please make them return bool and
true for success, false for failure.
(ps: I see this all over, please use bool everywhere you return
effectively a boolean state, not going to point out every single
function from now on)
- indentation issues at lines 524 and 527 of the patch, both case should
align after the previous line '('
- another bad testing pattern:
do not do things like:
ret = foo() == 0
if (ret) { ... }
do:
ret = fn()
if (ret == 0) { ... }
- Using a single ipa_otp_postop() function instead of one function per
operation makes things less clear, as you have to create the boilerplate
for each function seaprately anyway and then most of the function is in
the switch case statements which are completely separate. The only
common code is the initial checks that you have already split off in
_stared()/_oktodo() functions anyway.
Having separate function per operation would be preferable I think.
- bad indentation line 1054,1055,1065,1074,1092 and so on ... they
should be indented after the preceding line(s) '('
Also this is often the case with slapi_log_error() functions too, please
indent after the opening '(' on previous lines, not at a random
indentation.
- Is the logic with auth_type_used correct ?
At least the name of the variable sounds misleading, because you set it
only if the authentication was successful but not set if it was 'used'
but was unsuccessful. Made me look at it multiple times to reconstuct
the logic. The var name could be better, however I also want a comment
that explain exactly how we are going to manage authentication and
fallbacks here as that logic is critical and is useful for anyone that
is going to have to change this
