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

[Jan Cholasta]

On 13.9.2013 10:07, Jan Cholasta wrote:
> 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
>

+        # Generate a random uuid
+        if not entry_attrs.get('ipatokenuniqueid', None):
+            entry_attrs['ipatokenuniqueid'] = str(uuid.uuid4())
+            dn = DN("ipatokenuniqueid=%s" % 
entry_attrs['ipatokenuniqueid'], dn)
+
+        # Set a random key by default
+        if not entry_attrs.get('ipatokenotpkey', None):
+            key = random.SystemRandom().sample(range(255), KEY_LENGTH)
+            entry_attrs['ipatokenotpkey'] = "".join(map(chr, key))

This is not how default values are done, see the default_from keyword 
argument of Param.


+    def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
+        entry_attrs['uri'] = self.uri
+        return LDAPCreate.post_callback(self, ldap, dn, entry_attrs, 
*keys, **options)

"return dn" is OK here.


Honza

-- 
Jan Cholasta