Re: RADIUS authentication

(2010/01/26 6:30), Magnus Hagander wrote:
> 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.

I'd like to take the issue into committer review stage.
Your patch is technically right, but I don't know whether it is on the
right direction in the community decision.

I think it is not a role in round-robin reviewer's stage.

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

OK, I'll have nothing to comment on this patch any more.

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