Add support for PROXY protocol v2 #4211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
No news file has been detected. Please write one, if applicable. |
|
Build FAILURE |
bazsi
force-pushed
the
proxy-protocol-v2
branch
from
8271aa8
to
f0d5b8f
Compare
|
Do we want to update Light testcases for Proxy protocol under: If it helps I can work on it in my free time. |
this would be great. This C program generates the required binary payload. You don't have to use this program in the tests, rather just copy-paste the output into the testcase. |
|
Thanks, the result was the following: $ ./syslog-ng-proxyprotocolv2
QUIT
!
�5��foo bar bazI will test this as an input with the PR and if everything is fine I will add new testcases for Proxy Protocol v2. |
|
The output from strace |
|
I will also read the related latest documentation: |
|
I have checked the input with syslog-ng and I have got the following result, should this be the expected? It seems correct for me. (input message was: "\r\n\r\n\0\r\nQUIT\n!\21\0\f\1\2\3\4\4\3\2\1\2025\255\234foo bar baz") [2022-11-18T08:46:27.182514] Initializing PROXY protocol source driver; driver='0x55844ef80320'
[2022-11-18T08:46:27.182562] Syslog connection accepted; fd='13', client='AF_INET(127.0.0.1:34058)', local='AF_INET(127.0.0.1:30001)'
[2022-11-18T08:46:27.182695] PROXY protocol header received; version='2', header='<binary_data>'
[2022-11-18T08:46:27.182706] PROXY protocol header parsed successfully;
[2022-11-18T08:46:27.182829] EOF occurred while reading; fd='13'
[2022-11-18T08:46:27.182860] Incoming log entry; input='�foo bar baz', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182868] Setting value; name='MESSAGE', value='�foo bar baz', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182897] Setting value; name='PROXIED_SRCIP', value='1.2.3.4', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182902] Setting value; name='PROXIED_DSTIP', value='4.3.2.1', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182906] Setting value; name='PROXIED_SRCPORT', value='49794', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182923] Setting value; name='PROXIED_DSTPORT', value='13762', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182926] Setting value; name='PROXIED_IP_VERSION', value='4', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182933] >>>>>> Source side message processing begin; instance='proxied-tcp,127.0.0.1', location='syslog_ng_server.conf:4:5', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182963] Setting value; name='HOST_FROM', value='localhost', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182970] Setting value; name='HOST', value='localhost', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.182974] Setting tag; name='.source.source_227548333628901111505367134784799626167', value='1', msg='0x7f480c004000'
[2022-11-18T08:46:27.182979] Setting value; name='SOURCE', value='source_227548333628901111505367134784799626167', type='string', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.183006] Initializing destination file writer; template='output.log', filename='output.log', symlink_as='(null)'
[2022-11-18T08:46:27.183047] affile_open_file; path='output.log', fd='16'
[2022-11-18T08:46:27.183072] <<<<<< Source side message processing finish; instance='proxied-tcp,127.0.0.1', location='syslog_ng_server.conf:4:5', msg='0x7f480c004000', rcptid='1'
[2022-11-18T08:46:27.183088] Syslog connection closed; fd='13', client='AF_INET(127.0.0.1:34058)', local='AF_INET(127.0.0.1:30001)'
[2022-11-18T08:46:27.183095] Freeing PROXY protocol source driver; driver='0x55844ef80320'
[2022-11-18T08:46:27.183125] Closing log transport fd; fd='13'
[2022-11-18T08:46:27.183164] Outgoing message; message='1.2.3.4 4.3.2.1 49794 13762 4 �foo bar baz\x0a'
[2022-11-18T08:46:27.183180] Window size adjustment; old_window_size='99', window_size_increment='1', suspended_before_increment='FALSE', last_ack_type_is_suspended='FALSE'
[2022-11-18T08:46:27.212493] Running application hooks; hook='3'
[2022-11-18T08:46:27.212510] syslog-ng shutting down; version='3.38.1.209.g93434d7'
[2022-11-18T08:46:27.212564] EOF on control channel, closing connection;
[2022-11-18T08:46:27.312792] Closing log transport fd; fd='16' |
|
In the output: "Outgoing message; message='1.2.3.4 4.3.2.1 49794 13762 4 �foo bar baz\x0a'" |
I don't have that binary character in my local environment, but it's definitely strange. I have now manually rechecked the code and also run strace on syslog-ng how it consumed the bytes from the test program, and it seems to invoke these recvfrom() calls early on: 579817 recvfrom(13, "\r\n\r\n\0", 5, 0, NULL, NULL) = 5 So it does not have the binary character in front of "foo". It's not even part of the "Incoming log entry" message: |
|
Thanks, I will re-check my environment, and test input again. |
|
Unfortunately it still did not work for me. Input message: I send input with loggen: /install/bin/loggen --inet --stream --dont-parse --read-file=loggen_input_147930440155197378701184596055398432048.txt --permanent localhost 30002How syslog-ng process the input: recvfrom(13, "\r\n\r\n\0", 5, 0, NULL, NULL) = 5 <0.000045>
recvfrom(13, "\r\nQUIT\n!\21\0\f", 11, 0, NULL, NULL) = 11 <0.000006>
recvfrom(13, "\1\2\3\4\4\3\2\1\302\2025\302", 12, 0, NULL, NULL) = 12 <0.000006>
recvfrom(13, "\255\302\234foo bar baz", 65536, 0, NULL, NULL) = 14 <0.000007>
recvfrom(13, "", 65522, 0, NULL, NULL) = 0 <0.000007>Syslog-ng's console output: This is my last commit id: 93434d7 |
|
I assume that the input in the (loggen) file is not correct. It has written via Python from Light. I will check this part. |
|
I think the input is malformed somehow. There is a strange '\302' character. I will check this later |
|
If I replace the loggen input with the original output of the given c script (which I redirected into a file) everything is fine. I could use this written file in Light testcases. So I could go forward with the testcase updates. |
|
While I am working on Light testcases of proxy protocol v2 I have found that our and Light testcases uses My use-case would be the following:
(if the above would work, I assume that I could drop the external input file generation, loggen would do everything for me, am I right?) /install/bin/loggen --use-ssl --dont-parse --read-file=/source/tests/light/reports/2022-11-24-06-08-05-625062/test_pp_tls_passthrough_pp_v2_/message0_input --proxied-tls-passthrough --debug --number=1 localhost 30002My source config for syslog-ng Thanks. |
|
Hi,
On Thu, Nov 24, 2022, 07:18 Andras Mitzki ***@***.***> wrote:
Should the actual implementation work for SSL case also?
Yes, the implementation is right next to the existing one, e.g. all
transport options should work the same.
While I am working on Light testcases of proxy protocol v2 I have found
that our loggen has an inner implementation for
https://github.com/syslog-ng/syslog-ng/blob/bc48493db0fc8b6eba87bf2f146ac68fc14c18f9/tests/loggen/ssl_plugin/ssl_plugin.c#L328
I'll check that out if I can add a client implementation there.
Light testcases uses proxied_tls_passthrough parameter of loggen, which for
… now always generates a v1 header for my inputs.
My use-case would be the following:
- generating a fille which contains a proper v2 header and a payload
- send this file with loggen to syslog-ng with the following
parameters (this works well for me when SSL is not used):
/install/bin/loggen --use-ssl --dont-parse --read-file=/source/tests/light/reports/2022-11-24-06-08-05-625062/test_pp_tls_passthrough_pp_v2_/message0_input --proxied-tls-passthrough --debug --number=1 localhost 30002
My source config for syslog-ng
source source_67674173725322721703918758720504068388 {
network (
ip(localhost)
port(30002)
transport("proxied-tls-passthrough")
flags(no-parse)
tls(
key-file(/source/tests/light/reports/2022-11-24-06-08-05-625062/test_pp_tls_passthrough_pp_v2_/server.key)
cert-file(/source/tests/light/reports/2022-11-24-06-08-05-625062/test_pp_tls_passthrough_pp_v2_/server.crt)
peer-verify("optional-untrusted")
)
);
};
Thanks.
—
Reply to this email directly, view it on GitHub
<#4211 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFOK5TOSOF4ENB6BDSOFQLWJ4CD7ANCNFSM6AAAAAAR7EVOPE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
bazsi
force-pushed
the
proxy-protocol-v2
branch
from
93434d7
to
5062bdc
Compare
|
I have now updated loggen to support proxy v2 headers using the -H command line argument, e.g. -H 2 is how you request v2 proxy headers. $ loggen --inet -H 2 --proxy-dst-ip 9.8.7.6 --proxy-src-ip 5.6.7.8 --proxy-src-port 9999 --proxy-dst-port 8888 127.0.0.1 2000 |
bazsi
changed the title
|
Thanks for the loggen update. |
mitzkia
reviewed
Dec 2, 2022
|
I have tested syslog-ng and loggen behaviour for PROXY protocols and I have some findings. I have created a separate branch with new Light testcases (commit)(the branch currently is in progress), but it could reproduce the issues. Finding-1 (this could be an issue):
syslog-ng config for TLS mode: loggen start: loggen strace output (for sendto): syslog-ng strace output (for recvfrom): syslog-ng console log: Finding-2 (maybe this is only a minor issue):
loggen start: syslog-ng console log: syslog-ng config for TLS mode: loggen start: syslog-ng console log: |
bazsi
force-pushed
the
proxy-protocol-v2
branch
from
5062bdc
to
c7349eb
Compare
|
There was an issue that loggen sent the PROXY header twice in --proxied-tls-passthrough mode, which I have now fixed. But honestly, there's a BIG layering violation inside loggen, as the normal proxy header (and not the passthrough one) is generated as the part of the message stream, causing counters to be off by one. I could add a hack to remove this from the counts, but that would just complicate stuff, the real solution would be to move the sending of the proxy header outside of the message stream. loggen would definitely need some love, there's a lot of duplication in ssl_plugin/socket_plugin that could be shared and this is also an architectural problem. I made an attempt to at least fix this, but it's a larger scale change, unfortunately. This is not a regression, both the duplication of the proxy header and the counting of messages have been there before I've added proxy v2 support. So I would just merge this and get back to these issues at a later point. |
|
Ok, thanks for the change and the clarification. |
I have now added a simple unit test case to cover the v2 parsing code, so I think we can split off the light tests. These days as review capacity is pretty limited, I think it is easier to manage smaller PRs. |
|
@kira-syslogng retest this please |
bazsi
force-pushed
the
proxy-protocol-v2
branch
from
9a6bf07
to
c641f2e
Compare
MrAnno
reviewed
Mar 2, 2023
|
I'm investigating a conditional jump/move on uninitialized value reported by valgrind: |
|
It's an unrelated issue with a filter. LGTM otherwise. |
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
…tate machine Since I am about to add support for the PROXYv2 protocol, I need to be a bit more flexible how we fetch the proxy header. Refactor the fetch function to be a state machine, which can later be extended. There's one change functionality wise: one previously accepted case, where we only had a CR character without an NL is now rejected, instead of accepted. Earlier, the only reason it worked was that the unit test contained an EOF right after the CR character, causing the proxy header processing logic to process the PROXY line, even though there was no NL character. The current change however would return LPS_EOF as we receive it, so the PROXY line is not processed. This patch contains a change in the unit test to reflect the new reality. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
…roxiedTextServer Instead of dynamically allocating this structure, just embed it into the larger one. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
…eader from the buffer Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Instead of scattering the literal value of 5 in the code. Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
… of 16 Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
|
dropped the read_chunk function. |
MrAnno
approved these changes
Mar 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for PROXY protocol v2 as requested in #4205
@g-psantos I have tested this blindly, e.g. I wrote my own test program based on the specification, so it might not work in an actual environment, would be great if you could do some manual testing.
NOTE: This does adds some improvement to the existing code, but I feel PROXY v1 should have been done very differently. Instead of a LogProto implementation that combines the PROXY protocol and RFC3164 -like transport over TCP, this should have been a LogTransport implementation that can be combined with any LogProto instances easily. I did NOT address this architectural problem with this PR. I am also not sure that the key-value pairs we are adding to the message (e.g. PROXIED_SRCIP) are the right choice when handling this kind of transport protocol. But I didn't want to extend the scope of this PR too much. This potentially addresses an imminent use-case, hopefully solving it, so I didn't want to complicate things further. I have other things on my plate too :)
@p-gsantos let me know if this indeed solves your use-case. Thanks.