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 21:13:30 +0100

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




Archive powered by MHonArc 2.6.24.

Top of Page