[Freeipa-devel] [PATCH 0017] Add OTP support to ipalib CLI

[Jan Cholasta]

On 5.9.2013 06:25, Nathaniel McCallum wrote:
> This patch has a few problems that I'd like some help with. There are a
> few notes here as well.
>
> 1. The handling of the 'key' option is insecure. It should probably be
> treated like a password (hidden from logs, etc). However, in this case,
> it is binary, so I'm not quite sure how to do that. Passing it as a
> command line option may be nice for scripting, but is potentially a
> security problem if it ends up in bash.history. It would also be nice if
> the encoding were base32 instead of base64, since nearly all the OTP
> tools use this encoding.
>
> 2. The 'key' option also appears in otp-find. I'd like to suppress this.
> How?
>
> 3. I had to make the 'id' option optional to make the uuid
> autogeneration work in otp-add. However, this has the side-effect that
> 'id' is now optional in all the other commands. This is particularly bad
> in the case of otp-del, where calling this command with no ID
> transparently removes all tokens. How can I make this optional for
> otp-add but required for all other commands?
>
> 4. otp-import is not implemented. I spent a few hours looking and I
> didn't find any otp tool that actually uses this xml format for
> exporting. Should we implement this now or wait until someone can
> actually export data to us?
>
> 5. otp-del happily deletes the last token for a user. How can I find out
> the dn of the user executing the command? Also, what is the right
> exception to throw in pre_callback()?
>
> 6. user-show does not list the associated tokens for this user. Do we
> care? It is a single search: otp-find --owner npmccallum.
>
> 7. otp-add only prints the qr code if the --qrcode option is specified.
> This is for two reasons. First, and most importantly, the qr code
> doesn't fit on a standard 24x80 terminal. I wanted to avoid dumping
> garbage on people's screens by default. Second, you may not always want
> the qr code output (like for a hard token or manual code entry).
>
> Nathaniel
>

First, a nitpick:

-            values=(u'password', u'radius'),
+            values=(u'password', u'radius', u'otp'),

+ at register()
+class otp(LDAPObject):

IMO naming the authentication type and plugin just "otp" is confusing. 
We already support one time passwords for users (and hosts as well), so 
"otp" is ambiguous. I think you should at least rename the plugin to 
"otptoken".


+        # Resolve the user's dn
+        owner = entry_attrs.get('ipatokenowner', None)
+        if owner is not None:
+            result = self.api.Command.user_show(owner)['result']
+            owner = entry_attrs['ipatokenowner'] = result['dn']

I think you can rewrite the above as:

+        # Resolve the user's dn
+        owner = entry_attrs.get('ipatokenowner', None)
+        if owner is not None:
+            owner = self.api.Object.user.get_dn(owner)
+            entry_attrs['ipatokenowner'] = owner

This will save you several LDAP searches. You don't have to use 
get_dn_if_exists here, because if the user does not exist, it will be 
catched later in the "Get the issuer for the URI" block.


+        self.uri = "otpauth://totp/%s:%s?%s" % (args['issuer'], label, 
parameters)

You can't do this, because plugins are singletons. See the user and host 
plugins for how they handle the randompassword virtual attribute for 
inspiration.


+        entry_attrs['uri'] = self.uri

Please add a proper param to otp_add's output_params for "uri".


+            filters = filters.replace("(ipatokenowner=%s)" % owner,
+                                      "(ipatokenowner=%s)" % result['dn'])

Please do this in opt_find.args_options_2_entry (see my reply to your 
radius CLI patches for details).


Honza

-- 
Jan Cholasta