Thread

1. RADIUS authentication

   Magnus Hagander <magnus@hagander.net> — 2010-01-10T13:25:06Z

   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)
   
   -- 
    Magnus Hagander
    Me: http://www.hagander.net/
    Work: http://www.redpill-linpro.com/
   

2. Re: RADIUS authentication

   Peter Eisentraut <peter_e@gmx.net> — 2010-01-10T17:55:28Z

   On sön, 2010-01-10 at 14:25 +0100, 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)
   
   Sounds interesting; I didn't know RADIUS was still in use.
   
   There is a copy-and-paste'o in the patch, where LDAP is mentioned
   instead of RADIUS.
   
   
   

3. Re: RADIUS authentication

   Magnus Hagander <magnus@hagander.net> — 2010-01-10T18:05:28Z

   On Sun, Jan 10, 2010 at 18:55, Peter Eisentraut <peter_e@gmx.net> wrote:
   > On sön, 2010-01-10 at 14:25 +0100, 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)
   >
   > Sounds interesting; I didn't know RADIUS was still in use.
   
   It's very much in use. It works well for that kind of scenario, and
   it's still very much in use by ISPs (for other things than database,s
   of course)
   
   
   > There is a copy-and-paste'o in the patch, where LDAP is mentioned
   > instead of RADIUS.
   
   Yeah, Stefan just pointed that out to me over IM. Thanks. There's also
   a smis-spelling of the word RADIUS :-)
   
   -- 
    Magnus Hagander
    Me: http://www.hagander.net/
    Work: http://www.redpill-linpro.com/
   
   

4. Re: RADIUS authentication

   Stephen Frost <sfrost@snowman.net> — 2010-01-11T03:10:56Z

   Magnus,
   
   * Magnus Hagander (magnus@hagander.net) wrote:
   > The attached patch implements RADIUS authentication (RFC2865-compatible).
   
   Great!  We have a few environments which use RADIUS auth, nice that PG
   might be able to use that auth method in the future.  
   
   I'm not a fan of having the shared secret stored in a 'regular' config
   file.  Could you support, or maybe just change it to, breaking that out
   into another file?  Perhaps something simimlar to how pam_radius_auth
   works, where you can also list multiple servers?  
   
   http://freeradius.org/pam_radius_auth/
   
   Would also allow using the same file for multiple RADIUS-based servers..
   
   I know pg_hba.conf can just be set to have minimal permissions (and is
   on Debian), but that's the kind of file that tends to end up in things
   like subversion repositories or puppet configs where they aren't
   treated as carefully since, generally, what's in them doesn't come
   across as super-sensetive.
   
   	Thanks,
   
   		Stephen
   

5. Re: RADIUS authentication

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-01-18T10:08:50Z

   (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.
   
   It can be applied on the latest CVS HEAD, and built without any matter.
   
   I tried to work it with freeradius, then it performs well in a few
   configurations. (Of course, it is far from comprehensive.)
   Because I'm not good at RADIUS protocol, I didn't check correctness
   of the protocol.
   
   Hmm, it introduces the format of the UDP packets.
   It seems to me this patch is implemented correctly.
     http://technet.microsoft.com/en-us/library/cc958030.aspx
   
   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.
   
   * 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.
   
   * 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.
   
   * 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 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?
   
   Thanks,
   -- 
   OSS Platform Development Division, NEC
   KaiGai Kohei <kaigai@ak.jp.nec.com>
   
   

6. Re: RADIUS authentication

   Magnus Hagander <magnus@hagander.net> — 2010-01-19T15:19:58Z

   2010/1/18 KaiGai Kohei <kaigai@ak.jp.nec.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)
   
   
   > * 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?
   
   
   > * 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?
   
   
   -- 
    Magnus Hagander
    Me: http://www.hagander.net/
    Work: http://www.redpill-linpro.com/
   
   

7. Re: RADIUS authentication

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-19T16:38:13Z

   Magnus Hagander <magnus@hagander.net> writes:
   > 2010/1/18 KaiGai Kohei <kaigai@ak.jp.nec.com>:
   >> 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().
   
   > Maybe switching to erand48() would make this better, and good enough?
   
   Wouldn't help in the least.  The problem is not the RNG itself but lack
   of an adequately unpredictable random seed, and anything you do here
   is unlikely to be more random than what we already arranged for.
   
   			regards, tom lane
   
   

8. Re: RADIUS authentication

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-01-20T01:16:53Z

   (2010/01/20 0:19), Magnus Hagander wrote:
   > 2010/1/18 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
   
   

9. Re: RADIUS authentication

   Magnus Hagander <magnus@hagander.net> — 2010-01-24T14:29:30Z

   2010/1/20 KaiGai Kohei <kaigai@ak.jp.nec.com>:
   > (2010/01/20 0:19), Magnus Hagander wrote:
   >>> * 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.
   
   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.
   
   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...
   
   
   >>> * 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.
   
   -- 
    Magnus Hagander
    Me: http://www.hagander.net/
    Work: http://www.redpill-linpro.com/
   
   

10. Re: RADIUS authentication

    Magnus Hagander <magnus@hagander.net> — 2010-01-24T15:56:06Z

    2010/1/24 Magnus Hagander <magnus@hagander.net>:
    > 2010/1/20 KaiGai Kohei <kaigai@ak.jp.nec.com>:
    >> 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.
    >
    > 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.
    >
    > 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...
    
    Updated patch attached.
    
    
    -- 
     Magnus Hagander
     Me: http://www.hagander.net/
     Work: http://www.redpill-linpro.com/
    

11. Re: RADIUS authentication

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-01-25T03:16:34Z

    (2010/01/24 23:29), Magnus Hagander wrote:
    > 2010/1/20 KaiGai Kohei<kaigai@ak.jp.nec.com>:
    >> (2010/01/20 0:19), Magnus Hagander wrote:
    >>>> * 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.
    > 
    > 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().
    
    I also want any opinions from others.
    
    > 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.
    
    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.
    
    >>>> * 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);
    
    Thanks,
    -- 
    OSS Platform Development Division, NEC
    KaiGai Kohei <kaigai@ak.jp.nec.com>
    
    

12. Re: RADIUS authentication

    Magnus Hagander <magnus@hagander.net> — 2010-01-25T21:30:51Z

    2010/1/25 KaiGai Kohei <kaigai@ak.jp.nec.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/
    
    

13. Re: RADIUS authentication

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-01-25T23:52:00Z

    (2010/01/26 6:30), Magnus Hagander wrote:
    > 2010/1/25 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>