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

[Petr Viktorin]

I'm sorry for the late reply, I got caught up in other work and forgot 
about this thread.

On 09/05/2013 03:31 PM, Nathaniel McCallum wrote:
> On Thu, 2013-09-05 at 12:19 +0200, Petr Viktorin wrote:
>> On 09/05/2013 06:38 AM, Nathaniel McCallum wrote:
>>> On Thu, 2013-09-05 at 00:25 -0400, 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.
>>
>> Not only in bash_history; anyone can see command line parameters of
>> running programs.
>> We'll need to modify the framework to support more another password
>> parameter type.
>> The base32 on input/output can be added to that new type.
>
> To clarify, by scripting I meant calling this from a python script. In
> this case, the argument wouldn't show up in the argv. Sorry my wording
> wasn't clear here.
>
> The primary case where this will apply is in otp-import (if we implement
> it). We will parse the XML and call self.api.Command.otp_add() for each
> token found, including the key.
>
> So it would be good to have this option available in python but not the
> shell.

In Python you can just use the base64 module to convert between base64 
and base32. I don't think we need to go out of our way to make this easier.

>>>> 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()?
>>
>> Don't you want to check the token's owner rather than the current user?
>> You could use LastMemberError here, or make your own error.
>
> If the executing user has permissions to remove another user's token, we
> assume they are an admin and know what they are doing. So this case only
> arises if the executing user is removing his/her own tokens. For
> instance:
>
> if executor == owner and tokenCount(owner) == 1:
>    raise LastMemberError()

That's a non-intuitive assumption. I wouldn't base the behavior on it if 
it's not necessary (even if it was easy to get the executing user).
IMO, an admin should get the same safeguards as a regular user here. If 
we want to allow users that know what they are doing to remove their 
last token, we should a --force option.

>>>> 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).
>>
>> Would it make sense to add --qrcode to otp-show as well? If otp-add is
>> the only opportunity to get the QR code, it's is easy to miss.
>
> Only admins have permission to read 'key' from LDAP after creation.
> Standard users can create the OTP token object with a 'key', but cannot
> read back the key. Hence, the URI is only available at creation
> (provisioning) time.

OK
If you miss this chance, you can just delete that OTP and make another 
one, right?

-- 
Petr³