19 Oct
2021
19 Oct
'21
3:01 p.m.
<!-- Kamailio Pull Request Template -->
<!--
IMPORTANT:
- for detailed contributing guidelines, read:
https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md
- pull requests must be done to master branch, unless they are backports
of fixes from master branch to a stable branch
- backports to stable branches must be done with 'git cherry-pick -x
...'
- code is contributed under BSD for core and main components (tm, sl, auth, tls)
- code is contributed GPLv2 or a compatible license for the other components
- GPL code is contributed with OpenSSL licensing exception
-->
#### Pre-Submission Checklist
<!-- Go over all points below, and after creating the PR, tick all the checkboxes
that apply -->
<!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines
from above-->
<!-- If you're unsure about any of these, don't hesitate to ask on
sr-dev mailing list -->
- [X] Commit message has the format required by CONTRIBUTING guide
- [X] Commits are split per component (core, individual modules, libs, utils, ...)
- [X] Each component has a single commit (if not, squash them into one commit)
- [X] No commits to README files for modules (changes must be done to docbook files
in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change
- [X] Small bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds new functionality)
- [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist:
<!-- Go over all points below, and after creating the PR, tick the checkboxes that
apply -->
- [X] PR should be backported to stable branches
- [X] Tested changes locally
- [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
<!-- Describe your changes in detail -->
As I reported to developers' mailing list
(https://lists.kamailio.org/pipermail/sr-dev/2021-October/064243.html) if support for
proxy protocol is enabled, kamailio is not able to match a tcp DEST_IP/DEST_PORT to an
alias of an already existing tcp connection.
This beacause the (local) destination ip on which the connection was initiated is
overwritten by the destination IP and port carried by the proxy protocol header. Since the
original connection info is saved when parsing the ortocol we can use that information to
build a new alias and to use it to match outgoing tcp connection later.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2888
-- Commit Summary --
* <a
href="https://github.com/kamailio/kamailio/pull/2888/commits/3bd59bb17…quot;>core:
tcp - add alias for cinfo dst IP</a>
-- File Changes --
M src/core/tcp_main.c (11)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2888.patch
https://github.com/kamailio/kamailio/pull/2888.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888
19 Oct
19 Oct
5:28 p.m.
@phildeb approved this pull request.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#pullrequestreview-783449347
5:57 p.m.
Thanks for the pull request. You probably considered that, but would it not make sense to
only add the additional two aliases if the proxy protocol ("tcp_accept_haproxy")
is actually enabled?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-946865996
20 Oct
20 Oct
6:14 a.m.
Hi Henning, that's a good question. Currently the cinfo struct of the tcp connection
is only filled by the proxy protocol parsing (oterwise is init to null) so the check when
adding the alias gives the same results (we also chek that the dst IP there is different
from the one stored in rcv, to not have duplicate aliases). I've chosen this approach
in case that, for other reasons, in the future, we have cinfo != rcv in the connection
struct.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-947313426
10:52 a.m.
Thanks for the reply, then it should be equivalent. I would suggest to add a comment in
this regards to the new logic, to make it clear for eventual future changes. Lets wait a
few more days for other comments before merging.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-947461601
11:11 a.m.
@grumvalski pushed 1 commit.
86d897534d5c814de1f87744f8d5d6ca6a35f384 core: tcp - add alias for cinfo dst IP
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2888/files/3bd59bb17a575a4d369799…
11:11 a.m.
Right, I've added a comment and force pushed.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-947476905
1:30 p.m.
Hello Federico @grumvalski
I tried before use `haproxy` protocol and found Kamailio implementation is very
restrictive.
As example
1) CPU 100% usage when created TCP connection and data do not send #2658
2) no ability to define a list of trusted sources, because now any fraud host can send
crafted haproxy packet and break ACL rules used on Kamailio side. Relevant feature
`set_real_ip_from` exist in nginx
([Link](https://nginx.org/en/docs/http/ngx_http_realip_module.html#set_real_i…).
From my point of view it is a big security hole.
Could you also look for limitations described above.
Just for info, nginx config snippet with haproxy feature.
```
server {
listen 0.0.0.0:3128 proxy_protocol;
listen [::]:3128 proxy_protocol;
set_real_ip_from 4.101.84.5/32;
set_real_ip_from 4.101.84.133/32;
set_real_ip_from 4.236.25.5/32;
set_real_ip_from 4.236.25.133/32;
real_ip_header proxy_protocol;
...
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-947575963
3:31 p.m.
Thanks for the update @grumvalski. @sergey-safarov - lets not start an (unrelatated)
discussion in this pull request. Use the sr-dev list instead, there is already the
"Proxy protocol issue" thread, or start a new one.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-947670544
21 Oct
21 Oct
11:44 a.m.
Looks ok for me, I'm fine to merge it if no other comments.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-948439219
5:33 p.m.
Merged #2888 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#event-5499406027
22 Oct
22 Oct
8:08 a.m.
Thanks for merging Henning. Should we backport it also to stable branches?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-949314462
3:50 p.m.
@grumvalski: if it does not work without it, then you can backport - you have commit
access.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2888#issuecomment-949650352
931
days inactive
934
days old
12 comments
5 participants
participants (5)
-
Daniel-Constantin Mierla -
Federico Cabiddu
-
Henning Westerholt
-
Philippe
-
sergey-safarov