Re: RADIUS authentication

(2010/01/20 0:19), Magnus Hagander wrote:
> 2010/1/18 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> (2010/01/10 22:25), Magnus Hagander wrote:
>>> The attached patch implements RADIUS authentication (RFC2865-compatible).
>>>
>>> The main usecase for me in this is the ability to use (token based)
>>> one-time-password systems easily with PostgreSQL. These systems almost
>>> always support RADIUS, and the implementation is fairly simple. RADIUS
>>> can of course be used in many other scenarios as well (for example, it
>>> can be used to implement "only this group"-access with at least Active
>>> Directory, something our current LDAP doesn't support. We might
>>> eventually want to support that in our LDAP, but it's not there now)
>>
>> I checked this patch.
>
> Thanks!
>
>
>> Here is a few comments from the initial reviewing.
>>
>> * Is the feature to be configurable via ./configure scripts?
>> Currently, we have --with-pam or --with-ldap option, and it allows
>> users to turn on/off the feature.
>> Of course, it has dependency on libraries.
>
> I think not, because it doesn't rely on external libraries we might as
> well always enable it. As long as you don't configure it in
> pg_hba.conf, it has zero cost to the installation. Adding a configure
> parameter would just make things complicated. For example, we don't
> have a configure switch to enable ident or md5.
>
>
>> * A corresponding comment. This patch implements RADIUS protocol
>> by itself. Is there any commonly used libraries for the purpose?
>> It allows us to separate a burden to manage a certain network
>> protocol within PostgreSQL.
>
> I looked briefly at it. The ones I found would require almost as much
> code as doing the protocol itself, and had some compatibility issues
> (mainly wrt windows)

OK, I agreed.

>> * IIUC, inet_addr() takes only IPv4 address. It is used to translate
>> "radiusserver" parameter to netaddr format.
>> Could you document this parameter takes only IPv4 format.
>
> Will do.
>
>
>> * I think this comment is right.
>> + for (i = 0; i< RADIUS_VECTOR_LENGTH; i++)
>> + /* XXX: Generate a more secure random string? */
>> + packet->vector[i] = random() % 255;
>>
>> The random seed is initialized at BackendRun() with MyProcPid and
>> the time of backend process launched.
>> Then, PostgresMain() -> InitPostgres() -> PerformAuthentication()
>> will be called, and this random() shall be the first call just after
>> initialization of the srandom().
>>
>> Do you have any good idea?
>> Or, do you think it should be fixed with high priority?
>
> It does need a fairly good random number generator there to be secure,
> so it should probably be improved. OTOH, the whole thing can be more
> considered obfuscation rather than encryption, and those who really
> care about higher security will use ipsec or trusted networks.
>
> Maybe switching to erand48() would make this better, and good enough?

As Tom pointed out, it is fundamentally same.
The matter is this random() invocation is the first time after
initialization of random seed by srandom(). It means an external observer
can estimate the random value uniquely using pid and startup time.

In other representation, the "random" value is the result of function
which takes arguments of pid and startup time, without random factor.

for (i = 0; i< RADIUS_VECTOR_LENGTH; i++)
packet->vector[i] = f(getpid(), port->SessionStartTime, i);

One idea is to modify the logic to set up random seed in BackendRun().
In most of UNIX-like operating system, we can use /dev/random as a random
seed which is well randomized.

http://en.wikipedia.org/wiki//dev/random

It seems to me PostmasterRandom() is a right place to set random seed,
and we can inject a block something like #ifdef HAVE_DEV_RANDOM ...

Instead of such kind of efforts, we can also document that PostgreSQL and
RADIUS server should have communication using enough secure connection
explicitly. IMO, it will cover most of use cases.

>> * It casts char array (such as radius_buffer) into radius_packet
>> structure. The radius_packet structure represents the format of
>> RADIUS network packet as is.
>> It may be preferable to give compiler a hint not to align this
>> structure.
>> In GCC, we can use "__attribute__((packed))" to suggest not to
>> align the member of structure. Is there any portable way for this?
>
> This I can't answer, I don't know this well enough. Somebody else?

What manner is applied to handle network protocol in other part?

The radius_packet is declared as follows:

+ typedef struct
+ {
+ unsigned char code; +0
+ unsigned char id; +1
+ unsigned short length; +2
+ unsigned char vector[RADIUS_VECTOR_LENGTH]; +4? +8?
+ } radius_packet;

It may be a bit nervous, except for possible alignment of the vector
on 64bit architecture.

And, one more. It seems to me uint8 and uint16 are more preferable than
unsigned char/short in this context.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>