vyos-1x icon indicating copy to clipboard operation
vyos-1x copied to clipboard

T5871: ipsec remote access VPN: specify "cacerts" for client auth

Open lucasec opened this issue 1 year ago • 3 comments

Change Summary

For authentication methods that depend on validating a client certificate against a CA (e.g. EAP-TLS), we currently do not explicitly tell strongswan which CA to use. This PR specifies the cacerts option explicitly for every connection to ensure the connection only accepts certificates signed by its specific CA.

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes)
  • [ ] Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • [ ] Other (please describe):

Related Task(s)

  • https://vyos.dev/T5871

Related PR(s)

N/A

Component(s) name

ipsec remote-access

Proposed changes

The configured CA certificate fro the connection is added to the swanctl.conf connection definition using the cacerts option.

How to test

Example configuration:

[edit vpn ipsec remote-access connection ClientVPN]
lucas@lcn-router# show
 authentication {
     client-mode eap-tls
     local-id <local id>
     server-mode x509
     x509 {
         ca-certificate <ca certificate name>
         certificate <server certificate name>
     }
 }
 local-address <router IP address>
 ...

Validate that /etc/swanctl/swanctl.conf specifies cacerts = <ca certificate name>_1.pem under remote.

Smoketest result

Checklist:

  • [x] I have read the CONTRIBUTING document
  • [x] I have linked this PR to one or more Phabricator Task(s)
  • [x] I have run the components SMOKETESTS if applicable
  • [x] My commit headlines contain a valid Task id
  • [x] My change requires a change to the documentation
  • [ ] I have updated the documentation accordingly

lucasec avatar Dec 29 '23 06:12 lucasec

@c-po Updated with smoketests and rebase onto latest.

@sarthurdev I did a little more investigation into the CA chain behavior you introduced in https://github.com/vyos/vyos-1x/commit/1ac230548c86d3308ff5b479b79b0e64b75a0e8a. From my understanding, the user would specify the intermediate CA name under authentication x509 ca-certificate, and then the config logic would render the intermediate as CA_name_1.pem, next parent as CA_name_2.pem, up to the root being CA_name_N.pem.

I would think in most cases the user would want strongswan to be "strict", as in only accept client certs signed by the intermediate. In that case, the current behavior in this PR, which renders cacerts = CA_name_1.pem, would match the user's expectation. But I could see valid cases where the user would want more broad acceptance, e.g. accept client certs signed by other intermediates under the same root (closer to the current behavior without this PR).

Definitely open to more discussion on this, e.g. are we overloading too much behavior on the x509 ca-certificate option.

lucasec avatar Jan 08 '24 02:01 lucasec

@sarthurdev I did a little more investigation into the CA chain behavior you introduced in 1ac2305. From my understanding, the user would specify the intermediate CA name under authentication x509 ca-certificate, and then the config logic would render the intermediate as CA_name_1.pem, next parent as CA_name_2.pem, up to the root being CA_name_N.pem.

I would think in most cases the user would want strongswan to be "strict", as in only accept client certs signed by the intermediate. In that case, the current behavior in this PR, which renders cacerts = CA_name_1.pem, would match the user's expectation. But I could see valid cases where the user would want more broad acceptance, e.g. accept client certs signed by other intermediates under the same root (closer to the current behavior without this PR).

Thanks for raising this, it's a very good point. Thinking my commit should be reverted and a multi value ca-certificate node is made available instead.

sarthurdev avatar Jan 11 '24 09:01 sarthurdev

If I understand correctly, there are sort of two behaviors an admin might want to accomplish by supplying an intermediate to strongswan:

  1. Allow strongswan to "complete the chain" if the client supplies a cert signed by the intermediate but does not supply the intermediates along with it (I am familiar with transmitting multiple certs over the wire in the context of HTTP, but not as much with if/how that is implemented in IKE).
  2. The admin might want to only have certs issued by a specific intermediate trusted.

In case 1, if there were other intermediates below the root and the admin also wanted those trusted, they would need to be able to load all of the different intermediates at the same level into strongswan. If there are multiple levels of intermediates it seems like the number of certs that need to be loaded could explode out quite a bit.

And in case 2, if you only want the intermediate trusted, would strongswan need to have access to the root at all?

lucasec avatar Jan 12 '24 02:01 lucasec

@lucasec can you rebase your work to the latest current now that #3205 is merged?

c-po avatar Mar 28 '24 19:03 c-po

Rebased accordingly. Verified the updated smoketests pass but I am holding testing this on my live system until I finish up some other changes tomorrow (feel free to merge or hold at your discretion).

lucasec avatar Mar 30 '24 07:03 lucasec

Installed on my live system and looks fine.

I am still trying to chase some other issue causing system crashes seemingly related to ipsec, but those predate this PR (I am now investigating whether they are possibly related to https://github.com/vyos/vyos-1x/pull/3157).

lucasec avatar Apr 05 '24 07:04 lucasec

@lucasec is it ready to merge?

sever-sever avatar Apr 09 '24 17:04 sever-sever

Yes—this one is clear to merge.

lucasec avatar Apr 09 '24 17:04 lucasec

@MegifyIo backport sagitta

c-po avatar Apr 12 '24 05:04 c-po

@c-po it looks like it didn't create the backport pr

GurliGebis avatar Apr 12 '24 09:04 GurliGebis

@mergifyio backport sagitta

sarthurdev avatar Apr 12 '24 09:04 sarthurdev

backport sagitta

✅ Backports have been created

mergify[bot] avatar Apr 12 '24 09:04 mergify[bot]