Re: RADIUS authentication

2010/1/25 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2010/01/24 23:29), Magnus Hagander wrote:
>> There is one more option here - use OpenSSL if available. It has
>> functions for secure random number generations
>> (http://www.openssl.org/docs/crypto/RAND_bytes.html).  That seems easy
>> enough when OpenSSL is available.
>
> In just my opinion (so, committer may have different one), it is an
> option to utilize openSSL library when available. However, it should
> be moved to PostmasterRandom() and used to provide more randomness
> for srandom(). And, srandom() in the head of BackendRun() should be
> replaced by PostmasterRandom().

That is a feature separate from this.

And note that PostmasterRandom() and friends still deal with
pseudo-random numbers AFAIK. Not crytographically strong ones. Which
migh tactually be something we'd want to do in other places (like
generating salts), but again that's a completely different scope from
this.

> I also want any opinions from others.

Agreed, me too. I suggest a separate thread discussing random
generations in general for that.

>> The question then becomes what do we do if we don't have OpenSSL
>> available? Do we document that it's not secure, or refuse to run it?
>> I'd vote for document it.. If you don't have SSL enabled, then you
>> clearly don't use SSL for the libpq connection, which means the
>> password goes in cleartext in that stream...
>
> The seed of random is a different issue from safeness of password on
> the stream between client and server. For example, if admin set up
> IPsec/ESP between them, OpenSSL is not must-requirement.

Exactly, which is why I suggest a note in the docs only.

> Even if OpenSSL is not available, as long as both of PostgreSQL and
> RADIUS server are set up in trusted network, we can consider it is
> secure. So, all we can do is to introduce the risk, and the decisions
> are depending on end-users.

Agreed.

>>>>> * 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.
>>
>> Yeha, that is probably right. I copied that off a reference
>> implementation of the struct. Will change accordingly.
>>
>> FWIW, I tested it on Win64 and it didn't have any issues there at least.
>
> Just to be safe, could you inject an Assert() here?
> If a minor compiler aligned it unintentionally, it will be a bug not easy
> to find out.
>
>  /* check whether the compiler aligned it unintentionally, or not */
>  Assert(offsetof(radius_packet, vector) == 4);

Yeah, good point. I'll add that.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/