Skip to Content.
Sympa Menu

freetds - Re: [freetds] Encrypted connection

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: Marc Abramowitz <msabramo AT gmail.com>
  • To: FreeTDS Development Group <freetds AT lists.ibiblio.org>
  • Subject: Re: [freetds] Encrypted connection
  • Date: Tue, 22 Oct 2013 13:46:58 -0700

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
>




Archive powered by MHonArc 2.6.24.

Top of Page