On Tue, 2014-12-02 at 12:20 -0500, Nathaniel McCallum wrote: > On Tue, 2014-12-02 at 17:56 +0100, Petr Vobornik wrote: > > On 12/02/2014 05:39 PM, thierry bordaz wrote: > > > On 12/02/2014 05:24 PM, Nathaniel McCallum wrote: > > >> On Tue, 2014-12-02 at 17:12 +0100, Martin Kosek wrote: > > >>> On 12/02/2014 04:56 PM, Nathaniel McCallum wrote: > > >>>> On Tue, 2014-12-02 at 14:05 +0100, thierry bordaz wrote: > > >>>>> On 11/12/2014 11:37 PM, Nathaniel McCallum wrote: > > >>>>> > > >>>>>> On Mon, 2014-11-10 at 08:28 +0100, Martin Kosek wrote: > > >>>>>>> On 11/07/2014 04:44 PM, Petr Vobornik wrote: > > >>>>>>>> On 7.11.2014 08:58, Martin Kosek wrote: > > >>>>>>>>> On 11/04/2014 05:17 PM, Nathaniel McCallum wrote: > > >>>>>>>>>> On Wed, 2014-10-29 at 09:34 -0400, Nathaniel McCallum wrote: > > >>>>>>>>>>> On Wed, 2014-10-29 at 12:21 +0100, Petr Viktorin wrote: > > >>>>>>>>>>>> On 10/29/2014 10:37 AM, Martin Kosek wrote: > > >>>>>>>>>>>>> On 10/28/2014 09:59 PM, Nathaniel McCallum wrote: > > >>>>>>>>>>>>>> On Thu, 2014-10-23 at 18:07 -0400, Nathaniel McCallum wrote: > > >>>>>>>>>>>>>>> This patch gives the administrator variables to control > > >>>>>>>>>>>>>>> the size of > > >>>>>>>>>>>>>>> the authentication and synchronization windows for OTP > > >>>>>>>>>>>>>>> tokens. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4511 > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> NOTE: There is one known issue with this patch which I > > >>>>>>>>>>>>>>> don't know > > >>>>>>>>>>>>>>> how to > > >>>>>>>>>>>>>>> solve. This patch changes the schema in > > >>>>>>>>>>>>>>> install/share/60ipaconfig.ldif. > > >>>>>>>>>>>>>>> On an upgrade, all of the new attributeTypes appear > > >>>>>>>>>>>>>>> correctly. > > >>>>>>>>>>>>>>> However, > > >>>>>>>>>>>>>>> the modifications to the pre-existing objectClass do not > > >>>>>>>>>>>>>>> show up > > >>>>>>>>>>>>>>> on the > > >>>>>>>>>>>>>>> server. What am I doing wrong? > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> After modifying ipaGuiConfig manually, everything in this > > >>>>>>>>>>>>>>> patch > > >>>>>>>>>>>>>>> works > > >>>>>>>>>>>>>>> just fine. > > >>>>>>>>>>>>>> This new version takes into account the new (proper) OIDs and > > >>>>>>>>>>>>>> attribute > > >>>>>>>>>>>>>> names. > > >>>>>>>>>>>>> Thanks Nathaniel! > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> The above known issue still remains. > > >>>>>>>>>>>>> Petr3, any idea what could have gone wrong? ObjectClass MAY > > >>>>>>>>>>>>> list > > >>>>>>>>>>>>> extension > > >>>>>>>>>>>>> should work just fine, AFAIK. > > >>>>>>>>>>>> You added a blank line to the LDIF file. This is an entry > > >>>>>>>>>>>> separator, so > > >>>>>>>>>>>> the objectClasses after the blank line don't belong to > > >>>>>>>>>>>> cn=schema, so > > >>>>>>>>>>>> they aren't considered in the update. > > >>>>>>>>>>>> Without the blank line it works fine. > > >>>>>>>>>>> Thanks for the catch! > > >>>>>>>>>>> > > >>>>>>>>>>> Here is a version without the blank line. > > >>>>>>>>>> I forgot to remove the old steps defines. This patch performs > > >>>>>>>>>> this > > >>>>>>>>>> cleanup. > > >>>>>>>>> I am now wondering, is the global config object really the nest > > >>>>>>>>> place to > > >>>>>>>>> add these OTP specific settings? > > >>>>>>>>> > > >>>>>>>>> I would prefer not to overload the object and instead: > > >>>>>>>>> - create new ipaOTPConfig objectclass > > >>>>>>>>> - add it to cn=otp,$SUFFIX > > >>>>>>>>> - create otpconfig-mod and otpconfig-show commands to follow an > > >>>>>>>>> example > > >>>>>>>>> of dnsconfig-* and trustconfig-* commands > > >>>>>>>>> > > >>>>>>>>> IMO, this would allow more flexibility for the OTP settings and > > >>>>>>>>> would > > >>>>>>>>> also scale better for the future updates. > > >>>>>>>> +1 > > >>>>>>>> > > >>>>>>>> I will comment the patch as if ^^ would not exist because it > > >>>>>>>> will still be > > >>>>>>>> needed in the new plugin. > > >>>>>>>> > > >>>>>>>> Because of ^^ I did not test, just read. > > >>>>>>>> > > >>>>>>>> 1. Got: > > >>>>>>>> install/ui/src/freeipa/serverconfig.js(135): lint warning: extra > > >>>>>>>> comma is not > > >>>>>>>> recommended in array initializers > > >>>>>>>> > > >>>>>>>> Please run: > > >>>>>>>> jsl -nofilelisting -nosummary -nologo -conf jsl.conf > > >>>>>>>> in install/ui directory > > >>>>>>>> > > >>>>>>>> The goal is no have no warnings and errors. > > >>>>>>>> > > >>>>>>>> 2. new attrs should be added to 'System: Read Global > > >>>>>>>> Configuration' managed > > >>>>>>>> permission > > >>>>>>> +1. Though if we go with OTP config, it should be called > > >>>>>>> > > >>>>>>> System: Read OTP Configuration > > >>>>>>> > > >>>>>>> Martin > > >>>>>> Attached is a new set of patches that replaces this single patch. > > >>>>>> This > > >>>>>> now fixes multiple issues. > > >>>>>> > > >>>>>> I now create two new entries: > > >>>>>> * cn=TOTP,cn=Token Config,cn=etc,$SUFFIX > > >>>>>> * cn=HOTP,cn=Token Config,cn=etc,$SUFFIX > > >>>>>> > > >>>>>> There are two corresponding CLI commands: > > >>>>>> * totpconfig-(show|mod) > > >>>>>> * hotpconfig-(show|mod) > > >>>>>> > > >>>>>> There is no UI support for this yet (pointers welcome). > > >>>>>> > > >>>>>> This is designed so that eventually tokens can grow a per-token > > >>>>>> override, but I have not yet implemented this feature (it should > > >>>>>> be easy > > >>>>>> in the future). > > >>>>>> > > >>>>>> Additionally, I had to do some shared refactoring to address > > >>>>>> issues in > > >>>>>> ipa-otp-lasttoken, which is why all of these are now merged into a > > >>>>>> single patch set. > > >>>>>> > > >>>>>> Nathaniel > > >>>>>> > > >>>>>> > > >>>>>> _______________________________________________ > > >>>>>> Freeipa-devel mailing list > > >>>>>> Freeipa-devel@redhat.com > > >>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel > > >>>>> Hello Nathaniel, > > >>>>> > > >>>>> Very few comments. > > >>>> Just as a reminder, patch 0001 is already ACKed. > > >>>> > > >>>>> On patch 0002: > > >>>>> Is it possible that we later define a spec with 'dflt' > > >>>>> contains OTP_CONFIG_AUTH_TYPE_DISABLED ? If yes it needs > > >>>>> to be > > >>>>> 32bits. > > >>>> Fixed. It was just a typo. > > >>>> > > >>>>> When otp_config_fini is it called ? > > >>>> Sadly, never. I admit that I am cargo-culting the lack of calling > > >>>> otp_config_fini(). Surely there must be a way to sanely tear this down > > >>>> when 389 shuts down? > > >>>> > > >>>>> On patch 0003: > > >>>>> In ipa-otp-lasttoken:58 you may use SLAPI_ATTR_OBJECTCLASS > > >>>>> (slapi-plugin.h). > > >>>> Fixed. > > >>>> > > >>>>> In ipa-otp-lasttoken:preop_mod , the test is_allowed is done > > >>>>> on the original entry (SLAPI_ENTRY_PRE_OP). That is the entry > > >>>>> untouched by others BE_PREOP/TXN_BE_PREOP plugins. Is that > > >>>>> the > > >>>>> entry you want to check ? > > >>>> Yes, the code is correct as written. We check to see if a change to the > > >>>> existing state would cause bad behavior. Then, if any such change is > > >>>> attempted (ipa_otp_lasttoken.c:205) we reject it. In the future we > > >>>> might > > >>>> improve this to be more granular regarding the values of the change. > > >>>> But > > >>>> for now it is good enough. > > >>>> > > >>>>> On patch 0004: > > >>>>> In otp_config.c:otp_config_window you may use > > >>>>> SLAPI_ATTR_OBJECTCLASS (slapi-plugin.h) > > >>>> Fixed. > > >>>> > > >>>>> in otp_token: bvtod if 'code' contains non digit > > >>>>> character ,'out' is not reset before return. > > >>>> Yes. If the input value is invalid, the function returns an error > > >>>> status > > >>>> and the state of the output is undefined. This is pretty normal > > >>>> behavior. > > >>>> > > >>>> I think the first three patches are ready for merge. The last patch > > >>>> still needs some polish on my part. However, the first three also > > >>>> fix an > > >>>> important, independent bug. So let's merge them as soon as you feel > > >>>> they > > >>>> are ready. > > >>>> > > >>>> Nathaniel > > >>> If the Python parts are also OK and acked by Petr Vobornik or anyone > > >>> else then > > >>> sure, we can merge them. > > >> Python code is confined to patch 0004, so I think we just need Thierry's > > >> ACK on 0001-0003. > > > > > > Nathaniel, > > > > > > Thanks for all your explanations. ACK for the pacthes 0001-0002-0003. > > > > > > regarding the DS plugin part of 0004, the patch is good to me. For the > > > ipa plugins part I am too novice. > > > > Just a remainder: the python part of patch 0004 is still being > > discussed: > > http://www.redhat.com/archives/freeipa-devel/2014-November/msg00295.html > > Correct. Please merge 0001-0003 as they are ACKed, but NOT 0004. I will > have an alternative proposal for 0004 shortly.
I found a small typo in patch 0002. I will attach it in reply to Petr's review email. Nathaniel _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel