openconnect: T4982: Support defining minimum TLS version in openconnect VPN

[Embezzle]

Change Summary

Allow configuration of minimum acceptable TLS version for openconnect VPN.
Default is set at TLSv1.2 to ensure out-of-box/unconfigured option is not insecure.

Types of changes

• 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/T4982

Related PR(s)

Component(s) name

vpn -> openconnect

Proposed changes

How to test

1. Create an openconnect VPN configuration:

set vpn openconnect authentication local-users username example password 'test'
set vpn openconnect authentication mode local 'password'
set vpn openconnect network-settings client-ip-settings subnet '192.168.1.1/30'
set vpn openconnect network-settings name-server '192.168.1.254'
set vpn openconnect ssl ca-certificate 'test-ca'
set vpn openconnect ssl certificate 'test-certificate'
set vpn openconnect tls-version-min 1.2

2. Validate in the configuration file rendered that minimum TLS version has been set correctly:

vyos@vyos:~$ cat /var/run/ocserv/ocserv.conf | grep "tls-priorities"
tls-priorities = "NORMAL:%SERVER_PRECEDENCE:%COMPAT:-RSA:-VERS-SSL3.0:-ARCFOUR-128:-VERS-TLS1.0:-VERS-TLS1.1"

Smoketest result

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_vpn_openconnect.py
test_ocserv (__main__.TestVPNOpenConnect.test_ocserv) ...
SSL missing on OpenConnect config!

ok

----------------------------------------------------------------------
Ran 1 test in 8.705s

OK

Checklist:

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

[c-po]

Hi @Embezzle,

thank you for the contribution. I have some minor comments.

tls-version-min is already defined for OpenVPN interfaces at https://github.com/vyos/vyos-1x/blob/current/interface-definitions/interfaces_openvpn.xml.in#L742 - please create a new file interface-definitions/include/tls-version-min.xml.i which can then be included by both OpenVPN and OpenConnect. This saves redundant lines and makes future maintenance easier.

In addition you define that the default TLS version will be 1.2 - this breaks backward compatibility as the current default TLS version is 1.0. You will either need to downgrade the default to 1.0 or add a migration script which explicitly defined tls-version-min to be set at 1.0 if it‘s not defined in the old configurations. As VyOS 1.4.0-epa3 is yet not released we can add this change to lift the default TLS version.

[Embezzle]

Thank you for the feedback @c-po, I have amended the PR with your requested changes.

The OpenVPN interface implementation of tls-version-min did not previously have a <defaultValue> tag, so I have had to make a minor tweak in interfaces_openvpn.py to account for this.

I do not think this requires a migration script for OpenVPN, as the default minimum version supported if not explicitly configured by VyOS, as was the case before this PR, is also TLSv1.2.

https://openvpn.net/community-resources/reference-manual-for-openvpn-2-6 (see section for: --tls-version-min)

[c-po]

Hi @Embezzle,

no need for any changes on OpenVPN code. Please remove this again as it should happen in a different PR if needed.

Our defaultValue system works as follows:

OpenVPN will only have:

#include <include/tls-version-min.xml.i>

Whereas OpenConnect will have:

#include <include/tls-version-min.xml.i>
<leafNode name="tls-version-min">
    <defaultValue>1.2</defaultValue>
</leafNode>

Meaning that one can overload the default of a previous XML node definition

[Embezzle]

Hi @c-po,

I did not realise you could append tags to includes in the XML definitions like shown, that makes much more sense!
Thank you for your help, I have amended my commit to correct issues you raised & have re-run smoke tests.

[c-po]

@Mergifyio backport sagitta

[mergify]

backport sagitta

✅ Backports have been created

• #3377 openconnect: T4982: Support defining minimum TLS version in openconnect VPN (backport #3371) has been created for branch sagitta