13 Dec
2018
13 Dec
'18
4:24 p.m.
#### 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
- [ ] Small bug fix (non-breaking change which fixes an issue)
- [x] 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 -->
- [ ] PR should be backported to stable branches
- [x] Tested changes locally
- [x] Related to issue #1757
#### Description
<!-- Describe your changes in detail -->
Note: This PR follows the discussion in #1757.
- introduce new global variable `ksr_tcp_accept_proxy`.
- this variable can be modified by using the `tcp_accept_proxy=yes`
configuration parameter.
- when active, inbound TCP connections are expected to behave according
to the PROXY protocol[1].
- Both the v1 (human-readable) and v2 (binary) versions of the protocol
are supported.
[1]: https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1765
-- Commit Summary --
* core: Add PROXY protocol implementation
-- File Changes --
M src/core/cfg.lex (3)
M src/core/cfg.y (9)
M src/core/globals.h (1)
M src/core/tcp_main.c (244)
M src/core/tcp_read.c (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1765.patch
https://github.com/kamailio/kamailio/pull/1765.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/1765
14 Dec
14 Dec
8:50 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
At the first sight, it looks good overall, just few changes that I would suggest:
* I think it would be better to name the new core parameter `ksr_tcp_accept_haproxy`
instead of `ksr_tcp_accept_proxy`. The `proxy` term is also used in SIP a lot (Kamailio
being a proxy itself), so I think using `haproxy` is more suggestive for the purpose of
the parameter.
* there are couple of local variables when parsing the PROXY header which are not
declared at the beginning of a block (or at the beginning of a function). Latest C
compilers do not complain about, but we prefer to stay a bit more strict compliant with
older specs in order to have it working on some embedded devices or old distros.
--
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/1765#issuecomment-447257718
9:06 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
@miconda the `proxy` protocol was first defined by haproxy but there may be other programs
that implement the protocol (not `haproxy` exclusive). i suggest `accept_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/1765#issuecomment-447261855
9:12 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
@lazedo - even implemented in other applications I still find that `tcp_accept_haproxy` is
more suggestive and directs to what it is supposed to do. It is where it started and the
specs are still hosted by haproxy.org.
--
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/1765#issuecomment-447263362
15 Dec
15 Dec
12:30 p.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
henningw commented on this pull request.
I Haven't tested it, but the parsing part looks like a solid and good implementation.
I have added one small remark in the code, and have some general questions:
- You work on a local pointer src_ip and dst_ip in the parsing part, but directly on the
c->rcv.src_port/c->rcv.dst_port - is there a particular reasons for it?
- related to error cases, what would happen if you e.g. set in the IPv6 case 0x21 the
dst_ip->af = AF_INET6, but later one run unto an error in the parsing. Then you would
return an error to the callee, and would go to the read_ip_info path. Here the AF_INET6
would be still be set, or I am wrong? What about just setting all the data into a
temporary variable, and then if the parsing was completed successfully, set it.
- why you set the c->rcv.bind_address=ba; in line 1221?
- if (likely(local_addr)){
- su2ip_addr(&c->rcv.dst_ip, local_addr);
- c->rcv.dst_port=su_getport(local_addr);
- }else if (ba){
- c->rcv.dst_ip=ba->address;
- c->rcv.dst_port=ba->port_no;
+ if (unlikely(ksr_tcp_accept_haproxy && state == S_CONN_ACCEPT)) {
+ ret = tcpconn_read_haproxy(c);
+
+ if (ret == -1) {
+ LM_ERR("invalid PROXY protocol header\n");
+ goto error;
+ } else if (ret == 1) {
+ LM_DBG("PROXY protocol did not override IP addresses\n");
+ goto read_ip_info;
+ }
Small suggestion, add a LM_DBG output here, that the tcpconn_read_haproxy call was
successful.
--
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/1765#pullrequestreview-185352625
17 Dec
17 Dec
9:26 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
@henningw - setting c->rcv.bind_address=ba was at line 990, so it has to still be done
somewhere.
--
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/1765#issuecomment-447776788
18 Dec
18 Dec
10:11 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
related to error cases, what would happen if you e.g.
set in the IPv6 case 0x21 the dst_ip->af = AF_INET6, but later one run unto an error in
the parsing. Then you would return an error to the callee, and would go to the
read_ip_info path. Here the AF_INET6 would be still be set, or I am wrong?
I believe you might be misreading the code here. I've added a comment to clarify
things.
There is only a single error case after the `0x21` case, and that's the inability to
read from the socket. In this case, we return `-1`, which in the calling function would
result in a `goto error;`, which terminates the connection. We only set the `*_ip->af`
after we've done most of the v2 parsing. Same thing is true for the v1 parsing: once
we get to the `TCP{4,6}` part of the line, only `0` or `-1` are possible outputs for the
function.
Your comment did help me discover that I was missing the `LOCAL` case (line 1075), and
mistakenly returned `0` in that case instead of `1`, so thanks for that.
You work on a local pointer src_ip and dst_ip in the
parsing part, but directly on the c->rcv.src_port/c->rcv.dst_port - is there a
particular reasons for it?
Convenience. There is only one pointer access for `c->rcv.src_port` and
`c->rcv.dst_port`, whereas there's two pointer accesses in
`c->rcv.src_ip->...`, and multiple fields are accessed in `{src,dst}_ip` (`af`,
`len`, `u`, etc). I'm happy to use a local pointer if requested.
--
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/1765#issuecomment-448167746
10:14 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
teotwaki commented on this pull request.
- if (likely(local_addr)){
- su2ip_addr(&c->rcv.dst_ip, local_addr);
- c->rcv.dst_port=su_getport(local_addr);
- }else if (ba){
- c->rcv.dst_ip=ba->address;
- c->rcv.dst_port=ba->port_no;
+ if (unlikely(ksr_tcp_accept_haproxy && state == S_CONN_ACCEPT)) {
+ ret = tcpconn_read_haproxy(c);
+
+ if (ret == -1) {
+ LM_ERR("invalid PROXY protocol header\n");
+ goto error;
+ } else if (ret == 1) {
+ LM_DBG("PROXY protocol did not override IP addresses\n");
+ goto read_ip_info;
+ }
There is an `LM_DBG()` indicating whether we received a PROXY v1 or PROXY v2 header. Do
you feel that a parsing-success message is strictly required here (as opposed to a lack of
parsing-failure message)?
--
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/1765#discussion_r242476737
11:09 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
From my point of view, it can be merged.
--
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/1765#issuecomment-448184626
19 Dec
19 Dec
8:03 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
Thank you for the comments and extensions. About this question:
related to error cases, what would happen if you e.g.
set in the IPv6 case 0x21 the dst_ip->af =
AF_INET6, but later one run unto an error in the parsing. Then you would return an error
to the
callee, and would go to the read_ip_info path. Here the AF_INET6 would be still be set,
or I am
wrong?
I probably should formulated it better. My more general questions would be: are there
error conditions that could occur during the proxy header parsing that could leave the
Kamailio core with some (socket) data from the proxy protocol, and some data from the
normal parsing? I am asking especially as this would be a classical border condition that
could lead to e.g. some security issue.
Otherwise the code also for me looks ready to merge. Please add some documentation about
this new core parameter in the wiki core cookbook
(https://www.kamailio.org/wiki/cookbooks/devel/core).
--
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/1765#issuecomment-448504071
8:05 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
Merged #1765 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/1765#event-2034979926
8:27 a.m.
New subject: [kamailio/kamailio] core: Add PROXY protocol implementation (#1765)
@henningw As requested:
https://www.kamailio.org/wiki/cookbooks/devel/core?do=diff&rev2%5B0%5D=…
With regards to the border condition that you mention, I don't believe there should be
such a case, unless weird things are done in the `goto error;` branch. I understand your
concern, however.
I will let you know when we've deployed this in production.
--
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/1765#issuecomment-448509871
2037
days inactive
2043
days old
11 comments
4 participants
participants (4)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
lazedo
-
Sebastian Lauwers