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.

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

ims_ipsec_pcscf: sec-agree implementation for IMS #1605

Merged
merged 1 commit into from Aug 2, 2018

Conversation

tdimitrov
Copy link
Contributor

Pre-Submission Checklist

  • Commit message has the format required by CONTRIBUTING guide
  • Commits are split per component (core, individual modules, libs, utils, ...)
  • Each component has a single commit (if not, squash them into one commit)
  • 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)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)

Checklist:

  • PR should be backported to stable branches
  • Tested changes locally
  • Related to issue #XXXX (replace XXXX with an open issue number)

Description

This is an implementation of sec-agree used in IMS with IPSec. It's not a complete sec-agree implementation, only the flows used by IMS. The code is usable, but needs a few improvements, which I plan to push in the near future. My work is based on the implementation in OpenIMSCore.

For IPSec implementation the XFRM framework from the Linux kernel is used. Security association (SA) and Policies creation/removal is performed via netlink messages. For this reason the module depends on libmnl (a minimalistic netlink library).

As XFRM is Linux specific, the code is not portable and can't be used on operating system different from Linux. The code will not compile on *BSDs too. However all platform specific code resides in ipsec.c so support for other OSes/IPSec implementations can be added relatively easy.

The README file, which is commited is generated from docs dir with xsltproc.

Issues I still work on:

  • Kamailio must be run as root in order to be able to send netlink messages and create XFRM SAs and Policies.
  • SAs and Policies are not deleted on Kamailio startup and shutdown.
  • According to the current contact implementation in the PCSCF modules (adn the 3GPP specs) the IPSec tunnel should be created on two steps. Initial parameters should be saved in security_tmp and on confirmation - in security. At the moment everything remains in security.

As this is my first more serious contribution to the project, all kinds of feedback is highly appreciated :)

@henningw
Copy link
Contributor

Hello,
thank you for your contribution! I did a quick look to the code and noticed a few simple areas that should refined before going looking deeper in the functionality.

  • Please remove the README file from the commit. This is auto-generated and should not be commited to git
  • You mentioned that your work is based on OpenIMScore. Do you actually use code from this project in your implementation? Then you need to add proper copyright notices that refer to this code.
  • With regards to copyright - you need to also choose a copyright to your code. Most modules choose "GPLv2 and later", but BSD would be also ok if you prefer this. Have a look to other modules for an example copyright header.

Best regards,
Henning

@tdimitrov
Copy link
Contributor Author

Hello Henning,

Thanks for your comments. I've removed the README, however, the copyright/licensing part is a bit complicated for me.

I have copied code from ims_registrar_pcscf for things like module structure and finding the contact in memory, etc.
I've used the implementation in OpenIMS mainly as a reference - what functions they expose to the routing logic and how they use internal Kamailio structures to redirect SIP messages.
The IPSec handling itself is written from scratch because they used shell scripts, instead of netlink sockets.

For there reasons I copied the Copyright notice from ims_registrar_pcscf (which is actually the same as in OpenIMSCore) and put it in the sources, based on other's work. Everything else is in GPLv2.

Do you feel this is correct or it's better to put OpenIMSCore's copyright everywhere?

In general, all I want is to contribute the code back to the project and not to abuse the licensing. I don't care if my name will stay anywhere or not.

Best regards,
Tsvetomir

@miconda
Copy link
Member

miconda commented Aug 2, 2018

@tdimitrov - going to merge it and then add you to the dev team so you can maintain the new module.

@henningw - for the future, when a new module is added, the README must be committed, either by the developer in the initial patch or by someone else after the merge. I will do it after the merge. For safety, the automatic script is not adding new files, only updates exiting ones -- maybe it can be enhanced, but initially I thought that running git add can create troubles if some files are generated or relocated by mistake.

@miconda miconda merged commit e46ef10 into kamailio:master Aug 2, 2018
@henningw
Copy link
Contributor

henningw commented Aug 2, 2018

Tsvetomir - thank you for the clarification about the licensing. If the code in question is GPLv2, then all is fine if you keep it GPLv2 also in your license for new files. Regarding the copyright notices you choose the right approach, add the notice from the "original" file to your file if you add some parts from it.

Thank you Daniel for the clarification - I did not know this about the README for new modules.

@tdimitrov tdimitrov deleted the ipsec branch August 2, 2018 10:47
@tdimitrov
Copy link
Contributor Author

@miconda @henningw Thank you!

@tdimitrov tdimitrov restored the ipsec branch August 10, 2018 14:13
@tdimitrov tdimitrov deleted the ipsec branch August 10, 2018 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants