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

[Nathaniel McCallum]

On Thu, 2013-09-12 at 13:48 +0200, Petr Viktorin wrote:
> 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.

We need to pass the key as an argument in python. The format of this
argument is irrelevant (base32/base64).

When *displaying* the key to the user as the result of the otp-add
operation, base32 should be used. Most soft-token products do input in
base32 since it is easier to type and read.

> >>>> 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.

Okay, that makes sense.

> >>>> 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?

Yes.