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

Part 2 of 2 - EAP-RADIUS ipsec/strongswan support #1342

Closed
wants to merge 3 commits into from
Closed

Part 2 of 2 - EAP-RADIUS ipsec/strongswan support #1342

wants to merge 3 commits into from

Conversation

GurliGebis
Copy link
Contributor

This pull request is split across the tools and core repositories, this is the core part.

  1. commit improves readability of the generated strongswan.conf by fixing indentions, and ipsec.conf by adding some missing spaces.
  2. commit implements the config file generation parts that is missing.
  3. commit implements the ui part, adding two fields for entering the RADIUS server and secret.

@fichtner
Copy link
Member

I merged the tools part, but still waiting for a final merge in a 17.1.x and feedback from Ad on this. :)

@GurliGebis
Copy link
Contributor Author

Good :) It has been years since I have touched PHP, so it might need a few changes here and there.

I have attached the part of my config here, for people to test: https://forum.opnsense.org/index.php?topic=4323.0

@AdSchellevis AdSchellevis self-assigned this Jan 23, 2017
@AdSchellevis
Copy link
Member

@GurliGebis I will handle this one next week.
I have one small question, does this patch also add support for accounting, or is there more work needed for that? Our pluggable auth framework provides support for radius authentication, but doesn't provide accounting.

@GurliGebis
Copy link
Contributor Author

@AdSchellevis Great :)
All this patch does is make sure the right thing is added to the strongswan.conf and ipsec.conf for EAP-RADIUS.

The only parts added to ipsec.conf is the plugins/eap-radius part, with the "server" and "secret".
From what I know, that should make EAP-RADIUS use it for both auth and acc, but I haven't tested, since I have never had the need for accounting.

As for the RADIUS part in the pluggable auth framework, I haven't touched that.
I think that later on, it might be an idea to replace the two text boxes in my patch with a dropdown, where you can select multiple RADIUS servers defined in the auth framework.
But my plan with this is to get this up and running first, and then replace it later with the dropdown selecting one or more radius servers from the auth framework.

Does it make sense to go that route, or should I rework this to use the auth framework for selecting RADIUS servers?

@GurliGebis
Copy link
Contributor Author

Just checked the strongswan documentation - my patch does not enable accounting.
https://wiki.strongswan.org/projects/strongswan/wiki/EAPRadius

I think it would be better to save that for when it is changed to use the auth framework, since it would be possible to define it on the server there, what should be used and what shouldn't.

@GurliGebis
Copy link
Contributor Author

Okay, I know how this should be done.
Please don't merge this pull request until I give an okay (in case the idea I have does not work).

I will get back with a new pull request :)

@GurliGebis
Copy link
Contributor Author

Just got confirmation back from strongswan people - it is not possible to define different servers for different connections.

https://wiki.strongswan.org/issues/2229

So I plan on doing it the other way around - so to use this, you have to add one or more RADIUS servers to the auth framework.
Then, when you enable EAP-RADIUS for a connection, it takes all the RADIUS servers that is defined in there.

Does that sound like a usefull idea?

@GurliGebis
Copy link
Contributor Author

I will be recreating my repo, since the patches should not have been commited to the master branch (and it cannot be rolled back now that I has been pushed to github).

I will change my implementation like mentioned above, and create a new pull request from the new repository

@GurliGebis
Copy link
Contributor Author

Done, I have reimplemented it using the servers in the auth framework.

Please see the new pull request here: #1345

I'm closing this one, since this version should not be pulled.

@GurliGebis GurliGebis closed this Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants