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

[Martin Kosek]

On 09/05/2013 06:25 AM, 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

I just noticed you use few sub-optimal calls which could slower the calls:

+        if owner is not None:
+            result = self.api.Command.user_show(owner)['result']
+            entry_attrs['ipatokenowner'] = result['dn']
+        return dn

...

+        if owner is not None:
+            result = self.api.Command.user_show(owner)['result']
+            filters = filters.replace("(ipatokenowner=%s)" % owner,
+                                      "(ipatokenowner=%s)" % result['dn'])
+

These commands will load the entire user, loads all the membership etc. etc.,
so it may result into several LDAP searches. But you use just the DN.

This call should be much faster:

# get DN of user object
dn = self.api.Object['group'].get_dn_if_exists(owner)

Not a functionality blocker, just a something that I would like to next patch
iteration.

Martin