[Freeipa-devel] [PATCH 0058] Add the otptoken-add-yubikey command

[Nathaniel McCallum]

On Wed, 2014-06-25 at 09:53 -0400, Nathaniel McCallum wrote:
> On Wed, 2014-06-25 at 13:35 +0300, Alexander Bokovoy wrote:
> > On Mon, 23 Jun 2014, Nathaniel McCallum wrote:
> > >On Mon, 2014-06-23 at 10:29 +0300, Alexander Bokovoy wrote:
> > >> On Fri, 20 Jun 2014, Nathaniel McCallum wrote:
> > >> >On Thu, 2014-06-19 at 16:30 -0400, Nathaniel McCallum wrote:
> > >> >> This command behaves almost exactly like otptoken-add except:
> > >> >> 1. The new token data is written directly to a YubiKey
> > >> >> 2. The vendor/model/serial fields are populated from the YubiKey
> > >> >>
> > >> >> === NOTE ===
> > >> >> 1. This patch depends on the new Fedora package: python-yubico. If you
> > >> >> would like to help with the package review, please assign yourself here:
> > >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1111334
> > >> >
> > >> >New version of the patch. This one works (yay!).
> > >> >
> > >> >1. Because of the dependency on python-yubico, is this feature something
> > >> >we want in core FreeIPA? As a subpackage? Separate project altogether?
> > >> >The only dependency for python-yubico is pyusb.
> > >> I'd prefer to have it integrated but have a separate dummy subpackage
> > >> that pulls in all required dependencies, like, freeipa-tools-yubico. Instead
> > >> of failing when 'ipa otptoken-add-yubikey' is called, please wrap the
> > >> python-yubico import into a code that allows reporting a message back to
> > >> the user advising to install the package.
> > >>
> > >> Who is a supposed user for this command? IPA command line interface isn't
> > >> usually available on enrolled machines even though underlying Python
> > >> modules are all there. Are we talking about admins or just users?
> > >
> > >As discussed on IRC, we are currently hard-coding lots of optional
> > >dependencies. And breaking this apart into subpackages can be solved at
> > >a later point. YubiKey is also a unique case: we don't expect to be
> > >adding many more plugins like this.
> > >
> > >For these reasons, I have kept this as a hard dependency. To ease this
> > >transition, I have added python-yubico to F20 and EL6. You can help with
> > >the update review here:
> > >https://admin.fedoraproject.org/updates/python-yubico-1.2.1-3.fc20
> > >https://admin.fedoraproject.org/updates/python-yubico-1.2.1-3.el6
> > >
> > >> >3. This code currently emits a warning from the call to otptoken-add:
> > >> >WARNING: API Version number was not sent, forward compatibility not
> > >> >guaranteed. Assuming server's API version, 2.89
> > >> >
> > >> >How do I fix this?
> > >> Do not filter 'version' field in options in execute().
> > >
> > >I opted to not filter out version rather than hard-code it (pviktori's
> > >suggestion). This is based on the fact that otptoken-add-yubikey is
> > >tightly integrated with otptoken-add (even using some of the class
> > >attributes from otptoken).
> > >
> > >> >4. I am not sure why I have to delete the summary and value keys from
> > >> >the return dictionary. It would be nice to display this summary message
> > >> >just like otptoken-add.
> > >
> > >I still need help on this.
> > >
> > >> >5. Am I doing the ipatoken(vendor|model|serial) options correctly? These
> > >> >aren't user settable, but we need to pass them from the yubikey
> > >> >(client-side) to the server.
> > >
> > >This is no longer needed since I am doing everything in forward().
> > >However, listing these three as output params causes them to appear
> > >before the token's ID. I don't think this is the right way to output
> > >these. But this seems to me a framework issue.
> > >
> > >> >6. I'm not sure my use of assert or ValueError are correct. What should
> > >> >I do here?
> > >
> > >Still need help here.
> > Fixed this part.
> > 
> > >
> > >> >7. Considering that this is just a specialized invocation of
> > >> >otptoken-add, can't we do this all on the client-side? This is why I had
> > >> >originally used frontend.Local rather than frontend.Command.
> > >> You don't need to implement execute then, only forward, where you'll
> > >> forward your call to the server under otptoken_add name.
> > >>
> > >> Typically in #forward we call super's forward but that is because we
> > >> in Command.forward() we  simply forward the command to the remote backend,
> > >>  using the self.name. In your case we shouldn't really have a separate
> > >> command on the server under the same name, so you'll need to avoid
> > >> calling
> > >>
> > >> So, it should look like this:
> > >>
> > >>     def forward(self, *args, **kw):
> > >>         perform yubikey initialization
> > >>         filter out kw and args, if needed
> > >>         return self.Backend.rpcclient.forward('otptoken_add', *args, **kw)
> > >>
> > >> See service_show implementation for an example.
> > >
> > >Fixed.
> > I'm attaching few fixups to the patch that make it proper reporting for
> > non-Yubikey case and also properly update VERSION.
> > 
> > Provisional ACK.
> 
> Merged.

This patch includes everything above and fixes the missing ./makeapi
run.

Nathaniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-npmccallum-0058.4-Add-the-otptoken-add-yubikey-command.patch
Type: text/x-patch
Size: 8429 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140625/aa8c82f2/attachment.bin>