Skip to Content.
Sympa Menu

freetds - Re: [freetds] Encrypted connection

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: Frediano Ziglio <freddy77 AT gmail.com>
  • To: FreeTDS Development Group <freetds AT lists.ibiblio.org>
  • Subject: Re: [freetds] Encrypted connection
  • Date: Tue, 22 Oct 2013 22:50:36 +0100

Even gmail seems to dislike text mail...

See https://gitorious.org/freetds/mars-freetds/commits/config_error

I think it would be great to integrate the tdserror stuff so patch
would be more verbose.

Frediano

2013/10/22 Marc Abramowitz <msabramo AT gmail.com>:
> That looks fairly similar but better than mine as you put the invalid_flag
> in the tds_login struct instead of a global.
>
> If you email me a file with a patch that's based off master, I can try it
> out if you'd like. I had trouble copying and pasting the patch because of
> line breaks (from Gmail?) and even after fixing those, the patch didn't
> seem to apply cleanly. I'd be glad to try it though.
>
>
> On Tue, Oct 22, 2013 at 1:13 PM, Frediano Ziglio <freddy77 AT gmail.com> wrote:
>
>> I wrote something like this
>>
>> Date: Tue, 22 Oct 2013 21:12:13 +0100
>> Subject: [PATCH] Simple and probably incomplete patch to check
>> configuration
>> values.
>>
>> Signed-off-by: Frediano Ziglio <freddy77 AT gmail.com>
>> ---
>> include/freetds/tds.h | 3 ++-
>> src/odbc/connectparams.c | 8 ++++----
>> src/tds/config.c | 22 +++++++++++++---------
>> src/tds/login.c | 3 +++
>> 4 files changed, 22 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/freetds/tds.h b/include/freetds/tds.h
>> index b27fcf6..f4a2621 100644
>> --- a/include/freetds/tds.h
>> +++ b/include/freetds/tds.h
>> @@ -578,6 +578,7 @@ typedef struct tds_login
>> unsigned int use_ntlmv2:1;
>> unsigned int use_lanman:1;
>> unsigned int mars:1;
>> + unsigned int invalid_configuration:1;
>> } TDSLOGIN;
>>
>> typedef struct tds_locale
>> @@ -1187,7 +1188,7 @@ const char *tds_addrinfo2str(struct tds_addrinfo
>> *addr, char *name, int namemax)
>>
>> TDSRET tds_set_interfaces_file_loc(const char *interfloc);
>> extern const char STD_DATETIME_FMT[];
>> -int tds_config_boolean(const char *value);
>> +int tds_config_boolean(const char *value, TDSLOGIN * login);
>>
>> TDSLOCALE *tds_get_locale(void);
>> TDSRET tds_alloc_row(TDSRESULTINFO * res_info);
>> diff --git a/src/odbc/connectparams.c b/src/odbc/connectparams.c
>> index b287311..0a910d3 100644
>> --- a/src/odbc/connectparams.c
>> +++ b/src/odbc/connectparams.c
>> @@ -226,12 +226,12 @@ odbc_get_dsn_info(TDS_ERRS *errs, const char
>> *DSN, TDSLOGIN * login)
>> if (myGetPrivateProfileString(DSN, odbc_param_ServerSPN, tmp) > 0)
>> tds_parse_conf_section(TDS_STR_SPN, tmp, login);
>>
>> - if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
>> tmp) > 0 && tds_config_boolean(tmp)) {
>> + if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
>> tmp) > 0 && tds_config_boolean(tmp, login)) {
>> tds_dstr_copy(&login->user_name, "");
>> tds_dstr_copy(&login->password, "");
>> }
>>
>> - if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
>> tmp) > 0 && tds_config_boolean(tmp)) {
>> + if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
>> tmp) > 0 && tds_config_boolean(tmp, login)) {
>> login->mars = 1;
>> }
>>
>> @@ -389,12 +389,12 @@ odbc_parse_connect_string(TDS_ERRS *errs, const
>> char *connect_string, const char
>> } else if (CHK_PARAM(ServerSPN)) {
>> tds_parse_conf_section(TDS_STR_SPN, tds_dstr_cstr(&value),
>> login);
>> } else if (CHK_PARAM(Trusted_Connection)) {
>> - trusted = tds_config_boolean(tds_dstr_cstr(&value));
>> + trusted = tds_config_boolean(tds_dstr_cstr(&value), login);
>> tdsdump_log(TDS_DBG_INFO1, "trusted %s -> %d\n",
>> tds_dstr_cstr(&value), trusted);
>> num_param = -1;
>> /* TODO odbc_param_Address field */
>> } else if (CHK_PARAM(MARS_Connection)) {
>> - if (tds_config_boolean(tds_dstr_cstr(&value)))
>> + if (tds_config_boolean(tds_dstr_cstr(&value), login))
>> login->mars = 1;
>> }
>>
>> diff --git a/src/tds/config.c b/src/tds/config.c
>> index c4d38e7..41386e5 100644
>> --- a/src/tds/config.c
>> +++ b/src/tds/config.c
>> @@ -399,6 +399,8 @@ tds_read_conf_sections(FILE * in, const char
>> *server, TDSLOGIN * login)
>> default_port = login->port;
>>
>> found = tds_read_conf_section(in, server, tds_parse_conf_section,
>> login);
>> + if (login->invalid_configuration)
>> + return 0;
>>
>> /*
>> * If both instance and port are specified and neither one came
>> from the default, it's an error
>> @@ -428,7 +430,7 @@ static const struct {
>> };
>>
>> int
>> -tds_config_boolean(const char *value)
>> +tds_config_boolean(const char *value, TDSLOGIN *login)
>> {
>> int p;
>>
>> @@ -436,7 +438,7 @@ tds_config_boolean(const char *value)
>> if (!strcasecmp(value, boolean_values[p].value))
>> return boolean_values[p].to_return;
>> }
>> - tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED boolean value: '%s'.
>> Treating as 'no'.\n", value);
>> + login->invalid_configuration = 1;
>> return 0;
>> }
>>
>> @@ -451,8 +453,10 @@ tds_config_encryption(const char * value, TDSLOGIN *
>> login)
>> lvl = TDS_ENCRYPTION_REQUEST;
>> else if (!strcasecmp(value, TDS_STR_ENCRYPTION_REQUIRE))
>> lvl = TDS_ENCRYPTION_REQUIRE;
>> - else
>> + else {
>> tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED option value
>> '%s'...ignoring.\n", value);
>> + login->invalid_configuration = 1;
>> + }
>>
>> login->encryption_level = lvl;
>> }
>> @@ -568,10 +572,10 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>> if (val >= 512 && val < 65536)
>> login->block_size = val;
>> } else if (!strcmp(option, TDS_STR_SWAPDT)) {
>> - login->broken_dates = tds_config_boolean(value);
>> + login->broken_dates = tds_config_boolean(value, login);
>> } else if (!strcmp(option, TDS_GSSAPI_DELEGATION)) {
>> /* gssapi flag addition */
>> - login->gssapi_use_delegation = tds_config_boolean(value);
>> + login->gssapi_use_delegation = tds_config_boolean(value, login);
>> } else if (!strcmp(option, TDS_STR_DUMPFILE)) {
>> tds_dstr_copy(&login->dump_file, value);
>> } else if (!strcmp(option, TDS_STR_DEBUGFLAGS)) {
>> @@ -604,7 +608,7 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>> if (atoi(value))
>> login->port = atoi(value);
>> } else if (!strcmp(option, TDS_STR_EMUL_LE)) {
>> - login->emul_little_endian = tds_config_boolean(value);
>> + login->emul_little_endian = tds_config_boolean(value, login);
>> } else if (!strcmp(option, TDS_STR_TEXTSZ)) {
>> if (atoi(value))
>> login->text_size = atoi(value);
>> @@ -617,7 +621,7 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>> } else if (!strcmp(option, TDS_STR_LANGUAGE)) {
>> tds_dstr_copy(&login->language, value);
>> } else if (!strcmp(option, TDS_STR_APPENDMODE)) {
>> - tds_g_append_mode = tds_config_boolean(value);
>> + tds_g_append_mode = tds_config_boolean(value, login);
>> } else if (!strcmp(option, TDS_STR_INSTANCE)) {
>> tds_dstr_copy(&login->instance_name, value);
>> } else if (!strcmp(option, TDS_STR_ENCRYPTION)) {
>> @@ -625,9 +629,9 @@ tds_parse_conf_section(const char *option, const
>> char *value, void *param)
>> } else if (!strcmp(option, TDS_STR_ASA_DATABASE)) {
>> tds_dstr_copy(&login->server_name, value);
>> } else if (!strcmp(option, TDS_STR_USENTLMV2)) {
>> - login->use_ntlmv2 = tds_config_boolean(value);
>> + login->use_ntlmv2 = tds_config_boolean(value, login);
>> } else if (!strcmp(option, TDS_STR_USELANMAN)) {
>> - login->use_lanman = tds_config_boolean(value);
>> + login->use_lanman = tds_config_boolean(value, login);
>> } else if (!strcmp(option, TDS_STR_REALM)) {
>> tds_dstr_copy(&login->server_realm_name, value);
>> } else if (!strcmp(option, TDS_STR_SPN)) {
>> diff --git a/src/tds/login.c b/src/tds/login.c
>> index ec8eb5a..813290b 100644
>> --- a/src/tds/login.c
>> +++ b/src/tds/login.c
>> @@ -333,6 +333,9 @@ tds_connect(TDSSOCKET * tds, TDSLOGIN * login, int
>> *p_oserr)
>> , 0x402
>> };
>>
>> + if (login->invalid_configuration)
>> + return TDS_FAIL;
>> +
>> if (TDS_MAJOR(login) == 0) {
>> unsigned int i;
>> TDSSAVECONTEXT save_ctx;
>> --
>> 1.7.9.5
>>
>> 2013/10/22 Marc Abramowitz <msabramo AT gmail.com>:
>> >> I like tdserror. I don't like printf or an additional option. Printf
>> > cause it
>> >> can go nowhere or in dangerous places, an additional option cause it
>> force
>> >> different users to recompile the same library with different options
>> > based on
>> >> the usage. Using options to deal with debugging or different system
>> >> configuration is fine but this option would be ok for an application but
>> > not
>> >> for a library used in different places.
>> >
>> >> What I would do, also suggested by James is something strong. If you
>> pass
>> > a
>> >> wrong option just try to notify upper layer (as tdserror does) and do
>> not
>> >> connect! Surely user will fix the configuration! Of course should be
>> >> documented in the version changelog and UG. I'll try to write a partial,
>> > not
>> >> that tested patch. Does it sound reasonable?
>> >
>> > I had another branch where I did something like that actually. I just
>> > cleaned it up and made another merge request.
>> >
>> > https://gitorious.org/freetds/freetds/merge_requests/18
>> >
>> > If that resembles or is useful towards implementing what you suggested,
>> > feel free to use it, or if it's different, you can go ahead and do your
>> own
>> > thing. The idea certainly sounds reasonable.
>> >
>> >
>> > On Mon, Oct 21, 2013 at 4:51 PM, Frediano Ziglio <freddy77 AT gmail.com>
>> wrote:
>> >
>> >> I like tdserror. I don't like printf or an additional option. Printf
>> cause
>> >> it can go nowhere or in dangerous places, an additional option cause it
>> >> force different users to recompile the same library with different
>> options
>> >> based on the usage. Using options to deal with debugging or different
>> >> system configuration is fine but this option would be ok for an
>> application
>> >> but not for a library used in different places.
>> >>
>> >> What I would do, also suggested by James is something strong. If you
>> pass
>> >> a wrong option just try to notify upper layer (as tdserror does) and do
>> not
>> >> connect! Surely user will fix the configuration! Of course should be
>> >> documented in the version changelog and UG. I'll try to write a partial,
>> >> not that tested patch. Does it sound reasonable?
>> >>
>> >> Ps: we have still to sort out the Travis patch. Does the configuration
>> >> file need to be in the main directory? Cannot you change the name? What
>> >> happen if another developers want to use Travis too?
>> >>
>> >> Frediano Ziglio
>> >>
>> >> Il giorno 21/ott/2013, alle ore 21:48, Marc Abramowitz <
>> msabramo AT gmail.com>
>> >> ha scritto:
>> >>
>> >> > That makes sense.
>> >> >
>> >> > OK, I took another stab at creating a louder notification when the
>> >> > encryption setting is incorrect:
>> >> >
>> >> > *https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
>> >> > handling of invalid encryption setting")
>> >> >
>> >> > The goal here is to make it much more noticeable when there is an
>> invalid
>> >> > encryption setting. I do this by:
>> >> >
>> >> > - calling tdserror with a new error code (TDSECONF = 20214 = "Local
>> >> > configuration error")
>> >> > - print a message to stderr in case in case someone is looking there
>> >> > - exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is
>> defined
>> >> > (defined by passing --enable-exit-on-invalid-config to./configure).
>> >> >
>> >> >
>> >> >
>> >> > On Sat, Oct 19, 2013 at 11:49 AM, James K. Lowden <
>> jklowden AT freetds.org
>> >> >wrote:
>> >> >
>> >> >> On Sat, 19 Oct 2013 09:48:12 -0700
>> >> >> Marc Abramowitz <msabramo AT gmail.com> wrote:
>> >> >>
>> >> >>>>> Merge request to make an invalid encryption setting a fatal error:
>> >> >>>>> https://gitorious.org/freetds/freetds/merge_requests/10
>> >> >>>>
>> >> >>>> Ouch indeed. I think the next release of FreeTDS should probably
>> >> >>>> reject all incorrect configurations, both invalid keywords and
>> >> >>>> out-of-range values. Diagnosing configuration errors is easier if
>> >> >>>> the error is obvious, and the remedy is simple.
>> >> >>>
>> >> >>> Freddy pointed out that my merge request wasn't acceptable because
>> >> >>> printing to stderr and exit aren't appropriate for a library.
>> >> >> ...
>> >> >>> Is there some middle ground - some way of indicating a config error
>> >> >>> (esp. a serious config error like an invalid encryption setting)
>> that
>> >> >>> is very noticeable but doesn't overstep what a library should do?
>> >> >>
>> >> >> The Right Way is to hook into the error handler. Add something like
>> >> >> this to the table in sybdb.h:
>> >> >>
>> >> >> #define SYBECONF 2501 /* local configuration error */
>> >> >>
>> >> >> and to the dblib.c::dblib_error_messages table
>> >> >>
>> >> >> and invoke that error with tdserror or dbperror.
>> >> >>
>> >> >> It's not obvious to me that printing to standard error and exiting is
>> >> >> all that bad, though.
>> >> >>
>> >> >> In general, there are two kinds of errors:
>> >> >>
>> >> >> 1. logic errors, caused by the programmer getting something wrong.
>> >> >> Examples include off-by-one errors and failure to check for a NULL
>> >> >> pointer. These kinds of problems can and do cause the library to
>> >> >> exit via e.g. assert(3). There is no point in continuing, because
>> the
>> >> >> program has reached an "impossible" state that the program did not
>> >> >> anticipate and thus cannot handle.
>> >> >>
>> >> >> 2. runtime errors, caused by invalid user input or dynamic failures
>> >> >> such as network disconnection or the server not responding. These
>> >> >> should always return an error to the user, who is the only person in
>> a
>> >> >> position to deal with it.
>> >> >>
>> >> >> Is an invalid configuration a logic or runtime error? is the
>> >> >> administrator a programmer or a user?
>> >> >>
>> >> >> I would say the configuration file is more like program text. It is
>> >> >> static data isolated from the program in such a way that it can be
>> >> >> modified without requiring recompilation. The very first time it is
>> >> >> read -- compiled, if you will -- it will be validated. If it is
>> >> >> invalid, the library exits. Once the configuration is valid, no
>> amount
>> >> >> of other invalid input will cause it to become invalid.
>> >> >>
>> >> >> Printing to stderr is unfortunately not robust, because some
>> >> >> applications will not have that descriptor open. Invoking the error
>> >> >> handler is is more general, but will go unseen if the application
>> >> >> doesn't install and error handler. On the other hand, the problem
>> can be
>> >> >> instantantly diagnosed by using any command-line tool such as tsql.
>> >> >>
>> >> >> So, IMO printing to stderr and exiting is fine, and invoking the
>> error
>> >> >> handler is in some ways friendlier (perhaps in addition to writing to
>> >> >> stderr). Exiting is OK, too, even if it does force the admin to
>> clean
>> >> >> up errors that might otherwise be innocuous.
>> >> >>
>> >> >> HTH.
>> >> >>
>> >> >> --jkl
>> >> >> _______________________________________________
>> >> >> FreeTDS mailing list
>> >> >> FreeTDS AT lists.ibiblio.org
>> >> >> http://lists.ibiblio.org/mailman/listinfo/freetds
>> >> >>
>> >> > _______________________________________________
>> >> > FreeTDS mailing list
>> >> > FreeTDS AT lists.ibiblio.org
>> >> > http://lists.ibiblio.org/mailman/listinfo/freetds
>> >> _______________________________________________
>> >> FreeTDS mailing list
>> >> FreeTDS AT lists.ibiblio.org
>> >> http://lists.ibiblio.org/mailman/listinfo/freetds
>> >>
>> > _______________________________________________
>> > FreeTDS mailing list
>> > FreeTDS AT lists.ibiblio.org
>> > http://lists.ibiblio.org/mailman/listinfo/freetds
>> _______________________________________________
>> FreeTDS mailing list
>> FreeTDS AT lists.ibiblio.org
>> http://lists.ibiblio.org/mailman/listinfo/freetds
>>
> _______________________________________________
> FreeTDS mailing list
> FreeTDS AT lists.ibiblio.org
> http://lists.ibiblio.org/mailman/listinfo/freetds




Archive powered by MHonArc 2.6.24.

Top of Page