[Otpasswd-talk] Re: Today's Hand Grenades

[Hannes Beinert]

On Sat, Jan 9, 2010 at 02:06, Tomasz bla Fortuna <address@hidden> wrote:
> Dnia Sat, 9 Jan 2010 00:06:58 -0600
> Hannes Beinert <address@hidden> napisał(a):
>
>> On Fri, Jan 8, 2010 at 18:22, Tomasz bla Fortuna <address@hidden> wrote:
>> > Dnia Fri, 8 Jan 2010 16:23:32 -0600 Hannes Beinert
>> > <address@hidden> napisał(a):
>> >
>> >> 1. I've been thinking about the otpasswd.conf file.  One of the
>> >> things that occurs to me is that it would be more user-friendly if
>> >> instead of numeric values, one could use string values such as
>> >> ALLOW, PERMIT, DISALLOW, NOALLOW, NOPERMIT, DENY, ENFORCE,
>> >> OPTIONAL or whatever.  I think this would make things more lucid,
>> >> and it would also allow someone to grep the config file to get a
>> >> setting, and immediately be able to understand the meaning of the
>> >> setting.  As it is right now, if I grep on SHOW_ALLOW, I almost
>> >> need to go back to the config file to see what that value setting
>> >> means.
>> >>
>> >> I realize there are other types of settings to which the above
>> >> keywords wouldn't be appropriate, but one could use a set of
>> >> keywords appropriate to each parameter key, such as RETRY =
>> >> (DISALLOW | NEXT | SAME).  Etc.
>> >
>> > Yeah, it would be good to clean it up a bit. I'd have to redesign
>> > it a bit, parse argument (words you are proposing + db values) in
>> > one place to some enum and then parse the variable itself. After
>> > config is designed nicely I think it won't be that hard to
>> > implement it.
>>
>> I was thinking of some ad hoc table-driven parser, where you have a
>> list of keys, "value name", and values.  For example,
>>
>> struct vlist {
>>         char    *vname;
>>         int     value;
>> };
>>
>> struct ptable {
>>         char    *key;
>>         struct  vlist *values;
>> };
>>
>> struct  vlist   vl_endis[] = {
>>                         "ENABLE",       0,
>>                         "DISABLE",      1,
>>                         NULL,           0,
>>                 };
>>
>> struct  vlist   vl_retry[] = {
>>                         "RETRY",        0,
>>                         "NEXT",         1,
>>                         "SAME",         2,
>>                         NULL,           0,
>>                 };
>>
>> struct  ptable  ptable[] = {
>>         "RETRY",        vl_retry,
>>         "RETRIES",      vl_retry,
>>         "ALLOW_USER",   vl_endis,
>>         NULL,           NULL,
>> };
>>
>> The keys/values are nonsense, but you get the idea.  You could then
>> just plug in the keys and values you need, and leave the parsing to a
>> little automaton.
>
> That will complicate parsing code a bit (currently it
> parses config line-by-line, which should be sufficient since there's no
> relations between keys). But it can also clean up parsing approach a bit
> too if done well.

Yes, you're right -- there really is no need I can see to introduce
any complicated relationships between keys.

> Adding this value-keys can be done while keeping config-parsing in
> one-part, I mean, without splitting to:
> 1) Parse config and store key-value pairs
> 2) Parse read-value pairs check arguments, check if keys are ok and
> copy arguments to another struct.

Similarly, I agree that keeping it simple has its virtues.  I don't
see why this can't be done in a single pass.

>> >> 2. Any chance of allowing whitespace on either side of the equal
>> >> sign in the config?  ;-)
>> >
>> > Sure it's possible. Just some right/left trimming after split, as
>> > right trimming I've already done I guess this won't hurt.
>> >
>> > Problem is only not to make any mistake while parsing it. It's
>> > almost the only part of program which is run without dropping
>> > permissions.
>> >
>> > This part for sure not for now. But I believe cleaning up config
>> > file before release is needed.
>>
>> Yeah, I can understand your concern.
>>
>> >> 3. Generally speaking, would it make sense for all config
>> >> parameters to have a standard prefix to indicate that they can, or
>> >> cannot be enforced in DB=USER?  I'm not sure about this idea -- it
>> >> just occurred to me that something to make the parameters more
>> >> self-documenting.  A prefix like "POLICY_RETRY" or
>> >> "POLICY_RETRIES" for policies that are always enforced.
>> >
>> > There's also question what is policy, and what is just a PAM module
>> > configuration (like in the example of RETRY/RETRIES). ;)
>>
>> True.  Although, I think almost anything which determines the behavior
>> of the system could be viewed to be policy.  But I do understand your
>> point.
>>
>> >> Then "SYSPOL" or "SPOLICY" for policies that can
>> >> only be enforced in DB=GLOBAL -- like
>> >> "SPOLICY_ALLOW_KEY_GENERATION". An alternative approach which
>> >> would complicate the structure of the config file (and therefore
>> >> is probably less than optimal) would be to use "sections".  Ie,
>> >> something like "[GLOBAL DB]" for parameters that make sense only
>> >> in GLOBAL DB mode, and perhaps [POLICY] for those that are always
>> >> enforced.  And [GENERAL] for settings that aren't policy related.
>> >
>> > This is not technically needed, ...
>>
>> No, that's quite true.  My question was more directed towards making
>> things self-documenting.  IOW, if you see a parameter which is names
>> "SYS_BLAH_BLAH", you could immediately deduce that it depends upon
>> enforcement, which is only possible if operating in a global/system
>> mode.
>>
>> > ... and can be told the user with
>> > commentaries. This parsing really should be kept simple. ;) ...
>>
>> I'm just thinking that it might be nice to have a situation where you
>> don't have to read the comments to understand the scope/applicability
>> of a parameter.  I agree with you, however, that it's not essential.
>> It's just a thought.
>
> True, but it would be better to store this information inside key names.
> If we will move things to some distant [section] entries then user might
> as well look for # ***** GLOBAL SECTION ****** comments, which will be
> also visible.
>
> I agree with self-documenting names, I just don't thing adding
> [sectons] would help it much.

True.  I admit, it was a silly idea.  :-)

>> As for parsing -- the prefix wouldn't make parsing anymore difficult.
>> The "sections" obviously would.  And basically I do agree that it's
>> probably best that the config be kept simple.
>>
>> > ... Still
>> > prefixing variables is perfectly OK. They might be long though -
>> > user doesn't need to neither remember them nor to type them.
>>
>> As I say, it's just a thought.  I'm not really terribly keen on
>> 25-character parameter names, either.  :-)
>>
>> > I'm not very happy with POLICY/SPOLICY (long and doesn't really mean
>> > much) but something in lines of this pair.
>> >
>> > Maybe ALLOW / ENFORCE? Allow being fully enforceable only in global
>> > mode, while enforce being enforced mostly by PAM module? Not really
>> > happy with this either. Think. ;)
>>
>> Yeah, it's not the most obvious, either.  :-\
>>
>> GEN/PAM/SYS?  General configuration, PAM level enforcement, and
>> System/Global mode enforcement?
>
> That seems nice! I guess 'ALLOW' word from keys can be dropped and
> changed to the word-value.

Hmm.  That's true.  That would shorten up the key names, and the
assignments would be pretty easy to read.

>> > In DB=user, there are:
>> > 1) Things user CAN overcome by editing file (e.g. skipping)
>> > 2) Things utility can deny him, but he will overcome utility by
>> > changing state file. PAM will then deny him any authentication
>> > attempts so this is enforced somehow.
>> >
>> >
>> >> 4. You know, I've been struggling with a way to talk about
>> >> "DB=USER" and "DB=GLOBAL" -- since those "terms" are conceptually
>> >> accurate, but they don't roll off the tongue very easily.  There's
>> >> always "Global DB mode" and "User DB mode", but that is only
>> >> slightly better.  One confusion is that "DB=GLOBAL" (or "Global DB
>> >> mode") doesn't obviously *seem* to apply to "DB=LDAP" or
>> >> "DB=MYSQL".  One thought I had was to call these two operational
>> >> modes "User mode" and "System mode".  It really would be nice to
>> >> have something that's a little easier to talk about in the
>> >> manuals...  I don't know, though.  Thoughts?
>> >
>> > Renaming global to something is fine. I wasn't really happy about
>> > this option either. I'm even not very sure about 'DB'. Maybe 'MODE'
>> > is better?
>> >
>> > Still we talk about two modes of operation; one grouping global,
>> > ldap and mysql together.
>>
>> I think DB=global/mysql/ldap is fine, possibly substituting 'global'
>> for 'system'.  However, I think it's fine.
>>
>> The question was more about what to actually call it in the
>> documentation?  I'm beginning to prefer "system mode" and "user mode",
>> but I'm not sure.  The "system mode" would basically mean all types of
>> operation which use a centralized database.
>
> That wording seems ok. But since you'd use 'system mode' for
> all centralised approaches (mysql/ldap/global) I'd then keep 'global'
> instead of 'system'. Changing might confuse someone as to when we work
> in system-mode. Just thinking.

Absolutely, I think part of what I don't like about "global mode" is
that the word "global" is used in the actual DB=GLOBAL setting, which
leads to confusion.  For the purposes of the documentation, it would
be nice to have some terms which reflect the central/non-central
database difference.  Do you have a preference?  Do you prefer "system
mode", and keep DB=GLOBAL, or do you prefer "global mode" while using
DB=SYSTEM?  Or do you prefer something else?

>> >> 5. You knew I was going to ask about this, right?  What do you
>> >> have in mind with the ALLOW_BACKWARD_SKIPPING?  That seems...
>> >> counter-intuitive.  :-)
>> >
>> > Right... --skip currently works more like SETTING counter option,
>> > as it gets the same arguments as -t and -l. So -s 3 doesn't skip by
>> > 3 but sets counter to third passcard.
>> >
>> > While I think it's not particularly bad it'd be nice to be able to
>> > say something like -s '+3' or anything...
>>
>> Okay...  but I still don't understand why you would *ever* allow
>> skipping backwards?  This ability is completely contradictory to the
>> notion of OTP, isn't it?  ;-)
>>
>> I think that if you skip to a passcard which is < the current
>> passcard, it should be an error.  Don't you think?
>
> Skipping backwards during normal-authentication work of the system for
> sure should end up with an error.
>
> I can see someone who prints some passcards and then messes -t option
> with -s option making the system skip all what he has printed. This can
> be root who tried to skip someones codes but messed up user, or forgot
> --user at all. If he hasn't authenticated with this passcodes there's
> nothing fundamentally wrong with skipping back to the place he ended
> with. Still this can't be allowed for normal (non-secure-aware) users
> that's why policy is 0 by default.

Ah, that's the use case you have in mind.  Okay, I understand, now.

Well, I think that to do this "right", one would really have to have
two pieces of information in the user state, namely, the
LAST_USED_PASSCODE (LUP) and NEXT_PASSCODE (NP).  The LUP is *never*
adjusted backwards.  The NP can be skipped anywhere, as long as it is
*always* true that NP > LUP.

This would make sense to me.  By doing it this way, LUP would only be
updated if a passcode was used for an authentication.  As long as it
hasn't changed, NP could be modified to any value -- forward,
backward, whatever -- as long as NP is always greater than LUP.  The
moment an authentication takes place, LUP is set to NP, and backing up
is no longer possible.

My concern is really only with passcode reuse.  If there's no danger
of passcode reuse, then I'm not even sure that there needs to be a
policy to allow skipping.

Also, since the sysadmin can do *so* many things to screw up security,
it's not clear to me that the superuser shouldn't be exempted from
this, anyway.  IOW, perhaps he should be able to change NP such that
NP <= LUP.  But only with a warning.  And again, I think that LUP
should only be changed after an authentication.

Is this silly?

> If you think this is so strange usage it shouldn't really be an option
> in OTPasswd I guess I can remove it. I'm not very bound up with it.
>
>> >> 6. In the ChangeLog you have an item asking whether OTPasswd will
>> >> be PPPv3 compatible on a big-endian system.  I think that will
>> >> "come out in the wash" when we try to test OTPasswd on a PPC Mac.
>> >>  As it happens, I have one sitting here.  ;-)  Just FYI.
>> >
>> > Cool!
>> >
>> >> By the way, I realize you've got less free time right now to
>> >> devote to this project.  If you think any of these ideas are worth
>> >> anything, I'd be happy to do some of the coding, if that would
>> >> help.
>> >
>> > I'm pretty much busy before wednesday and a little bit more than
>> > usual after it. You can do design of config if you like - the way
>> > you see it. Keeping it mind that it's essential to be able to parse
>> > it easily without mistakes. ;-)
>>
>> Heh.  I understand.  :-)
>>
>> Well, I still have plenty of documentation to write, but I'll put it
>> on my list, unless you get to it first.  ;-)

Hannes.