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.
--
/ Alexander Bokovoy
>From 1c83f1943a635a4b2541addd9fc21b6155fb12d6 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <[email protected]>
Date: Wed, 25 Jun 2014 13:32:16 +0300
Subject: [PATCH 10/10] fixup! Add the otptoken-add-yubikey command
---
ipalib/plugins/otptoken_yubikey.py | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/ipalib/plugins/otptoken_yubikey.py
b/ipalib/plugins/otptoken_yubikey.py
index 30a81e5..b4f2188 100644
--- a/ipalib/plugins/otptoken_yubikey.py
+++ b/ipalib/plugins/otptoken_yubikey.py
@@ -18,6 +18,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
from ipalib import _, Str, IntEnum
+from ipalib.errors import NotFound
from ipalib.plugable import Registry
from ipalib.frontend import Command
from ipalib.plugins.otptoken import otptoken
@@ -78,7 +79,11 @@ class otptoken_add_yubikey(Command):
def forward(self, *args, **kwargs):
# Open the YubiKey
- yk = yubico.find_yubikey()
+ try:
+ yk = yubico.find_yubikey()
+ except yubico.yubikey.YubiKeyError, e:
+ raise NotFound(reason=_('No YubiKey found'))
+
assert yk.version_num() >= (2, 1)
# If no slot is specified, find the first free slot.
@@ -87,7 +92,7 @@ class otptoken_add_yubikey(Command):
used = yk.status().valid_configs()
kwargs['slot'] = sorted({1, 2}.difference(used))[0]
except IndexError:
- raise ValueError('No free YubiKey slot!')
+ raise NotFound(reason=_('No free YubiKey slot!'))
# Create the key (NOTE: the length is fixed).
key = os.urandom(20)
--
1.9.3
>From 3f8d3d009cc45fb28a937770bb370646a4261c39 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <[email protected]>
Date: Wed, 25 Jun 2014 09:37:18 +0300
Subject: [PATCH 07/10] fixup! Add the otptoken-add-yubikey command
---
VERSION | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/VERSION b/VERSION
index 65e5273..f0c2db5 100644
--- a/VERSION
+++ b/VERSION
@@ -89,5 +89,5 @@ IPA_DATA_VERSION=20100614120000
# #
########################################################
IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=93
-# Last change: mbasti - New record type added: DLV
+IPA_API_VERSION_MINOR=94
+# Last change: npmaccallum - otptoken-add-yubikey
--
1.9.3
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
