[kamailio/kamailio] Discrepancies in migration of commits from herlesupreeth/kamailio to upstream (Issue #3772)
AFAIK the commits from Herle Supreeth from its fork https://github.com/herlesupreeth/kamailio were migrated manually (not via PR) to upstream at the end of last year. While reviewing the migration I have found two discrepancies:
1. https://github.com/kamailio/kamailio/commit/8b9a2977e111d9adb8595d98ab59f8c8... vs. https://github.com/kamailio/kamailio/commit/a6a3bd088368fbf65c283ae27e999d31...
``` herlesupreeth/kamailio (fork) commit 8b9a2977e111d9adb8595d98ab59f8c8eb033120 Author: herlesupreeth herlesupreeth@gmail.com Date: Mon Jun 7 14:01:54 2021 +0200
IPSec fixes
diff --git a/src/modules/ims_ipsec_pcscf/cmd.c b/src/modules/ims_ipsec_pcscf/cmd.c index 8e0cabcb80..9de3cf7331 100644 --- a/src/modules/ims_ipsec_pcscf/cmd.c +++ b/src/modules/ims_ipsec_pcscf/cmd.c @@ -937,7 +939,14 @@ int ipsec_forward(struct sip_msg* m, udomain_t* d) dst_port = s->port_us; }
- int buf_len = snprintf(buf, sizeof(buf) - 1, "sip:%.*s:%d", ci.via_host.len, ci.via_host.s, dst_port); + int buf_len = 0; + if (dst_proto == PROTO_TCP) { + buf_len = snprintf(buf, sizeof(buf) - 1, "sip:%.*s:%d;transport=tcp", ci.via_host.len, ci.via_host.s, dst_port); + } else if (dst_proto == PROTO_TLS) { + buf_len = snprintf(buf, sizeof(buf) - 1, "sip:%.*s:%d;transport=tls", ci.via_host.len, ci.via_host.s, dst_port); + } else { + buf_len = snprintf(buf, sizeof(buf) - 1, "sip:%.*s:%d", ci.via_host.len, ci.via_host.s, dst_port); + }
if((m->dst_uri.s = pkg_malloc(buf_len + 1)) == NULL) { LM_ERR("Error allocating memory for dst_uri\n"); ```
vs.
``` kamailio/kamailio (upstream) commit a6a3bd088368fbf65c283ae27e999d315db0844b Author: Daniel-Constantin Mierla miconda@gmail.com Date: Wed Jun 1 18:11:22 2022 +0200
ims_ipsec_pcscf: new option for ipsec_forward() to set trasport for tcp dst uri
diff --git a/src/modules/ims_ipsec_pcscf/cmd.c b/src/modules/ims_ipsec_pcscf/cmd.c index 5aa208d334..6f6c15e21b 100644 --- a/src/modules/ims_ipsec_pcscf/cmd.c +++ b/src/modules/ims_ipsec_pcscf/cmd.c @@ -947,8 +949,14 @@ int ipsec_forward(struct sip_msg *m, udomain_t *d, int _cflags)
if(!(_cflags & IPSEC_NODSTURI_RESET)) { char buf[1024]; - int buf_len = snprintf(buf, sizeof(buf) - 1, "sip:%.*s:%d", ci.via_host.len, - ci.via_host.s, dst_port); + int buf_len; + if((_cflags & IPSEC_SETDSTURI_FULL) && (dst_proto == PROTO_TCP)) { + buf_len = snprintf(buf, sizeof(buf) - 1, "sip:%.*s:%d;transport=tcp", + ci.via_host.len, ci.via_host.s, dst_port); + } else { + buf_len = snprintf(buf, sizeof(buf) - 1, "sip:%.*s:%d", ci.via_host.len, + ci.via_host.s, dst_port); + }
if((m->dst_uri.s = pkg_malloc(buf_len + 1)) == NULL) { LM_ERR("Error allocating memory for dst_uri\n"); ```
To me it seems the handling of the case `dst_proto == PROTO_TLS` is missing upstream.
2. The commit https://github.com/kamailio/kamailio/commit/bfb0a17f3998d26991b4da7a8e83b8c7... seems to be absent in upstream.
(From my point of view all the other commits are migrated fine, thanks for the good work!)
Thanks @axelsommerfeldt for taking time to compare the changes.
For point 1, I think its fine because I have not seen any COTS UE using TLS but for completeness, yes we can add it
For point 2, you are right that the part in fill_contact() are missing but regarding the changes under ipsec_forward() I am not sure whether they are relevant since that function got changed in several commits.
For point 2, you are right that the part in fill_contact() are missing
I had another look at the code and i think the changes may not be needed since the contact information would definitely be present as the below code in kamailio master ensures it
``` memset(&tmsg, 0, sizeof(sip_msg_t)); tmsg.buf = tbuf; memcpy(tmsg.buf, t->uas.request->buf, t->uas.request->len); tmsg.buf[t->uas.request->len] = '\0'; tmsg.len = t->uas.request->len; if(parse_msg(tmsg.buf, tmsg.len, &tmsg) != 0) { LM_ERR("buffer parsing failed!"); goto error; } ```
Thanks for the great analysis guys. It looks that its now fine, maybe it can be closed then. If I misunderstood it, just create a PR with the necessary change.
For point 1, I think its fine because I have not seen any COTS UE using TLS but for completeness, yes we can add it
I just opened a PR as suggested by [henningw](https://github.com/henningw).
Thank you @axelsommerfeldt
Closed #3772 as completed.
PR was merged.
-
Axel Sommerfeldt -
Daniel-Constantin Mierla
-
Henning Westerholt
-
Supreeth Herle