Hi Chris, Thank you for your reply to my question. Now, I did checkout freeradius-client from CVS and the function that I asked for exist. However, I did run into a few problems / issues. I am trying to cross-compile this for an ARM Intel ixp425 platform. 1) I needed to export ac_cv_func_uname=no into the environment before executing ./configure. Configure just didn't like the fact that I was cross-compiling with a different toolchain and barfed when it figured it couldn't run the test program. This should probably get fixed for 1.1.6. 2) There are some added macro "DEBUG" added to the freeradius-client.h. This conflicts with a different define of DEBUG defined somewhere else. I'd say either remove it, or rename it. It's for debugging anyway. 3) Cross-compiling 1.1.5, I had a problem with sending authentication requests where the password got corrupted due to some alignment errors in rc_pack_list() and rc_md5_calc(). It appears that the problem is gone in CVS, however; the code looks pretty much the same to me. What I did to circumvent the problem was to declare a char md5[MD5_DIGEST_LENGTH] and use that for MD5Final(), then do a memcpy(output, md5, MD5_DIGEST_LENGTH). This works well, when the output variable doesn't align with the boundary of my processor, in gdb it showed up as 0x...0a and it stomped on 0x...08 (the two first bytes got corrupted and my radiusd didn't understand squat). 4) I looked at the function signature for rc_add_config(...), and it does not seem to require "line", and "filename". It's more an artifact of your static functions requiring some input in case they fail. In my opinion, since the code is shared between rc_read_config(), the static functions should not need to know anything about filename and position, that's a responsibility of the caller function. Return codes can be used to solve this better refactoring the functions to better fit a users (freeradius-client api) need. The filename and line arguments simply doesn't have any value when you embed the radius client. Please consider the following as comments: 5) If you really want to 'embed' freeradius-client, it would be nice if I could specify something similar to "authserv <ip>:<port>:<secret>" and not needing to write a "server" file which declares the servers w/secrets. 6) I don't necessarily understand the radius-protocol in detail, but I would think it would be nice to be able to specify the dictionary attributes/values as you kind of do with rc_add_config(..). This again relates to the fact of reducing external dependencies when embedding freeradius-client. 7) This comment is related to logs, and I haven't studied the use of rc_log too closely. Just in general, this is a library and not an application. It would be nice to possibly use logs to trace calls within the library, but for error reporting it seems like it would be more useful to return an error code, and then call a rc_geterror_str(errno) or something. I am speaking of this as an integrator of OSS and I often like the ability to control the output of an application and not to necessarily "pollute" the system's event logs. BTW, I think freeradius is a great project, keep up the good work :o) I've attached a patch as a set of differences that fixes #2 #4. ---------------------------------------------------------- diff -ruNBb ../../vendor-drop/src/include/freeradius-client.h ../src/include/freeradius-client.h --- ../../vendor-drop/src/include/freeradius-client.h 2007-10-05 08:37:26.000000000 -0700 +++ ../src/include/freeradius-client.h 2007-11-07 08:15:23.000000000 -0800 @@ -17,12 +17,6 @@ #ifndef RADIUSCLIENT_NG_H #define RADIUSCLIENT_NG_H -#ifdef CP_DEBUG -#define DEBUG rc_log -#else -#define DEBUG -#endif - #include <sys/types.h> /* * Include for C99 uintX_t defines is stdint.h on most systems. Solaris uses @@ -447,7 +441,7 @@ SERVER *rc_conf_srv(rc_handle *, char *); int rc_find_server(rc_handle *, char *, uint32_t *, char *); void rc_config_free(rc_handle *); -int rc_add_config(rc_handle *, const char *, const char *, const char *, const int); +int rc_add_config(rc_handle *, const char *, const char *); rc_handle *rc_config_init(rc_handle *); int test_config(rc_handle *, char *); diff -ruNBb ../../vendor-drop/src/lib/config.c ../src/lib/config.c --- ../../vendor-drop/src/lib/config.c 2007-07-11 10:29:29.000000000 -0700 +++ ../src/lib/config.c 2007-11-07 08:16:14.000000000 -0800 @@ -52,7 +52,7 @@ * Returns: 0 on success, -1 on failure */ -static int set_option_str(const char *filename, int line, OPTION *option, const char *p) +static int set_option_str(OPTION *option, const char *p) { if (p) { option->val = (void *) strdup(p); @@ -67,12 +67,11 @@ return 0; } -static int set_option_int(const char *filename, int line, OPTION *option, const char *p) +static int set_option_int(OPTION *option, const char *p) { int *iptr; if (p == NULL) { - rc_log(LOG_ERR, "%s: line %d: bogus option value", filename, line); return -1; } @@ -87,7 +86,7 @@ return 0; } -static int set_option_srv(const char *filename, int line, OPTION *option, const char *p) +static int set_option_srv(OPTION *option, const char *p) { SERVER *serv; char *p_pointer; @@ -100,13 +99,11 @@ p_dupe = strdup(p); if (p_dupe == NULL) { - rc_log(LOG_ERR, "%s: line %d: Invalid option or memory failure", filename, line); return -1; } serv = (SERVER *) option->val; if (serv == NULL) { - DEBUG(LOG_ERR, "option->val / server is NULL, allocating memory"); serv = malloc(sizeof(*serv)); if (serv == NULL) { rc_log(LOG_CRIT, "read_config: out of memory"); @@ -152,12 +149,11 @@ else serv->port[serv->max] = ntohs ((unsigned int) svp->s_port); else { - rc_log(LOG_ERR, "%s: line %d: no default port for %s", filename, line, option->name); if (option->val == NULL) { free(p_dupe); free(serv); } - return -1; + return -2; } } @@ -180,7 +176,7 @@ return 0; } -static int set_option_auo(const char *filename, int line, OPTION *option, const char *p) +static int set_option_auo(OPTION *option, const char *p) { int *iptr; char *p_dupe = NULL; @@ -190,7 +186,6 @@ p_dupe = strdup(p); if (p_dupe == NULL) { - rc_log(LOG_WARNING, "%s: line %d: bogus option value", filename, line); return -1; } @@ -200,18 +195,15 @@ } *iptr = 0; - /*if(strstr(p_dupe,", \t") != NULL) {*/ p_pointer = strtok_r(p_dupe, ", \t", &p_save); - /*}*/ if (!strncmp(p_pointer, "local", 5)) *iptr = AUTH_LOCAL_FST; else if (!strncmp(p_pointer, "radius", 6)) *iptr = AUTH_RADIUS_FST; else { - rc_log(LOG_ERR,"%s: auth_order: unknown keyword: %s", filename, p); free(p_dupe); - return -1; + return -2; } p_pointer = strtok_r(NULL, ", \t", &p_save); @@ -222,9 +214,8 @@ else if ((*iptr & AUTH_LOCAL_FST) && !strcmp(p_pointer, "radius")) *iptr = (*iptr) | AUTH_RADIUS_SND; else { - rc_log(LOG_ERR,"%s: auth_order: unknown or unexpected keyword: %s", filename, p); free(p_dupe); - return -1; + return -2; } } @@ -242,7 +233,7 @@ * Returns: 0 on success, -1 on failure */ -int rc_add_config(rc_handle *rh, const char *option_name, const char *option_val, const char *source, const int line) +int rc_add_config(rc_handle *rh, const char *option_name, const char *option_val) { OPTION *option; @@ -260,22 +251,22 @@ switch (option->type) { case OT_STR: - if (set_option_str(source, line, option, option_val) < 0) { + if (set_option_str(option, option_val) < 0) { return -1; } break; case OT_INT: - if (set_option_int(source, line, option, option_val) < 0) { + if (set_option_int(option, option_val) < 0) { return -1; } break; case OT_SRV: - if (set_option_srv(source, line, option, option_val) < 0) { + if (set_option_srv(option, option_val) < 0) { return -1; } break; case OT_AUO: - if (set_option_auo(source, line, option, option_val) < 0) { + if (set_option_auo(option, option_val) < 0) { return -1; } break; @@ -353,6 +344,7 @@ char buffer[512], *p; OPTION *option; int line; + int rcode; size_t pos; rc_handle *rh; @@ -420,28 +412,46 @@ switch (option->type) { case OT_STR: - if (set_option_str(filename, line, option, p) < 0) { + rcode = set_option_str(option, p); + if (rcode < 0) { + rc_log(LOG_ERR, "%s: line %d: unable to set option value", filename, line ); fclose(configfd); rc_destroy(rh); return NULL; } break; case OT_INT: - if (set_option_int(filename, line, option, p) < 0) { + rcode = set_option_int(option, p); + if (rcode < 0) { + rc_log(LOG_ERR, "%s: line %d: unable to set option value", filename, line); fclose(configfd); rc_destroy(rh); return NULL; } break; case OT_SRV: - if (set_option_srv(filename, line, option, p) < 0) { + rcode = set_option_srv(option, p); + if (rcode == -1) + rc_log(LOG_ERR, "%s: line %d: unable to set option value", filename, line); + + if (rcode == -2) + rc_log(LOG_ERR, "%s: line %d: no default port for %s", filename, line, option->name); + + if (rcode < 0) { fclose(configfd); rc_destroy(rh); return NULL; } break; case OT_AUO: - if (set_option_auo(filename, line, option, p) < 0) { + rcode = set_option_auo(option, p); + if (rcode == -1) + rc_log(LOG_ERR, "%s: line %d: unable to set option value", filename, line); + + if (rcode == -2) + rc_log(LOG_ERR, "%s: line %d: found unknown keyword/value: %s", filename, line, p); + + if (rcode < 0) { fclose(configfd); rc_destroy(rh); return NULL; Binary files ../../vendor-drop/src/lib/.config.c.swp and ../src/lib/.config.c.swp differ diff -ruNBb ../../vendor-drop/src/lib/sendserver.c ../src/lib/sendserver.c --- ../../vendor-drop/src/lib/sendserver.c 2007-08-07 07:26:29.000000000 -0700 +++ ../src/lib/sendserver.c 2007-11-07 07:35:44.000000000 -0800 @@ -226,8 +226,6 @@ /*}*/ } - DEBUG(LOG_ERR, "DEBUG: rc_send_server: creating socket to: %s", server_name); - sockfd = socket (AF_INET, SOCK_DGRAM, 0); if (sockfd < 0) { @@ -297,10 +295,6 @@ auth->length = htons ((unsigned short) total_length); } - DEBUG(LOG_ERR, "DEBUG: local %s : 0, remote %s : %u\n", - inet_ntoa(sinlocal.sin_addr), - inet_ntoa(sinremote.sin_addr), data->svc_port); - for (;;) { sendto (sockfd, (char *) auth, (unsigned int) total_length, (int) 0, Binary files ../../vendor-drop/src/lib/.sendserver.c.swp and ../src/lib/.sendserver.c.swp differ diff -ruNBb ../../vendor-drop/src/src/radembedded.c ../src/src/radembedded.c --- ../../vendor-drop/src/src/radembedded.c 2007-10-08 08:48:06.000000000 -0700 +++ ../src/src/radembedded.c 2007-11-07 08:18:46.000000000 -0800 @@ -52,42 +52,42 @@ /* set auth_order to be radius only */ - if (rc_add_config(rh, "auth_order", "radius", "config", 0) != 0) + if (rc_add_config(rh, "auth_order", "radius") != 0) { printf("ERROR: Unable to set auth_order.\n"); rc_destroy(rh); exit(1); } - if (rc_add_config(rh, "login_tries", "4", "config", 0) != 0) + if (rc_add_config(rh, "login_tries", "4") != 0) { printf("ERROR: Unable to set login_tries.\n"); rc_destroy(rh); exit(1); } - if (rc_add_config(rh, "dictionary", "/usr/local/radius/dictionary", "config", 0) != 0) + if (rc_add_config(rh, "dictionary", "/usr/local/radius/dictionary") != 0) { printf("ERROR: Unable to set dictionary.\n"); rc_destroy(rh); exit(1); } - if (rc_add_config(rh, "seqfile", "/var/run/radius.seq", "config", 0) != 0) + if (rc_add_config(rh, "seqfile", "/var/run/radius.seq") != 0) { printf("ERROR: Unable to set seq file.\n"); rc_destroy(rh); exit(1); } - if (rc_add_config(rh, "radius_retries", "3", "config", 0) != 0) + if (rc_add_config(rh, "radius_retries", "3") != 0) { printf("ERROR: Unable to set radius_retries.\n"); rc_destroy(rh); exit(1); } - if (rc_add_config(rh, "radius_timeout", "5", "config", 0) != 0) + if (rc_add_config(rh, "radius_timeout", "5") != 0) { printf("ERROR: Unable to set radius_timeout.\n"); rc_destroy(rh); @@ -100,14 +100,14 @@ * be used. */ - if (rc_add_config(rh, "authserver", "localhost::testing123", "config", 0) != 0) + if (rc_add_config(rh, "authserver", "localhost::testing123") != 0) { printf("ERROR: Unable to set authserver.\n"); rc_destroy(rh); exit(1); } - if (rc_add_config(rh, "acctserver", "localhost:1813:testing123", "config", 0) != 0) + if (rc_add_config(rh, "acctserver", "localhost:1813:testing123") != 0) { printf("ERROR: Unable to set acctserver.\n"); rc_destroy(rh);
On Wed 07 Nov 2007, Eivind Naess wrote:
Hi Chris,
Thank you for your reply to my question. Now, I did checkout freeradius-client from CVS and the function that I asked for exist. However, I did run into a few problems / issues. I am trying to cross-compile this for an ARM Intel ixp425 platform.
1) I needed to export ac_cv_func_uname=no into the environment before executing ./configure. Configure just didn't like the fact that I was cross-compiling with a different toolchain and barfed when it figured it couldn't run the test program. This should probably get fixed for 1.1.6.
Yep. We need remove the use of AC_CHECK_FILE from configure.ac as it breaks cross compiling... -- Peter Nixon http://peternixon.net/
On Nov 7, 2007, at 10:59 AM, Eivind Naess wrote:
Hi Chris,
Thank you for your reply to my question. Now, I did checkout freeradius-client from CVS and the function that I asked for exist. However, I did run into a few problems / issues. I am trying to cross-compile this for an ARM Intel ixp425 platform.
1) I needed to export ac_cv_func_uname=no into the environment before executing ./configure. Configure just didn't like the fact that I was cross-compiling with a different toolchain and barfed when it figured it couldn't run the test program. This should probably get fixed for 1.1.6.
Agreed, cross-compiling isn't something that I have tested, so that will likely need some tweaks.
2) There are some added macro "DEBUG" added to the freeradius-client.h. This conflicts with a different define of DEBUG defined somewhere else. I'd say either remove it, or rename it. It's for debugging anyway.
Yes, that needs to be cleaned up. That was added to enable some debugging of the new functions.
3) Cross-compiling 1.1.5, I had a problem with sending authentication requests where the password got corrupted due to some alignment errors in rc_pack_list() and rc_md5_calc(). It appears that the problem is gone in CVS, however; the code looks pretty much the same to me. What I did to circumvent the problem was to declare a char md5[MD5_DIGEST_LENGTH] and use that for MD5Final(), then do a memcpy(output, md5, MD5_DIGEST_LENGTH). This works well, when the output variable doesn't align with the boundary of my processor, in gdb it showed up as 0x...0a and it stomped on 0x...08 (the two first bytes got corrupted and my radiusd didn't understand squat).
This is probably due to moving to uint_X rather than uchar and the like.
4) I looked at the function signature for rc_add_config(...), and it does not seem to require "line", and "filename". It's more an artifact of your static functions requiring some input in case they fail. In my opinion, since the code is shared between rc_read_config(), the static functions should not need to know anything about filename and position, that's a responsibility of the caller function. Return codes can be used to solve this better refactoring the functions to better fit a users (freeradius-client api) need. The filename and line arguments simply doesn't have any value when you embed the radius client.
I've left those there, as I'm passing values from the config file of the application I'm embedding the radius client into. :) Also, it helps a bit with backwards compatability.
Please consider the following as comments: 5) If you really want to 'embed' freeradius-client, it would be nice if I could specify something similar to "authserv <ip>:<port>:<secret>" and not needing to write a "server" file which declares the servers w/secrets.
You can do that with rc_add_config. rc_add_config(rad_config, "acctserver", server, filename, lineno) Where server is a string with 'ip:port:secret'.
6) I don't necessarily understand the radius-protocol in detail, but I would think it would be nice to be able to specify the dictionary attributes/values as you kind of do with rc_add_config(..). This again relates to the fact of reducing external dependencies when embedding freeradius-client.
There's been discussion about that. For now, the dictionary is one of the few external files still required for the client lib.
7) This comment is related to logs, and I haven't studied the use of rc_log too closely. Just in general, this is a library and not an application. It would be nice to possibly use logs to trace calls within the library, but for error reporting it seems like it would be more useful to return an error code, and then call a rc_geterror_str(errno) or something. I am speaking of this as an integrator of OSS and I often like the ability to control the output of an application and not to necessarily "pollute" the system's event logs.
That's a very good idea. I think I will steal it and implement it. :)
BTW, I think freeradius is a great project, keep up the good work :o)
Thanks for the valuable testing and feedback! -Chris
Hi Chris! And thank you for your reply. Do you have any estimates on when 1.1.6 will be released? On Nov 7, 2007 2:56 PM, Chris Parker <cparker@starnetusa.net> wrote:
On Nov 7, 2007, at 10:59 AM, Eivind Naess wrote:
Hi Chris,
Thank you for your reply to my question. Now, I did checkout freeradius-client from CVS and the function that I asked for exist. However, I did run into a few problems / issues. I am trying to cross-compile this for an ARM Intel ixp425 platform.
1) I needed to export ac_cv_func_uname=no into the environment before executing ./configure. Configure just didn't like the fact that I was cross-compiling with a different toolchain and barfed when it figured it couldn't run the test program. This should probably get fixed for 1.1.6.
Agreed, cross-compiling isn't something that I have tested, so that will likely need some tweaks.
Thanks!
2) There are some added macro "DEBUG" added to the freeradius-client.h. This conflicts with a different define of DEBUG defined somewhere else. I'd say either remove it, or rename it. It's for debugging anyway.
Yes, that needs to be cleaned up. That was added to enable some debugging of the new functions.
Thanks!
3) Cross-compiling 1.1.5, I had a problem with sending authentication requests where the password got corrupted due to some alignment errors in rc_pack_list() and rc_md5_calc(). It appears that the problem is gone in CVS, however; the code looks pretty much the same to me. What I did to circumvent the problem was to declare a char md5[MD5_DIGEST_LENGTH] and use that for MD5Final(), then do a memcpy(output, md5, MD5_DIGEST_LENGTH). This works well, when the output variable doesn't align with the boundary of my processor, in gdb it showed up as 0x...0a and it stomped on 0x...08 (the two first bytes got corrupted and my radiusd didn't understand squat).
This is probably due to moving to uint_X rather than uchar and the like.
It works in CVS, thanks!
4) I looked at the function signature for rc_add_config(...), and it does not seem to require "line", and "filename". It's more an artifact of your static functions requiring some input in case they fail. In my opinion, since the code is shared between rc_read_config(), the static functions should not need to know anything about filename and position, that's a responsibility of the caller function. Return codes can be used to solve this better refactoring the functions to better fit a users (freeradius-client api) need. The filename and line arguments simply doesn't have any value when you embed the radius client.
I've left those there, as I'm passing values from the config file of the application I'm embedding the radius client into. :) Also, it helps a bit with backwards compatability.
Very well, I'll define a macro in my code, that expands to rc_add_config(..., "config", 0); Thanks for clarifying this!
Please consider the following as comments: 5) If you really want to 'embed' freeradius-client, it would be nice if I could specify something similar to "authserv <ip>:<port>:<secret>" and not needing to write a "server" file which declares the servers w/secrets.
You can do that with rc_add_config.
rc_add_config(rad_config, "acctserver", server, filename, lineno)
Where server is a string with 'ip:port:secret'.
Thanks!, I just noticed that the functionality is in CVS and not in 1.1.5 when I sent the email, sorry!
6) I don't necessarily understand the radius-protocol in detail, but I would think it would be nice to be able to specify the dictionary attributes/values as you kind of do with rc_add_config(..). This again relates to the fact of reducing external dependencies when embedding freeradius-client.
There's been discussion about that. For now, the dictionary is one of the few external files still required for the client lib.
Oh well, I don't mean to start any religious debate on this, and frankly I don't understand radius to the extent that you probably do. But! I need to add some of the MS-CHAP attributes to the dictionary, and I think it would be nice if I had the chance to do so with a function and then use the dictionary as provided as a template. Also, much of what I intend to do is to use simple username and password, and ms-chap related Radius attributes. Do I really need to keep the others around (except those which is specified by an RFC?) The 'other' vendor specifics seems like an overkill to me, but I might be wrong. Just a few comments on the dicitionary file and format: 1) the dictionary seems to be a bit confusing to me, maybe add some details regarding how one should add vendor specific attributes which is different from freeradius server. VENDOR Microsoft 311 ATTRIBUTE MS-CHAP-Response 1 string vendor=Microsoft ATTRIBUTE MS-CHAP-Error 2 string vendor=Microsoft 2) the types have string, but not octets as specified by freeradius. Thus, if octets could be specified then you could perform a rc_log_avpair() printing octets as hex, and strings as strings. Also, wouldn't you say that a dictionary compatibility with freeradius would be a beneficial? Anyways, I was able to configure and run the client using freeradius as the server. And, it passed both my tests: user/pass and user + mschap-challenge, mschapv2-response.
7) This comment is related to logs, and I haven't studied the use of rc_log too closely. Just in general, this is a library and not an application. It would be nice to possibly use logs to trace calls within the library, but for error reporting it seems like it would be more useful to return an error code, and then call a rc_geterror_str(errno) or something. I am speaking of this as an integrator of OSS and I often like the ability to control the output of an application and not to necessarily "pollute" the system's event logs.
That's a very good idea. I think I will steal it and implement it. :)
It's not stealing! I just tossed you a bone ;p)
BTW, I think freeradius is a great project, keep up the good work :o)
Thanks for the valuable testing and feedback!
Glad I could be of any help, maybe I'll join your development efforts...
-Chris
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
participants (3)
-
Chris Parker -
Eivind Naess -
Peter Nixon