Project review: OTPOverRadius

[Dmitri Pal]

On 12/14/2012 03:38 PM, Nathaniel McCallum wrote:
> 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).

If user password is actually the PIN we need to split it. It should be
possible to send the code to radius server and validate the password
inside KDC from the initial implementation.
Is it not possible? I assume it should be possible to extrac tthe
password and pass to the KDB to do do the matching.

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

I have implemented RADIUS servers in the past. Ability to send packets
that you can read visually without any other tools is very important. If
you always have to use tools to decompose the radius packet and see what
actually was sent you significantly slow down development and testing. I
was also shocked about the fact that secrets can be empty but then I
have seen several cases when removing the secret and seeing what
actually goes over the wire led to quick identification of root cause of
the problem.

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

This is the assumption that needs to be reflected on the page.

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

We can always add it later if someone asks.

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

Match how? What is compared to what?

> from the user string, and tries the rest of them in order. Is
> there really a more sensible behavior?
>
> Nathaniel
>


-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/