Thread: RADIUS authentication
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/
Attachment
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.
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
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
(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 itallows 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 thepurpose? 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 youdocument 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 randomstring? */ + 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 justafter 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 formatof RADIUS network packet as is. It may be preferable to give compiler a hint not to align this structure. In GCC, wecan 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>
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
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
(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>
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
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/
Attachment
(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>
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
(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>