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

Jump to bottom

[HAPROXY] use $SOURCEIP instead of $PROXIED_SRCIP #361

Merged
merged 24 commits into from
Feb 7, 2025
Merged

[HAPROXY] use $SOURCEIP instead of $PROXIED_SRCIP #361

merged 24 commits into from
Feb 7, 2025

Conversation

bazsi
Copy link
Member

@bazsi bazsi commented Nov 2, 2024 •
edited
Loading

This is a large refactor of the HAProxy support, and preparations for protocol auto detection. It also changes the HAProxy support to use the standard source/destination addresses in LogMessage, instead of a proxy specific values e.g. $SOURCEIP instead of $PROXIED_SRCIP.

Short summary of the patches:

  • small refactors and cleanups (first 5 patches)
  • simplify TLS compression setup code, previously this crossed a lot of layers, whereas it can be a lot simpler
  • LogTransportTLS is becoming an adapter, e.g. instead of going directly to file descriptors, go through another layer of the LogTransport interface. This prepares for protocol auto detection.
  • introduces LogTransportStack level aux data, making it possible to enrich messages from the any of the Transports
  • adds the $SOURCEPORT macro
  • LogTransportHAProxy is changed so it invokes a log_transport_stack_switch() instead of doing this internally
  • has a big refactor of TransportMapperInet, as its various settings and options were becoming too difficult to read. This patch also adds a large light testcase to cover all possible transport() cases
  • contains an alternative fix for afsocket/transport-mapper: fix auto transport with tls #482 (although pretty similar)
  • and few smaller patches

@bazsi bazsi force-pushed the haproxy-use-sourceip-instead-of-proxied-srcip branch 2 times, most recently from cdfd3b6 to c2c1e8f Compare November 2, 2024 21:09
@bazsi bazsi mentioned this pull request Nov 8, 2024
Merged
@bazsi bazsi force-pushed the haproxy-use-sourceip-instead-of-proxied-srcip branch 4 times, most recently from 58f020f to dff7494 Compare November 11, 2024 19:51
@bazsi bazsi force-pushed the haproxy-use-sourceip-instead-of-proxied-srcip branch 2 times, most recently from 672e0c2 to 1c9e896 Compare December 3, 2024 17:30
@bazsi bazsi force-pushed the haproxy-use-sourceip-instead-of-proxied-srcip branch from 1c9e896 to 5ee3ffd Compare January 27, 2025 08:55
@bazsi bazsi mentioned this pull request Jan 30, 2025
Closed
@bazsi bazsi force-pushed the haproxy-use-sourceip-instead-of-proxied-srcip branch 6 times, most recently from b3b5bc7 to e8322f7 Compare February 2, 2025 16:21
@bazsi bazsi requested a review from MrAnno February 2, 2025 17:04
@bazsi
Copy link
Member Author

bazsi commented Feb 2, 2025

This is now ready for review and I think would be a better solution than #482 with the same fix.

@bazsi
Copy link
Member Author

bazsi commented Feb 3, 2025

@OverOrion @MrAnno could you re-review this, so we can merge this instead of #482?

@MrAnno MrAnno requested a review from OverOrion February 3, 2025 12:46
@MrAnno
Copy link
Member

MrAnno commented Feb 3, 2025

I'm reviewing it.

OverOrion
OverOrion reviewed Feb 3, 2025
View reviewed changes
lib/transport/transport-tls.c Outdated Show resolved Hide resolved
lib/transport/transport-tls.c Show resolved Hide resolved
lib/transport/transport-aux-data.h Show resolved Hide resolved
modules/afsocket/transport-mapper-inet.h Show resolved Hide resolved
modules/afsocket/transport-mapper-inet.c Show resolved Hide resolved
@MrAnno MrAnno self-requested a review February 3, 2025 16:21
bazsi added 5 commits February 4, 2025 13:08
@bazsi
light: allow the use of "auto" transport for syslog() source drivers
a56dc57 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
logproto: rename prepare() methods to poll_prepare
a9054df 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-stack: add poll_prepare() methods for LogTransport and LogT…
b2c5cc7 
…ransportStack

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-stack: add LogTransportStack level read/write/writev methods
e3eae2a 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-adapter: add free_method
97b8259 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
bazsi added 17 commits February 4, 2025 13:08
@bazsi
transport/tls-context: simplify compression setup
5fa97df 
Previously TLS compression was enabled using an overly complicated mechanism
crossing a number of layers (TransportMapperInet -> TransportFactoryTLS ->
TLSSession -> SSL). This can be a lot simpler, which this patch
implements.

NOTE: compression will not work in most cases due to OpenSSL security
levels and this patch adds a warning about it.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-tls: convert to a LogTransportAdapter
2b3296a 
Instead of going to the fd directly, wrap the lower-level LogTransport
instance into a BIO and use that. This implements proper stacking
for LogTransportTLS.

This adds the use of OpenSSL BIOs to wrap the lower level LogTransport
instance.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-aux-data: add log_transport_aux_data_move()
8aeb9ff 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-stack: make it possible to add aux data from the LogTranspo…
919bccd 
…rtStack level

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
logproto: call transport-stack level I/O methods
aebabff 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
logproto/logproto-auto-server: remove RFC6587 references from log mes…
f0704b0 
…sages

The "auto" protocol can be applied to both syslog() and network(), so
it's not strictly RFC6587 related and it does not add too much information
anyway.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
templates: add $SOURCEPORT macro
02e1fd9 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-haproxy: use the normal $SOURCEIP, $DESTIP, $DESTPORT macros
bc95df8 
Instead of using proxy protocol specific name value pairs, set the
addresses in the message's saddr/daddr members.

This should be a lot faster and a lot easier to use.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-haproxy: use the LogTransportStack->aux to save src/dst add…
dd9f318 
…resses

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-haproxy: implement transport switch instead of changing bas…
b4f0552 
…e_index

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport-haproxy: add more information to the proxy detection failur…
d038370 
…e message

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
afsocket: refactor TransportMapperInet transport setup logic
7511b97 
This reworks the various boolean members in TransportMapperInet that
control which logproto/transport we apply to a specific connection.

With these renames, it's much easier to follow what happens and why.

NOTE: there's a followup bugfix that fixes the same bug as axoflow#482.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
afsocket: transport(auto) with tls() should immediately start using TLS
b52ecdb 
"auto" has originally been planned to auto-detect TLS as well as framing
format, but at this point it does not do TLS auto-detection.

But this means that transport(auto) with tls() options set will start reading
data without SSL, e.g. the encrypted stuff will make it into the
messages received.

This patch fixes that for both the syslog() and the network() driver. The
only change is that delegate_tls_start_to_logproto is FALSE for the "auto"
case. This will be changed once the TLS auto detection feature is also
in.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport/transport-factory-haproxy: use factory for creating HAProxy…
96cc1f1 
… transports

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
light: generalize test_pp_network and test_pp_syslog test cases
cc9260a 
Instead of just exercising the proxyprotocol try all valid transports, including
the "auto" variants.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
light: fix up files included in the dist
0a44754 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
news: added news file
6344ea8 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi bazsi force-pushed the haproxy-use-sourceip-instead-of-proxied-srcip branch from e8322f7 to 6344ea8 Compare February 4, 2025 12:22
MrAnno
MrAnno reviewed Feb 4, 2025
View reviewed changes
Copy link
Member

@MrAnno MrAnno left a comment

Choose a reason for hiding this comment

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

I've started manual-testing the generic- and TLS-related transport changes (I think we need thorough testing since this touches core functionality).

lib/transport/transport-tls.c Outdated Show resolved Hide resolved
lib/transport/transport-tls.c Show resolved Hide resolved
@MrAnno
Copy link
Member

MrAnno commented Feb 5, 2025

I've tested the TLS-related changes on a real network connection through multiple hops, it works as expected.

bazsi added 2 commits February 6, 2025 14:04
@bazsi
transport: remove unused files
d4a7589 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi
transport: free LogTransportBIO BIO_meth instance at shutdown
704ded6 
This was a one-off allocation, but it's better if it is freed.

Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi bazsi force-pushed the haproxy-use-sourceip-instead-of-proxied-srcip branch from d05bb69 to 704ded6 Compare February 6, 2025 13:29
@bazsi
Copy link
Member Author

bazsi commented Feb 6, 2025

@MrAnno I've resolved the memory leak and did not add BIO_set_nbio() call and resolved your comments. Let me know if you agree. Thanks

@MrAnno MrAnno merged commit fe2fdf9 into axoflow:main Feb 7, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OverOrion OverOrion OverOrion left review comments

@MrAnno MrAnno MrAnno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bazsi @MrAnno @OverOrion