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'),
+@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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
