Add common proxy solution for all the supported transport methods that works transparently in the network() and the syslog() sources #4544
Conversation
alexb271
changed the title
Signed-off-by: Alex Becker <beckeralex@protonmail.com>
|
First of all, sorry for this taking this long. I find I have less than the ideal amount of time to do code level changes to syslog-ng these days. This looks so MUCH better than the previous iteration, thanks for doing it. One more ask: can you please also remove the entire old implementation as well? The check for transport(proxied-tcp) will shadow the old implementation anyway, so it's going to be just dead code. If anything wrong comes up with this implementation we would fix it instead of going back to the old one. |
Signed-off-by: Alex Becker <beckeralex@protonmail.com>
alexb271
force-pushed
the
proxied-socket
branch
from
12f472d
to
f042859
Compare
Thank you for the review! I have added commits to remove the proxied text server. It was used by the network driver originally, not the syslog driver, so I had to make a few tweaks in order to keep the network driver functioning correctly. |
Signed-off-by: Alex Becker <beckeralex@protonmail.com>
alexb271
force-pushed
the
proxied-socket
branch
from
f042859
to
850d2de
Compare
Signed-off-by: Alex Becker <beckeralex@protonmail.com>
alexb271
force-pushed
the
proxied-socket
branch
from
850d2de
to
f327435
Compare
Signed-off-by: Alex Becker <beckeralex@protonmail.com>
alexb271
force-pushed
the
proxied-socket
branch
from
f327435
to
708e701
Compare
| STATUS_AGAIN, | ||
| } Status; | ||
|
|
||
| struct proxy_hdr_v2 |
There was a problem hiding this comment.
The convention is to use the glib supplied version for primitives where applicable. Could you please change the uintN_ts to their glib equivalents (e.g, guintN)?
| gboolean result = FALSE; | ||
| msg_debug("PROXY header params", evt_tag_str("params", (const gchar *)msg)); | ||
|
|
||
| gchar **params = strsplit(params_str->str, ' ', 5); |
There was a problem hiding this comment.
[nitpick]
Could you please use g_strsplit() here?
|
|
||
| gchar **params = strsplit(params_str->str, ' ', 5); | ||
| gint params_n = g_strv_length(params); | ||
| if (params_n < 4) |
There was a problem hiding this comment.
Could you please give a name to this magic number?
| uint16_t len; /* number of following bytes part of the header */ | ||
| }; | ||
|
|
||
| union proxy_addr |
There was a problem hiding this comment.
Could you please also add a typedef to it so it is not necessary to use it with the union prefix?
Thank you for your reply! Just to avoid any misunderstandings, I did not write the PROXY implementation, my PR is a refactor to move proxy processing into the logtransport layer so that it can be enabled for the syslog source driver. Because of this, I do not know what the magic numbers stand for. However, I can try to address smaller issues like adding typedefs and such. |
|
Thanks for the clarification, I started the review by going through each commit and did not realize until now that it was a simple move (lib/logproto/logproto-proxied-text-server.c -> lib/transport/transport-proxied-socket.c). I will check the diff first and then go through the commits, and close my notes where I think it is not really relevant in this PR. Sorry for the confusion! I see that the CI is failing due to the light test case |
I've tried to replicate the TLS functionality in the new logtransport as it was in the |
@alexb271 would you still work on this or should/may I try to finish? |
I'm not actively troubleshooting this right now, so of course you would be welcome to investigate or finish it. If the solution turns out to be something very simple I've missed I can fix that too if required. |
I do not know if it's possible correctly,(or what is the simplest solution here), but could you please give me rights to this fork (it seems github allows forking of a fork only if I do not yet have my own fork of the same origin), I do not want to move this branch or create a new PR later on, the best would be if I could just simply edit here anything |
HofiOne
force-pushed
the
proxied-socket
branch
4 times, most recently
from
da79b47
to
349a052
Compare
Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
HofiOne
force-pushed
the
proxied-socket
branch
from
a9d5d0e
to
e34cfa2
Compare
tests/light/functional_tests/source_drivers/network_source/proxyprotocol/test_pp_network.py
Outdated
Show resolved
Hide resolved
| if (self->proxy) | ||
| { | ||
| log_transport_socket_proxy_free(self->proxy); | ||
| self->proxy = NULL; | ||
| } |
There was a problem hiding this comment.
I think the if is not necessary here as g_free will just simply return if the pointer is NULL.
There was a problem hiding this comment.
i think we cannot rely on the fact that the current impl of log_transport_socket_proxy_free is a simple g_free there might be any other cleanup code attached to it later that not neccesarily would be that smart as g_free
HofiOne
force-pushed
the
proxied-socket
branch
from
e34cfa2
to
eb20109
Compare
| if (self->tls_context) | ||
| else if (self->tls_context) | ||
| { | ||
| is_multi = TRUE; |
There was a problem hiding this comment.
This is not true, when self->super.create_multitransport == TRUE everything will be created as a multi_transport
There was a problem hiding this comment.
the variable name here is misleading, it should refer to if the transport must be switched in the proxy
i will rename it
Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
…the logproto proxied text server Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
…ansport and use regular and tls transports Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Alex Becker <beckeralex@protonmail.com> Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Hofi <hofione@gmail.com>
… class itself, not inherited from LogTransport anymore Only the network() source is tested and working yet, syslog() is a TODO Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Hofi <hofione@gmail.com>
…roxied' loggen parameter Signed-off-by: Hofi <hofione@gmail.com>
…e loggen with proper `proxied` switches Signed-off-by: Hofi <hofione@gmail.com>
…ce folder Signed-off-by: Hofi <hofione@gmail.com>
Signed-off-by: Hofi <hofione@gmail.com>
…the proxy and in its init Signed-off-by: Hofi <hofione@gmail.com>
HofiOne
force-pushed
the
proxied-socket
branch
from
d21b92c
to
73db4bd
Compare
| @@ -36,6 +36,10 @@ _do_transport_switch(MultiTransport *self, LogTransport *new_transport, const Tr | |||
| { | |||
| self->super.fd = log_transport_release_fd(self->active_transport); | |||
| self->super.cond = new_transport->cond; | |||
| /* At this point the proxy of the active transport must be released, and would be nice to handle the ownership passing here, but | |||
There was a problem hiding this comment.
Is this comment really necessary here?
Can this be moved to where the free is called and be mentioned that the Multi Transport does not support this?
(I find it not so elegant that the multi transport here refers to a specific implementation that is not it's dependency)
| { "PROXY TCP6 ::1 ::2 3333 4444\r\n", TRUE }, | ||
|
|
||
| /* INVALID PROTO */ | ||
| { "PROXY UNKNOWN\n", TRUE }, // WRONG TERMINATION BUT PERMISSIVE IMPLEMENTATION |
There was a problem hiding this comment.
Consider changing the the lines tagged as // WRONG TERMINATION BUT PERMISSIVE IMPLEMENTATION
/* INVALID PROTO */
....
/* INVALID PROTO - WRONG TERMINATION BUT PERMISSIVE IMPLEMENTATION */
{ "PROXY UNKNOWN\n", TRUE },
{ "PROXY TCP4 1.1.1.1 2.2.2.2 3333 4444\n", TRUE },
{ "PROXY UNKNOWN\r", TRUE },
{ "PROXY TCP4 1.1.1.1 2.2.2.2 3333 4444\r", TRUE },
To avoid duplicationg the // WRONG TERMINATION BUT PERMISSIVE IMPLEMENTATION comment 4 times
| { "PROXY TCP4 1.1.1.1 2.2.2.2 3333 4444\r\n", FALSE }, | ||
|
|
||
| /* INVALID ARGUMENTS - PERMISSIVE */ | ||
| { "PROXY TCP6 1.1.1.1 2.2.2.2 3333 4444\r\n", TRUE }, // WRONG IP PROTO |
There was a problem hiding this comment.
Same as above, avoid duplicating the comments as it makes it harder to copy
| STATUS_EOF, | ||
| STATUS_PARTIAL, | ||
| STATUS_AGAIN, | ||
| } Status; |
There was a problem hiding this comment.
Consider using enum specific prefix like PROXY_Status, with prefixes like: PROXY_STATUS_ERROR, to avoid accidental name colision if someone uses enum {} Status later
| } | ||
| g_assert_not_reached(); | ||
|
|
||
| case LPPTS_PROXY_V1_READ_LINE: | ||
|
|
||
| process_proxy_v1: |
There was a problem hiding this comment.
remove this label , since they are not used.
If removing the label causes readability issues, change the name of the LPPTS_PROXY_V1_READ_LINE macro.
| @@ -0,0 +1,117 @@ | |||
| #!/usr/bin/env python | |||
| ############################################################################# | |||
| # Copyright (c) 2020 One Identity | |||
There was a problem hiding this comment.
The correct copyright today is: Copyright (c) 2024 One Identity LLC.
| @@ -0,0 +1,119 @@ | |||
| #!/usr/bin/env python | |||
| ############################################################################# | |||
| # Copyright (c) 2020 One Identity | |||
There was a problem hiding this comment.
The correct copyright today is: Copyright (c) 2024 One Identity LLC.
| @@ -91,6 +91,9 @@ def __decode_start_parameters( | |||
| if proxied is True: | |||
| start_parameters.append("--proxied") | |||
|
|
|||
| if proxied == 1 or proxied == 2: | |||
There was a problem hiding this comment.
Add OneIdentity copyright to the file.
The correct copyright today is: Copyright (c) 2024 One Identity LLC.
| @@ -0,0 +1,56 @@ | |||
| #!/usr/bin/env python | |||
| ############################################################################# | |||
| # Copyright (c) 2015-2020 Balabit | |||
There was a problem hiding this comment.
Add OneIdentity copyright to the file.
The correct copyright today is: Copyright (c) 2024 One Identity LLC.
| @@ -47,6 +47,7 @@ | |||
| from src.syslog_ng_config.statements.sources.file_source import FileSource | |||
There was a problem hiding this comment.
Add OneIdentity copyright to the file.
The correct copyright today is: Copyright (c) 2024 One Identity LLC.
Resolves #4388
A new logtransport class for proxied socket has been implemented based on code from the proxied text server.
If everything is good then theoretically the new logtransport could allow deleting the proxied text server code in favor of configuring a regular text server with this new transport, however nothing has been deleted yet in this PR.