opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.25k stars 725 forks source link

IPSec: Parameters and Description in the IPsec configuration dialog should be improved #5241

Closed somova closed 2 years ago

somova commented 2 years ago

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

Is your feature request related to a problem? Please describe.

For a better understanding and clarification some descriptions in the IPsec configuration dialogues should to be updated:

Describe the solution you like

  1. In the IPsec configuration in the web gui some authentication schemes are named incorrectly:

    • Certificate based authentication schemes contain "RSA" in their name, whereas "RSA" is only a specific cryptographic algorithm. The chosen algorithm is defined by the certificate used for authentication (e.g. RSA, DSA, ECC). Please rename it to some kind of "x.509 cert" or similar.
  2. Furthermore, some parameter descriptions look a little bit confusing and should be renamed (some examples)

    • "My Certificate Authority" defines the CA for remote endpoint authentication (should be better named as "Remote endpoint authentication CA")
    • "My Certificate Authority" description should describe that this CA defines the trust anchor of remote endpoint certificates
    • "My Certificate" description should describe "Certificate to authenticate the local endpoint among peer the endpoint"
somova commented 2 years ago

Bump

AdSchellevis commented 2 years ago

Bumping a number of tickets (https://github.com/opnsense/core/issues/5240#issuecomment-962435891, https://github.com/opnsense/core/issues/5239#issuecomment-962435908) likely doesn't help people to get enthusiastic about your priorities. (personally I find it a bit rude to be honest)

Spending (your own) time to propose changes using pull requests might be more fruitful.

somova commented 2 years ago

There is nothing rude on that. I'm just pretty worried about security because it looks like there is no really interest in this kind of issues. Nobody demands solving this issues within a day. But, we should consider Opnsense is a security component and a lax approach will damage the reputation of and user confidence in this project. This I'll try to avoid and I differentiate classical software bugs from security related ones.

So, please don't get angry about bumping this issues in upper direction. For me it's important to invest more effort in quality and security of this great product. Let's do it as a whole community (not everybody is a programmer or software developer).

Thanks.

joelmacklow commented 2 years ago

Hi @AdSchellevis. In working to get S2S Certificate based VPN configured, I believe I have come across the issue identified as point 2 in this original ticket.

In the GUI, under VPN -> IPSEC ->Tunnel Settings -> Phase 1 proposal (Authentication)

When using RSA (certificate based connection), the option "My Certificate Authority" under "My Certificate" is a little misleading... I thought it should be the intermediate that signs the certificate I am presenting.

I couldn't get the tunnel to connect, and as part of troubleshooting, I looked at /usr/local/etc/ipsec.conf. There I discovered that what is listed as "My Certificate" in the GUI is saved in the ipsec.conf file as rightca. This is of course the intermediate CA that I have loaded in the CA trust store that I am using to validate the certificate presented to me from the remote end.

So I think the wording in the GUI "My Certificate" needs to be changed to maybe "Remote CA Certificate".

Fixing this bug would lead to IKEv2 certificate based VPNs having a better chance of authenticating.... and we all know how difficult they can be to start with! Appreciate any help you can provide. I can test any patches and provide feedback if that's helpful.

AdSchellevis commented 2 years ago

@joelmacklow "My Certificate" should be the left part, according to : https://github.com/opnsense/core/blob/c3bae88bf5195e3f54b6e7bfba8cafce633164c5/src/etc/inc/plugins.inc.d/ipsec.inc#L1587-L1590

"My Certificate Authority" is indeed misleading as it points to the right (remote) trust anchor. https://github.com/opnsense/core/blob/c3bae88bf5195e3f54b6e7bfba8cafce633164c5/src/etc/inc/plugins.inc.d/ipsec.inc#L1591-L1600

As changing the ca description doesn't seem to impact our documentation (or the examples), I don't mind changing that part now. Unfortunately the IPsec is inherited and contains more inconsistencies, most are not very easy to solve without breaking existing setups. https://github.com/opnsense/core/commit/f18ae14f98f77594c55154dd4f44ee494c0415c0 contains the suggested change

somova commented 2 years ago

@AdSchellevis Do you know what exactly can break? I can imagine that selecting the appropriate authentication algorithms for multiple authentication rounds can be tricky because in the GUI they are partly combined. Extending the GUI to configure missing configuration parameters should be uncomplicated.

IPsec itself is a complicated but a powerful and flexible VPN technology. So, this is nothing which will work out of the box.

AdSchellevis commented 2 years ago

@somova I was just pointing to the fact that the gui doesn't always follow the logic from the underlaying technology (strongswan), which means that there are interpretations/"assumptions" in different areas. If existing labels are wrong, we can change them, but only if it's absolutely sure it's wrong and verified. (like the "My Certificate Authority" label)

Too broad suggestions about which areas we might change are just unlikely to gain a lot of traction due to the time it takes to validate claims. We just can't revert past choices easily, like how authentication schemes end up in strongswans config:

https://github.com/opnsense/core/blob/c3bae88bf5195e3f54b6e7bfba8cafce633164c5/src/etc/inc/plugins.inc.d/ipsec.inc#L1555-L1557

somova commented 2 years ago

Because of the complexity a good way will be to do a brown field migration. Configuration can be programmed as a new GUI (parallel to existing one, marked as legacy). The new GUI uses only the new swanctl.conf and vici interface. The old ipsec/stroke and swanctl/vici can exist in parallel.

This way, we have enough time for implementation and testing while the old configuration and GUI can be phased out at a later time.

AdSchellevis commented 2 years ago

Yes it could, but needs a huge investment and a lengthy and difficult migration path, the costs currently outweigh the benefits (we looked into this with prospects in the past).

somova commented 2 years ago

I don't think the costs outweigh the benefits even though I cannot exactly estimate the costs. I often heard this argument around software development, but it does not help postponing things which need to be solved in a certain time. The used interface (stroke and configuration file format) is quite old and its support can be dropped sometimes. For this case the Opnsense project should be prepared. Starting a brown field migration (the new interface can coexist to the old one) will not break anything.

Besides this there are some security issues (I already mentioned some of them) which are currently ignored. This is not a good advertisement for a product that is also offered as a business product. At the latest, if a security incident is traced back to one of these security gaps, this can mean considerable damage to the reputation of the project.

AdSchellevis commented 2 years ago

I don't think the costs outweigh the benefits even though I cannot exactly estimate the costs. I often heard this argument around software development, but it does not help postponing things which need to be solved in a certain time. The used interface (stroke and configuration file format) is quite old and its support can be dropped sometimes. For this case the Opnsense project should be prepared

We can, have done so in many occasions, and the current situation doesn't warrant a rewrite.

Starting a brown field migration (the new interface can coexist to the old one) will not break anything.

Nope, it wouldn't, there would be two used interfaces for a very long period of time without a migration path both needing attention in terms of development and documentation. If we would consider a rewrite, it would make more sense to put it into a commercial edition.

Besides this there are some security issues (I already mentioned some of them) which are currently ignored.

Not sure what you're referring too. My advise would be to describe the solution you're seeking within the boundaries that exist, which makes it more likely to be picked up at some point. Ovoid the use of words as "some" and "like", be explicit. You obviously know what you're talking about, try to put it into a ticket that others can relate to (and assess what would break if your proposal would be accepted).

At the latest, if a security incident is traced back to one of these security gaps, this can mean considerable damage to the reputation of the project.

Which incidents? I'm really having difficulties by the ease you are seemingly assuming others are going to work on your concerns without considering the interpretation of the reader on the other end. Think in steps, maybe try to write some code and offer a PR, just an advise if you're seeking change (be part of the change you're seeking).

somova commented 2 years ago

I don't think the costs outweigh the benefits even though I cannot exactly estimate the costs. I often heard this argument around software development, but it does not help postponing things which need to be solved in a certain time. The used interface (stroke and configuration file format) is quite old and its support can be dropped sometimes. For this case the Opnsense project should be prepared

We can, have done so in many occasions, and the current situation doesn't warrant a rewrite.

It's only a recommendation. But, in case the strongswan project decides to drop old interface support (e. g. within 6 months) opnsense is not prepared. The project can then run an old version of strongswan or do the patching of possible security issues itself.

Starting a brown field migration (the new interface can coexist to the old one) will not break anything.

Nope, it wouldn't, there would be two used interfaces for a very long period of time without a migration path both needing attention in terms of development and documentation. [...]

I don't think so. The strongswan project has a good wiki page around the migration topic [1]. And there is also a python based config translator which hopefully works for any standard configuration (I haven't tested it).

Besides this there are some security issues (I already mentioned some of them) which are currently ignored.

Not sure what you're referring too. [...]

I had already described one security issue [2]. But without any noteworthy reaction :-(

At the latest, if a security incident is traced back to one of these security gaps, this can mean considerable damage to the reputation of the project.

Which incidents?

See link [2]. Related to this there are further issues with orphaned certificates and revocation of certificates.

[1] https://wiki.strongswan.org/projects/strongswan/wiki/Fromipsecconf [2] https://github.com/opnsense/core/issues/5239

AdSchellevis commented 2 years ago

It's only a recommendation. But, in case the strongswan project decides to drop old interface support (e. g. within 6 months) opnsense is not prepared. The project can then run an old version of strongswan or do the patching of possible security issues itself.

yes, eventually we should switch the output format to swanctl.conf as part of our maintenance, which highly likely doesn't require a rewrite of the system. opened a ticket for it here https://github.com/opnsense/core/issues/5636, if there's a clear path when Strongswan decides to drop support, it would be good to add it, I couldn't find it.

I don't think so. The strongswan project has a good wiki page around the migration topic [1]. And there is also a python based config translator which hopefully works for any standard configuration (I haven't tested it).

We're obviously not talking about the same things, a rewrite means new more logical input using forms that are powered by our mvc framework. Output format changes are usually part of the maintenance we do, it would help though if explicit target dates are communicated somewhere so we can include those in our planning. (The end of ipsec.conf support isn't clear as far as I could find).

I had already described one security issue [2]. But without any noteworthy reaction :-(

Try reading your own issue again and think of what someone on the other end should do with it... not saying there is no room for improvement, my earlier comments still stands.

See link [2]. Related to this there are further issues with orphaned certificates and revocation of certificates.

Which is a good example, https://github.com/opnsense/core/issues/5600 explains the stickiness of a cert authority and eventually leads to action.

OPNsense-bot commented 2 years ago

This issue has been automatically timed-out (after 180 days of inactivity).

For more information about the policies for this repository, please read https://github.com/opnsense/core/blob/master/CONTRIBUTING.md for further details.

If someone wants to step up and work on this issue, just let us know, so we can reopen the issue and assign an owner to it.

somova commented 2 years ago

opened a ticket for it here https://github.com/opnsense/core/issues/5636

Sounds good. If I get any results from tests, I'll report there.

Try reading your own issue again and think of what someone on the other end should do with it... not saying there is no room for improvement, my earlier comments still stands.

I still don't know what's missing. If anybody of the readers do not understand he should ask within the specific ticket.

But, how do we generally like to proceed? The last days this (#5241) and some more tickets timed out due to inactivity (+ #5239, #5240). I can solve these things myself by manually writing my own IPsec configuration files. But it's not fair to keep back improvements towards the community. Thus, I'd like to address issues here.