Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: implement TLS auto-detection #372

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

RFC: implement TLS auto-detection #372

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
7fa17ee
light: allow the use of "auto" transport for syslog() source drivers
bazsi Feb 1, 2025
17cacbe
logproto: rename prepare() methods to poll_prepare
bazsi Nov 8, 2024
7acce3d
transport-stack: add poll_prepare() methods for LogTransport and LogT…
bazsi Nov 8, 2024
d31e254
transport-stack: add LogTransportStack level read/write/writev methods
bazsi Nov 10, 2024
008fc75
transport-adapter: add free_method
bazsi Feb 1, 2025
43ed813
transport/tls-context: simplify compression setup
bazsi Feb 1, 2025
fcddbce
transport-tls: convert to a LogTransportAdapter
bazsi Nov 8, 2024
b2d6bb4
transport-aux-data: add log_transport_aux_data_move()
bazsi Nov 11, 2024
6d026b0
transport-stack: make it possible to add aux data from the LogTranspo…
bazsi Nov 10, 2024
93df75b
logproto: call transport-stack level I/O methods
bazsi Nov 8, 2024
9853f73
logproto/logproto-auto-server: remove RFC6587 references from log mes…
bazsi Feb 1, 2025
8eaec3e
templates: add $SOURCEPORT macro
bazsi Feb 1, 2025
eea12d4
transport-haproxy: use the normal $SOURCEIP, $DESTIP, $DESTPORT macros
bazsi Nov 2, 2024
26ac510
transport-haproxy: use the LogTransportStack->aux to save src/dst add…
bazsi Nov 10, 2024
1d1d126
transport-haproxy: implement transport switch instead of changing bas…
bazsi Nov 10, 2024
492e17e
transport-haproxy: add more information to the proxy detection failur…
bazsi Nov 10, 2024
8acb805
afsocket: refactor TransportMapperInet transport setup logic
bazsi Feb 1, 2025
6b1ee3b
afsocket: transport(auto) with tls() should immediately start using TLS
bazsi Feb 1, 2025
1794e27
transport/transport-factory-haproxy: use factory for creating HAProxy…
bazsi Feb 1, 2025
c435cf1
light: generalize test_pp_network and test_pp_syslog test cases
bazsi Feb 1, 2025
7b7ce57
light: fix up files included in the dist
bazsi Feb 2, 2025
e8322f7
news: added news file
bazsi Dec 3, 2024
bbe0595
logproto: add support for auto-detecting TLS handshakes
bazsi Nov 10, 2024
770c78a
logproto-auto-server: also recognize TLS alerts and reject them expli…
bazsi Feb 2, 2025
7c9a46a
logproto-auto-server: add detection of binary data
bazsi Feb 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 103 additions & 14 deletions lib/logproto/logproto-auto-server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,43 +25,107 @@
#include "logproto-framed-server.h"
#include "messages.h"

enum
{
LPAS_FAILURE,
LPAS_NEED_MORE_DATA,
LPAS_SUCCESS,
};

typedef struct _LogProtoAutoServer
{
LogProtoServer super;

gboolean tls_detected;
} LogProtoAutoServer;

static LogProtoServer *
_construct_detected_proto(LogProtoAutoServer *self, const gchar *detect_buffer, gsize detect_buffer_len)
{
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);
gint fd = self->super.transport_stack.fd;

if (g_ascii_isdigit(detect_buffer[0]))
{
msg_debug("Auto-detected octet-counted-framing on RFC6587 connection, using framed protocol",
evt_tag_int("fd", transport->fd));
msg_debug("Auto-detected octet-counted-framing, using framed protocol",
evt_tag_int("fd", fd));
return log_proto_framed_server_new(NULL, self->super.options);
}
if (detect_buffer[0] == '<')
{
msg_debug("Auto-detected non-transparent-framing on RFC6587 connection, using simple text protocol",
evt_tag_int("fd", transport->fd));
msg_debug("Auto-detected non-transparent-framing, using simple text protocol",
evt_tag_int("fd", fd));
}
else
{
msg_debug("Unable to detect framing on RFC6587 connection, falling back to simple text transport",
evt_tag_int("fd", transport->fd),
msg_debug("Unable to detect framing, falling back to simple text transport",
evt_tag_int("fd", fd),
evt_tag_mem("detect_buffer", detect_buffer, detect_buffer_len));
}
return log_proto_text_server_new(NULL, self->super.options);
}

static gint
_is_tls_client_hello(const gchar *buf, gsize buf_len)
{
/*
* The first message on a TLS connection must be the client_hello, which
* is a type of handshake record, and it cannot be compressed or
* encrypted. A plaintext record has this format:
*
* 0 byte record_type // 0x16 = handshake
* 1 byte major // major protocol version
* 2 byte minor // minor protocol version
* 3-4 uint16 length // size of the payload
* 5 byte handshake_type // 0x01 = client_hello
* 6 uint24 length // size of the ClientHello
* 9 byte major // major protocol version
* 10 byte minor // minor protocol version
* 11 uint32 gmt_unix_time
* 15 byte random_bytes[28]
* ...
*/
if (buf_len < 1)
return LPAS_NEED_MORE_DATA;

/* 0x16 indicates a TLS handshake */
if (buf[0] != 0x16)
return LPAS_FAILURE;

if (buf_len < 5)
return LPAS_NEED_MORE_DATA;

guint32 record_len = (buf[3] << 8) + buf[4];

/* client_hello is at least 34 bytes */
if (record_len < 34)
return LPAS_FAILURE;

if (buf_len < 6)
return LPAS_NEED_MORE_DATA;

/* is the handshake_type 0x01 == client_hello */
if (buf[5] != 0x01)
return FALSE;

if (buf_len < 9)
return LPAS_NEED_MORE_DATA;

guint32 payload_size = (buf[6] << 16) + (buf[7] << 8) + buf[8];

/* The message payload can't be bigger than the enclosing record */
if (payload_size + 4 > record_len)
return LPAS_FAILURE;
return LPAS_SUCCESS;
}

static LogProtoPrepareAction
log_proto_auto_server_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout G_GNUC_UNUSED)
log_proto_auto_server_poll_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout G_GNUC_UNUSED)
{
LogProtoAutoServer *self = (LogProtoAutoServer *) s;
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);

*cond = transport->cond;
if (log_transport_stack_poll_prepare(&self->super.transport_stack, cond))
return LPPA_FORCE_SCHEDULE_FETCH;

if (*cond == 0)
*cond = G_IO_IN;

Expand All @@ -72,12 +136,11 @@ static LogProtoStatus
log_proto_auto_handshake(LogProtoServer *s, gboolean *handshake_finished, LogProtoServer **proto_replacement)
{
LogProtoAutoServer *self = (LogProtoAutoServer *) s;
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);
gchar detect_buffer[8];
gchar detect_buffer[16];
gboolean moved_forward;
gint rc;

rc = log_transport_read_ahead(transport, detect_buffer, sizeof(detect_buffer), &moved_forward);
rc = log_transport_stack_read_ahead(&self->super.transport_stack, detect_buffer, sizeof(detect_buffer), &moved_forward);
if (rc == 0)
return LPS_EOF;
else if (rc < 0)
Expand All @@ -87,6 +150,32 @@ log_proto_auto_handshake(LogProtoServer *s, gboolean *handshake_finished, LogPro
return LPS_ERROR;
}

if (!self->tls_detected)
{
switch (_is_tls_client_hello(detect_buffer, rc))
{
case LPAS_NEED_MORE_DATA:
if (moved_forward)
return LPS_AGAIN;
break;
case LPAS_SUCCESS:
self->tls_detected = TRUE;
/* this is a TLS handshake! let's switch to TLS */
if (log_transport_stack_switch(&self->super.transport_stack, LOG_TRANSPORT_TLS))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLS auto-detection sounds like a security issue for me. TLS provides encryption and authentication.
If we want to allow non-TLS and TLS traffic on the same port, we should notify the user with a big warning that neither encryption nor authentication is guaranteed. We could probably do this when peer-verify(off) is set, but even then we should warn the user that encryption is also optional so vulnerable to "downgrade attack".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with that in general, however I think the syslog over TLS use-case is slightly different.

  1. everything is unauthenticated and plain text by default (unfortunately)
  2. the sensitive data is always on the client side and syslog itself is uni-directional, the server does not respond to anything
  3. there's no negotiation, the syslog client ultimately determines whether the syslog traffic is using TLS or not.
  4. the auto detection only applies to the server, not on the client

With those in mind:

  1. Downgrade of the client: this is not possible, as the client has TLS enabled or it doesn't. If we had an active attacker, and this would be merged, the attacker could fool the server to accept plain text instead of TLS. But this does not change how the client is configured: the client would require TLS anyway with both encryption and authentication.

  2. Downgrade of the server: this is indeed possible, client talking TLS while the server receives plain text. Going back to the first case, if the client has TLS configured and authenticates the server, the attacker would need to have a trusted X.509 certificate. In which case they don't even need to talk to the server.

And some more context:

  • with a patch like this, it actually becomes a lot easier to deploy TLS on the clients, no need to reconfigure firewalls, load balancers or change ports on the client. You "just" configure tls() on the server on port 514 with PKI issued certificate and then you can incrementally enable the use of TLS on any of the clients, without any further configuration. Which means that you could end up with TLS encryption of log data, which is a big win.

Copy link
Member

@MrAnno MrAnno Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is a useful feature, I would like to just add some security measures of how we enable such functionality. From the server's perspective, encrypted and non-encrypted, authenticated and non-authenticated data will flow from the same source and the user has very little influence on how to distinguish between the two (same source -> same log paths, same destinations, and we only have a few TLS-related macros to filter for authenticated and/or encrypted data).

My suggestion would be the following:

  • transport(auto) should allow everything that does not have security implications
  • transport(auto) + optional-tls(yes/no) should enable TLS auto-detection (I'm open to rename the option to anything else). In the documentation of this option, we should mention how users can filter for TLS-encrypted/authenticated data using macros.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree completely. That's exactly what we discussed in the past couple of days. That's why the patch is in RFC yet.

So, yes, I agree we need a way to specify require-tls(yes). And I think we should also revisit information about a message being encrypted or not once we receive it

{
msg_debug("TLS handshake detected, switching to TLS");
return LPS_AGAIN;
}
else
{
msg_error("TLS handshake detected, unable to switch to TLS, no tls() options specified");
return LPS_ERROR;
}
break;
default:
break;
}
}
*proto_replacement = _construct_detected_proto(self, detect_buffer, rc);
return LPS_SUCCESS;
}
Expand All @@ -101,6 +190,6 @@ log_proto_auto_server_new(LogTransport *transport, const LogProtoServerOptions *

log_proto_server_init(&self->super, transport, options);
self->super.handshake = log_proto_auto_handshake;
self->super.prepare = log_proto_auto_server_prepare;
self->super.poll_prepare = log_proto_auto_server_poll_prepare;
return &self->super;
}
17 changes: 7 additions & 10 deletions lib/logproto/logproto-buffered-server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,9 @@ log_proto_buffered_server_apply_state(LogProtoBufferedServer *self, PersistEntry
struct stat st;
gint64 ofs = 0;
LogProtoBufferedServerState *state;
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);
gint fd;

fd = transport->fd;
fd = self->super.transport_stack.fd;
self->persist_handle = handle;

if (fstat(fd, &st) < 0)
Expand Down Expand Up @@ -255,7 +254,7 @@ log_proto_buffered_server_apply_state(LogProtoBufferedServer *self, PersistEntry
raw_buffer = g_alloca(state->raw_buffer_size);
}

rc = log_transport_read(transport, raw_buffer, state->raw_buffer_size, NULL);
rc = log_transport_stack_read(&self->super.transport_stack, raw_buffer, state->raw_buffer_size, NULL);
if (rc != state->raw_buffer_size)
{
msg_notice("Error re-reading buffer contents of the file to be continued, restarting from the beginning",
Expand Down Expand Up @@ -584,12 +583,12 @@ log_proto_buffered_server_restart_with_state(LogProtoServer *s, PersistState *pe
}

LogProtoPrepareAction
log_proto_buffered_server_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout G_GNUC_UNUSED)
log_proto_buffered_server_poll_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout G_GNUC_UNUSED)
{
LogProtoBufferedServer *self = (LogProtoBufferedServer *) s;
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);

*cond = transport->cond;
if (log_transport_stack_poll_prepare(&self->super.transport_stack, cond))
return LPPA_FORCE_SCHEDULE_FETCH;

/* if there's no pending I/O in the transport layer, then we want to do a read */
if (*cond == 0)
Expand All @@ -602,9 +601,7 @@ static gint
log_proto_buffered_server_read_data_method(LogProtoBufferedServer *self, guchar *buf, gsize len,
LogTransportAuxData *aux)
{
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);

return log_transport_read(transport, buf, len, aux);
return log_transport_stack_read(&self->super.transport_stack, buf, len, aux);
}

static void
Expand Down Expand Up @@ -1081,7 +1078,7 @@ log_proto_buffered_server_init(LogProtoBufferedServer *self, LogTransport *trans
const LogProtoServerOptions *options)
{
log_proto_server_init(&self->super, transport, options);
self->super.prepare = log_proto_buffered_server_prepare;
self->super.poll_prepare = log_proto_buffered_server_poll_prepare;
self->super.fetch = log_proto_buffered_server_fetch;
self->super.free_fn = log_proto_buffered_server_free_method;
self->super.restart_with_state = log_proto_buffered_server_restart_with_state;
Expand Down
4 changes: 2 additions & 2 deletions lib/logproto/logproto-buffered-server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ log_proto_buffered_server_cue_flush(LogProtoBufferedServer *self)
self->flush_partial_message = TRUE;
}

LogProtoPrepareAction log_proto_buffered_server_prepare(LogProtoServer *s, GIOCondition *cond,
gint *timeout G_GNUC_UNUSED);
LogProtoPrepareAction log_proto_buffered_server_poll_prepare(LogProtoServer *s, GIOCondition *cond,
gint *timeout G_GNUC_UNUSED);
LogProtoBufferedServerState *log_proto_buffered_server_get_state(LogProtoBufferedServer *self);
void log_proto_buffered_server_put_state(LogProtoBufferedServer *self);
gboolean log_proto_buffered_server_restart_with_state(LogProtoServer *s,
Expand Down
7 changes: 3 additions & 4 deletions lib/logproto/logproto-client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ struct _LogProtoClient
LogProtoStatus status;
const LogProtoClientOptions *options;
LogTransportStack transport_stack;
/* FIXME: rename to something else */
gboolean (*prepare)(LogProtoClient *s, gint *fd, GIOCondition *cond, gint *timeout);
gboolean (*poll_prepare)(LogProtoClient *s, gint *fd, GIOCondition *cond, gint *timeout);
LogProtoStatus (*post)(LogProtoClient *s, LogMessage *logmsg, guchar *msg, gsize msg_len, gboolean *consumed);
LogProtoStatus (*process_in)(LogProtoClient *s);
LogProtoStatus (*flush)(LogProtoClient *s);
Expand Down Expand Up @@ -124,9 +123,9 @@ log_proto_client_handshake(LogProtoClient *s, gboolean *handshake_finished)
}

static inline gboolean
log_proto_client_prepare(LogProtoClient *s, gint *fd, GIOCondition *cond, gint *timeout)
log_proto_client_poll_prepare(LogProtoClient *s, gint *fd, GIOCondition *cond, gint *timeout)
{
gboolean result = s->prepare(s, fd, cond, timeout);
gboolean result = s->poll_prepare(s, fd, cond, timeout);

if (!result && *timeout < 0)
*timeout = s->options->idle_timeout;
Expand Down
18 changes: 9 additions & 9 deletions lib/logproto/logproto-framed-server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ typedef struct _LogProtoFramedServer
} LogProtoFramedServer;

static LogProtoPrepareAction
log_proto_framed_server_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout G_GNUC_UNUSED)
log_proto_framed_server_poll_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout G_GNUC_UNUSED)
{
LogProtoFramedServer *self = (LogProtoFramedServer *) s;
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);

*cond = transport->cond;
if (log_transport_stack_poll_prepare(&self->super.transport_stack, cond))
return LPPA_FORCE_SCHEDULE_FETCH;

/* there is a half message in our buffer so try to wait */
if (!self->half_message_in_buffer)
Expand All @@ -97,7 +97,6 @@ static gboolean
log_proto_framed_server_fetch_data(LogProtoFramedServer *self, gboolean *may_read,
LogProtoStatus *status)
{
LogTransport *transport = log_transport_stack_get_active(&self->super.transport_stack);
gint rc;
*status = LPS_SUCCESS;

Expand All @@ -111,15 +110,16 @@ log_proto_framed_server_fetch_data(LogProtoFramedServer *self, gboolean *may_rea
return FALSE;

log_transport_aux_data_reinit(&self->buffer_aux);
rc = log_transport_read(transport, &self->buffer[self->buffer_end], self->buffer_size - self->buffer_end,
&self->buffer_aux);
rc = log_transport_stack_read(&self->super.transport_stack,
&self->buffer[self->buffer_end], self->buffer_size - self->buffer_end,
&self->buffer_aux);

if (rc < 0)
{
if (errno != EAGAIN)
{
msg_error("Error reading RFC6587 style framed data",
evt_tag_int("fd", transport->fd),
evt_tag_int("fd", self->super.transport_stack.fd),
evt_tag_error("error"));
*status = LPS_ERROR;
}
Expand All @@ -134,7 +134,7 @@ log_proto_framed_server_fetch_data(LogProtoFramedServer *self, gboolean *may_rea
if (rc == 0)
{
msg_trace("EOF occurred while reading",
evt_tag_int(EVT_TAG_FD, transport->fd));
evt_tag_int(EVT_TAG_FD, self->super.transport_stack.fd));
*status = LPS_EOF;
return FALSE;
}
Expand Down Expand Up @@ -430,7 +430,7 @@ log_proto_framed_server_new(LogTransport *transport, const LogProtoServerOptions
LogProtoFramedServer *self = g_new0(LogProtoFramedServer, 1);

log_proto_server_init(&self->super, transport, options);
self->super.prepare = log_proto_framed_server_prepare;
self->super.poll_prepare = log_proto_framed_server_poll_prepare;
self->super.fetch = log_proto_framed_server_fetch;
self->super.free_fn = log_proto_framed_server_free;
self->half_message_in_buffer = FALSE;
Expand Down
5 changes: 2 additions & 3 deletions lib/logproto/logproto-record-server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,16 @@ static gint
log_proto_record_server_read_data(LogProtoBufferedServer *s, guchar *buf, gsize len, LogTransportAuxData *aux)
{
LogProtoRecordServer *self = (LogProtoRecordServer *) s;
LogTransport *transport = log_transport_stack_get_active(&self->super.super.transport_stack);
gint rc;

/* assert that we have enough space in the buffer to read record_size bytes */
g_assert(len >= self->record_size);
len = self->record_size;
rc = log_transport_read(transport, buf, len, aux);
rc = log_transport_stack_read(&s->super.transport_stack, buf, len, aux);
if (rc > 0 && rc != self->record_size)
{
msg_error("Record size was set, and couldn't read enough bytes",
evt_tag_int(EVT_TAG_FD, transport->fd),
evt_tag_int(EVT_TAG_FD, s->super.transport_stack.fd),
evt_tag_int("record_size", self->record_size),
evt_tag_int("read", rc));
errno = EIO;
Expand Down
7 changes: 3 additions & 4 deletions lib/logproto/logproto-server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ struct _LogProtoServer
AckTracker *ack_tracker;

LogProtoServerWakeupCallback wakeup_callback;
/* FIXME: rename to something else */
LogProtoPrepareAction (*prepare)(LogProtoServer *s, GIOCondition *cond, gint *timeout);
LogProtoPrepareAction (*poll_prepare)(LogProtoServer *s, GIOCondition *cond, gint *timeout);
gboolean (*restart_with_state)(LogProtoServer *s, PersistState *state, const gchar *persist_name);
LogProtoStatus (*fetch)(LogProtoServer *s, const guchar **msg, gsize *msg_len, gboolean *may_read,
LogTransportAuxData *aux, Bookmark *bookmark);
Expand Down Expand Up @@ -125,9 +124,9 @@ log_proto_server_set_options(LogProtoServer *self, const LogProtoServerOptions *
}

static inline LogProtoPrepareAction
log_proto_server_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout)
log_proto_server_poll_prepare(LogProtoServer *s, GIOCondition *cond, gint *timeout)
{
LogProtoPrepareAction result = s->prepare(s, cond, timeout);
LogProtoPrepareAction result = s->poll_prepare(s, cond, timeout);

if (result == LPPA_POLL_IO && *timeout < 0)
*timeout = s->options->idle_timeout;
Expand Down
Loading