Project review: OTPOverRadius

[Nathaniel McCallum]

On Fri, 2012-12-14 at 14:47 -0500, Dmitri Pal wrote:
> On 12/14/2012 11:21 AM, ghudson at MIT.EDU wrote:
> > Here is a writeup of a project to implement KDC-side OTP support by
> > delegating to a RADIUS server:
> >
> > http://k5wiki.kerberos.org/wiki/Projects/OTPOverRADIUS
> >
> > Comments are appreciated.
> > _______________________________________________
> > krbdev mailing list             krbdev at mit.edu
> > https://mailman.mit.edu/mailman/listinfo/krbdev
> >
> >
> Couple comments:
> 
> 1) This seems confusing
> [otp]
>  <name> = {
>   vendor = <string>
>   length = <integer>
>   format = <enum: decimal|hexadecimal|alphanumeric|binary|base64>
>   algorithm  = <string>
>   server = <string>
>   secret = <string>
>   strip_realm = <boolean>
>   attributes = <name_or_number>:<value>
>  }
>
> Are a part of the token info. I suspect you are including it here to be able to use in the challenge to the client kerberos client. Does not make sense much to me frankly because this information would not be known from the external vendor. One would have to sync it. It just would not fly asking people to sync it. I think you mentioned that you want it for testing purposes but IMO even for testing purposes it does not make much sense. Why test something that no one would use?

You are mostly correct, but we need to support the OTP protocol for
interoperability. They will only be added by the admin when they are
needed for this purpose, and I think we should support them.

> The only thing that might be needed is in fact the "length".
> We also need to have a flag that would indicate whether the whole reply from the user is what need to go to the radius server or just last "length" symbols.
> May be length can be used for that? If length is 0 the whole user input goes to radius server, if it is non zero only last "length" symbols. The rest is assumed to be the password and needs to be checked against user password hash.

We probably need a split_value (boolean) option for this. If true length
would be used to find the offset. However, can we even get the user
password hash in a KDC preauth plugin? If yes, I'm all for adding this
option. If no, then maybe we can do this for a subsequent revision
(before 1.12).

> Secret should be optional in all cases because it is useful for debugging however every authentication via unprotected non RoUS channel should produce log messages.
> Strip_realm should be yes by default. It is more likely that people have flattened the namespace in some way. 

1. There is no such thing as "no secret" only a "well-known, pre-defined
secret"
2. A well-known secret adds nothing to your debug capabilities
3. We get significant benefit in making sure people aren't sending
effectively clear-text passwords over the network by mistake

> 2) IMO the whole RADIUS server config should be a separate configuration entity.
> Also there should be a list of the radius servers rather than just one in general case. They might share the same secret. 
> 
> [otp]
> 
>   token1 = { length = 8, radius = radius 1 }
> 
>   token2 = { length = 6, radius = radius 1 }
> 
>   token3 = { length = 6, radius = radius 2 }
> 
>   radius 1 = { server = [a:port1 ,b:port2], secret = 1234}
> 
>   radius 2 = { server = [x:portN ], secret = 5678}
> 
> Alternatively it can be (which is a bit better if we can use json here too):
> 
>   radius 1 = { server: [ { name: a, port: port1, secret: 1234},
> 
>                          { name: b, secret: 5678} ],
> 
>                strip_realm: false }
> 
> I do not know how flexible the krb5.conf syntax is and can it support arrays or not.

JSON is not permitted. We decided in a phone call that RADIUS redundancy
is typically done with DNS round-robin. In this case, configuration is
greatly simplified by only listing one server/secret.

If we use DNS round-robin, then having a separate radius config adds
complexity just to save a little typing in a write-once config. I'm not
inclined to complicate the code for that reason.


> 2) It is unclear why ID is really needed.
> 
> ||
> 
> [{
>    "type": <string>,
>    "id": <string>,
>    "username": <string>
>  }, ...]
> 
> The commens says "The *id* field identifies the unique id of the token and is sent in the PA-OTP-CHALLENGE." But it is really never used.

This exists for interoperability. Some clients may require this and the
admin should be able to just set a field.

> I suspect that you expect the prompt to tell the user to a specific token but I am not sure that in practice it would ever be used.
> Even in the migration case when company tries to move from one OTP vendor to another it would be very cumbersome to populate this field in the KDC. 

Less cumbersome than having to create a patch, run their own build and
still have to populate the field in the KDC.

> If we want to support migration in future we probably would have to allow more then one type per user and would try several token configurations for the user until one RADIUS server returns OK.
> I think you alluding to it in the following sentence : 
> "All matches will then be used as configuration for RADIUS validation, in the order they were specified in the *otp* user string, stopping after the first Access-Accept response is received."
> But it really requires a bit more clarity and explanation.
> At least JSON syntax seems to allow more then one type per user but it is not clear what would happen if multiple types are specified. 

How is that statement not clear? It eliminates the tokens that don't
match from the user string, and tries the rest of them in order. Is
there really a more sensible behavior?

Nathaniel