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

[Alexander Bokovoy]

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?


>2. Should the "import yubico" statement be inside of the
>otptoken_add_yubikey.forward() method to reduce server dependencies?
Here is how we do it in ipalibs/plugins/trust.py:
====================================================================
try:
    import pysss_murmur #pylint: disable=F0401
    _murmur_installed = True
except Exception, e:
    _murmur_installed = False

try:
    import pysss_nss_idmap #pylint: disable=F0401
    _nss_idmap_installed = True
except Exception, e:
    _nss_idmap_installed = False

if api.env.in_server and api.env.context in ['lite', 'server']:
    try:
        import ipaserver.dcerpc #pylint: disable=F0401
        _bindings_installed = True
    except ImportError:
        _bindings_installed = False
====================================================================

and then in the actual code we do 

if not _bindings_installed:
    raise errors.NotFound(
        name=_('AD Trust setup'),
        reason=_(
            'Cannot perform join operation without Samba 4 support '
            'installed. Make sure you have installed server-trust-ad '
            'sub-package of IPA'
        )
    )
   


>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().

>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.
>
>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.
>
>6. I'm not sure my use of assert or ValueError are correct. What should
>I do here?
>
>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.

-- 
/ Alexander Bokovoy