ssl_options() option inside tls() #457

pasztor · 2015-04-08T09:02:29Z

allowed values: no-sslv2, no-sslv3, no-tlsv1, no-tlsv11, no-tlsv12, none
I think, they are self-explanatory.

Signed-off-by: PÁSZTOR György gyorgy.pasztor@balabit.com

allowed values: no-sslv2, no-sslv3, no-tlsv1, no-tlsv11, no-tlsv12, none I think, they are self-explanatory. Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

* GList *l -> Glist *option * handle if tls library does not know SSL_OP_NO_TLSv1_2. * I put into the same block the handling of SSL_OP_NO_TLSv1_1, since they appeared at the same time ssl: 1.0.0->1.0.1 * removed comment Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

* option change back to l, since that is an iterater. See bazsi's comment * remove the "none" option. if we want an empty set instead of no-sslv2, then we can write ssl_options() which also means an empty set. So, based on this, we do not need the "none" keyword. however, recent ssl libraries does not allow sslv2, even if we do not set the SSL_OP_NO_SSLv2 option. Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

deirf · 2015-05-05T09:47:27Z

👍

juhaszviktor · 2015-05-20T14:55:10Z

lib/tlscontext.h

+  TSO_NOTLSv11=0x0008,
+  TSO_NOTLSv12=0x0010,
+} TLSSslOptions;
+


You use TlsSslOption as a flag, but the type of it is enumeration.
Enumerations are usually used for unique values, not for combinations of these.
I mean, that you should "typedef unsigned int TlsSslOptions" and declare the TSO_* as define (e.g.: #define TSO_NOSSLv2 1)

I would slightly disagree. Enums are available in a debugger while defines
are not.
On May 20, 2015 4:55 PM, "juhaszviktor" notifications@github.com wrote:

In lib/tlscontext.h
#457 (comment):

@@ -44,6 +44,16 @@ typedef enum
TVM_REQUIRED=0x0020,
} TLSVerifyMode;

+typedef enum
+{

TSO_NONE,

TSO_NOSSLv2=0x0001,

TSO_NOSSLv3=0x0002,

TSO_NOTLSv1=0x0004,

TSO_NOTLSv11=0x0008,

TSO_NOTLSv12=0x0010,
+} TLSSslOptions;

You use TlsSslOption as a flag, but the type of it is enumeration.
Enumerations are usually used for unique values, not for combinations of
these.
I mean, that you should "typedef unsigned int TlsSslOptions" and declare
the TSO_* as define (e.g.: #define TSO_NOSSLv2 1)

—
Reply to this email directly or view it on GitHub
https://github.com/balabit/syslog-ng/pull/457/files#r30710623.

I can agree your standpoint, but if I look at the whole syslog-ng code, the flags and some kind of options are declared as a define. For example in logwriter.h LW_* and LWO_* are also defines.

And what if we have a typedef for TlsSslOption and static const TlsSslOption delarations for the values? Or we just change the return type of tls_lookup_options to int...

I think the problem with enum is that when we return from tls_lookup_options we say that type of the value is TLSSslOptions but it's not: it`s a combination of the enumerated values.

a number of this calls were converted to enums recently. I did start to
move to the enum direction. An example is TLSVerifyMode just above this
declaration. Or msg-format.h, or a number of others. So this is not
consistent. I do know that I was converting a couple of these.

In these cases the call sites do not use the enum type (I don't even see
the use for the typedef itself, that should be abandoned), the the
call-sites should use "int"

Bazsi

On Wed, May 20, 2015 at 8:09 PM, juhaszviktor notifications@github.com
wrote:

In lib/tlscontext.h
#457 (comment):

@@ -44,6 +44,16 @@ typedef enum
TVM_REQUIRED=0x0020,
} TLSVerifyMode;

+typedef enum
+{

TSO_NONE,

TSO_NOSSLv2=0x0001,

TSO_NOSSLv3=0x0002,

TSO_NOTLSv1=0x0004,

TSO_NOTLSv11=0x0008,

TSO_NOTLSv12=0x0010,
+} TLSSslOptions;

I can agree your standpoint, but if I look at the whole syslog-ng code,
the flags and some kind of options are declared as a define. For example in
logwriter.h LW_* and LWO_* are also defines.

—
Reply to this email directly or view it on GitHub
https://github.com/balabit/syslog-ng/pull/457/files#r30731421.

Ok, then we can keep enum, eliminate typedef and just use an int as return type.

I tried to be consequent with the solution what was already there for the verify options. I did pretty much the same.
If my solution is bad, then verify options's is bad too.
If you'd like, I'd gladly correct both of them, but I think that should be a scope of a different PR, and this PR should focus only to implement the ssl-options() setting.
Opinions?

hi,

I'm reluctant to merge the patch in this form.
Please, use int as return type, as I suggested before (and you can keep enum... ).

L.

hi,

as I said: this is consistent with the other parts from the same module in this form.
I also said, that I agree with you about the typedef enum part.
So, I am wondering if

you do not trust my word, that I will make a code cleaning for this module where I fix the ssl-options and the verify-mode both

or you want a code where two very-very similar functions (tls_lookup_verify_mode, tls_lookup_options) are defined two different ways in the same code

or you want a larger patch, where a code cleanup is done at the same branch where a new function/ option is implemented?

hi,

I told you IRL that this is not about consistency: it's about correctness.
(a legacy code will always contain some legacy code ;-) That doesn't mean we should always follow it
and doesn't mean that it cannot be changed... )

I think 2nd option in your list would be fine with an additional note:
If we have a chance for writing a not-legacy code, we should have to do that, IMHO.

L.

coding style fixation as jviktor asked lib/tlscontext.h: try to be consequent with verify_mode so TLSSslOptions goes into gint inside the TLSContext struct Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

justcallmegreg · 2015-06-12T10:04:52Z

lib/tlscontext.c

@@ -280,6 +280,7 @@ tls_context_setup_session(TLSContext *self)
  SSL *ssl;
  TLSSession *session;
  gint ssl_error;
+  long ssl_options;


I know no reason why this option should be long.

I just say only one: Manual page SSL_CTX_set_options(3ssl)
SYNOPSIS
#include <openssl/ssl.h>

long SSL_CTX_set_options(SSL_CTX *ctx, long options);

That internal variable is for prepare the value what SSL_CTX_set_options will get.
Is that satisfactory enough? ;-)

Absolutely. :)

* tls_lookup_options return type is gint now, as lbudai asked * TLSSslOptions is not typedef-ed now, it's just an enum now Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

lbudai · 2015-06-24T14:31:13Z

Thanks!!

ssl_options() option inside tls()

PÁSZTOR György added 3 commits April 8, 2015 11:00

ssl_options() option inside tls()

d02fb42

allowed values: no-sslv2, no-sslv3, no-tlsv1, no-tlsv11, no-tlsv12, none I think, they are self-explanatory. Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

juhaszviktor reviewed May 20, 2015
View reviewed changes

lib/tlscontext.c:

3697504

coding style fixation as jviktor asked lib/tlscontext.h: try to be consequent with verify_mode so TLSSslOptions goes into gint inside the TLSContext struct Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

justcallmegreg reviewed Jun 12, 2015
View reviewed changes

deirf added the in progress label Jun 19, 2015

lib/tlscontext.c & h:

f5f0eb1

* tls_lookup_options return type is gint now, as lbudai asked * TLSSslOptions is not typedef-ed now, it's just an enum now Signed-off-by: PÁSZTOR György <gyorgy.pasztor@balabit.com>

lbudai added a commit that referenced this pull request Jun 24, 2015

Merge pull request #457 from pasztor/f/ssl_options

acf4339

ssl_options() option inside tls()

lbudai merged commit acf4339 into syslog-ng:master Jun 24, 2015

lbudai removed the in progress label Jun 24, 2015

algernon mentioned this pull request Oct 1, 2015

ssl-options backport for 3.6 #720

Merged

MrAnno mentioned this pull request May 31, 2019

How to disable tls1.0 on syslog-ng verison 3.6 #2760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ssl_options() option inside tls() #457

ssl_options() option inside tls() #457

pasztor commented Apr 8, 2015

deirf commented May 5, 2015

juhaszviktor May 20, 2015

bazsi May 20, 2015

juhaszviktor May 20, 2015

lbudai May 20, 2015

bazsi May 20, 2015

lbudai May 21, 2015

pasztor Jun 10, 2015

lbudai Jun 19, 2015

pasztor Jun 22, 2015

lbudai Jun 22, 2015

justcallmegreg Jun 12, 2015

pasztor Jun 12, 2015

justcallmegreg Jun 12, 2015

lbudai commented Jun 24, 2015

ssl_options() option inside tls() #457

ssl_options() option inside tls() #457

Conversation

pasztor commented Apr 8, 2015

deirf commented May 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbudai commented Jun 24, 2015