T6486: T6379: Rewrite generate openvpn client-config #3747

sever-sever · 2024-07-02T05:48:43Z

Change Summary

This command helps to generate users .ovpn files Rewrite generate openvpn client-config to use Config() It needs to get the default values as ConfigTreeQuery is not supporting default values.

Fixed "ignores configured protocol type" if TCP is used
Fixed lzo, was used even if lzo was not configured
Fixed encryption does not parse the dict

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)

Related PR(s)

Component(s) name

openvpn

Proposed changes

How to test

Configure Openvpn in TCP mode,

set interfaces openvpn vtun20 encryption ncp-ciphers 'aes256'
set interfaces openvpn vtun20 hash 'sha512'
set interfaces openvpn vtun20 local-host '203.0.113.1'
set interfaces openvpn vtun20 mode 'server'
set interfaces openvpn vtun20 protocol 'tcp-passive'
set interfaces openvpn vtun20 server subnet '10.0.22.0/24'
set interfaces openvpn vtun20 tls ca-certificate 'openvpn-ca'
set interfaces openvpn vtun20 tls certificate 'openvpn-cert'
set interfaces openvpn vtun20 tls dh-params 'dh'

Before the fix the proto is UDP, but expected TCP
Generated OpenVPN client config file

client
nobind
remote None 1194
remote-cert-tls server
proto udp
dev tun
dev-type tun
persist-key
persist-tun
verb 3
comp-lzo no

After the fix the protocol is correct, encyption with correct maps, lzo not exists:

vyos@vyos# run generate openvpn client-config interface vtun20 ca openvpn-ca certificate openvpn-client

client
nobind
remote 203.0.113.1 1194
remote-cert-tls server
proto tcp-client
dev tun
dev-type tun
persist-key
persist-tun
verb 3

# Encryption options

cipher AES-256-CBC
data-ciphers AES-256-CBC
auth sha512
...

Smoketest result

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

github-actions · 2024-07-02T05:49:09Z

👍
No issues in PR Title / Commit Title

github-actions · 2024-07-02T08:09:29Z

❌ VyOS CLI smoketests failed!

dmbaturin

There are lots of very necessary updates and good ideas here, but I think if we are at it, we can make the script a lot better by integrating it with the pki subtree and converting it to the new-style op mode.

src/op_mode/generate_ovpn_client_file.py

dmbaturin · 2024-07-02T13:51:16Z

src/op_mode/generate_ovpn_client_file.py

-    parser.add_argument("-a", "--ca", type=str, help='OpenVPN CA cerificate', required=True)
-    parser.add_argument("-c", "--cert", type=str, help='OpenVPN client cerificate', required=True)
-    parser.add_argument("-k", "--key", type=str, help='OpenVPN client cerificate key', action="store")
+    parser.add_argument(


Is there a reason not to convert this scripts to the new-style op mode? I think people may want to retrieve OpenVPN client configs through the API.

The script is not ideal.
For example, if the server listen address is not set, which address should be on the client site (x.x.x.x/None/'replace me')?
For now, it generates x.x.x.x, which is not the address. This is not a good solution for the API where all data must be actual, but if it is OK, we can probably overwrite it.

I wonder if we should add an argument like strict: book. If it's true, then any incomplete config will result in an exception vyos.opmode.DataUnavailable. If it's false, then the script will output a placeholder, maybe as a commented-out line.

# listen-address is missing from the config # To make this config usable, add "remote <server address> option, like: # remote openvpn.example.net

It is more bug fix.
Can we merge it as it is and refactor it if required in the separate PR?

dmbaturin · 2024-07-02T13:54:49Z

src/op_mode/generate_ovpn_client_file.py

+        help='OpenVPN interface the client is connecting to',
+        required=True,
+    )
+    parser.add_argument(


Those options were more or less direct translations from my really old script that I wrote for use outside of VyOS and then crudely ported to VyOS. At the time explicit options used to make sense, sort of, because certs and keys were stored in files.

But now they are stored in the config, so I think we should remove the explicit CA option and just fetch it from the pki entry.

But we get them from PKI as completion helper, we do not use this script directly as separate file
it generates from the command

vyos@vyos# run generate openvpn client-config interface vtun20 ca openvpn-ca certificate openvpn-client

Where openvpn-ca and openvpn-client are certificate names from the PKI path

My point is, the interface already defines which CA to use. There is no reason to ask the user for it separately.

To be honest, the tls ca is multi-value

vyos@r4# show interfaces openvpn vtun20 tls ca-certificate ca-certificate ca +ca-certificate cb +ca-certificate cc +ca-certificate cd [edit] vyos@r4#

I wonder if clients should also allow all the same CAs in that case.

It will be behavior change and should be processed by a separate PR
If I understand correctly, your goal is replacing CLI op-mode to provide client-cert only as an arg
I.e Before:

generate openvpn client-config interface vtun20 ca openvpn-ca certificate openvpn-client

After:

generate openvpn client-config interface vtun20 certificate openvpn-client

This way all CA's will be included from pki ca based on the vtunX configuration
Do we really need it? If yes, it is better to do it in a separate PR

I never used several CA's for a client. Even in theory is it possible to sign a client cert by several CA's?

Does client mode support multiple CAs? If there are multiple CAs in an organiazation, I suppose the assumption is that the server cert can also be signed with any of them, and thus the client may need to verify the server cert against multiple CAs.

Let's fix bugs without new options.
The original bugs https://vyos.dev/T6486 and https://vyos.dev/T6379
And both should solved in this change.

src/op_mode/generate_ovpn_client_file.py

This command helps to generate users `.ovpn` files Rewrite `generate openvpn client-config` to use Config() It needs to get the default values as `ConfigTreeQuery` is not supporting default values. Fixed "ignores configured protocol type" if TCP is used Fixed lzo, was used even if lzo not configured Fixed encryption is not parse the dict

github-actions · 2024-07-02T15:34:35Z

CI integration 👍 passed!

Details

CI logs

👍 passed CLI Smoketests returned:
👍 passed Config tests returned:
👍 passed RAID1 tests returned:

dmbaturin

Ok, let's merge it for the fixes first and figure out the rest later.

sever-sever · 2024-08-01T12:51:56Z

@Mergifyio backport circinus sagitta

mergify · 2024-08-01T12:52:10Z

backport circinus sagitta

✅ Backports have been created

#3925 T6486: T6379: Rewrite generate openvpn client-config (backport #3747) has been created for branch circinus
#3926 T6486: T6379: Rewrite generate openvpn client-config (backport #3747) has been created for branch sagitta

T6486: T6379: Rewrite generate openvpn client-config (backport #3747)

sever-sever requested a review from a team as a code owner July 2, 2024 05:48

sever-sever requested review from dmbaturin, sarthurdev, zdc, jestabro, c-po and nicolas-fort July 2, 2024 05:48

github-actions bot assigned sever-sever Jul 2, 2024

github-actions bot added the current label Jul 2, 2024

dmbaturin requested changes Jul 2, 2024

View reviewed changes

sever-sever force-pushed the T6486 branch from 0bffdb0 to 6e47ea6 Compare July 2, 2024 15:26

sever-sever force-pushed the T6486 branch from 6e47ea6 to fe50f1a Compare July 2, 2024 15:30

sever-sever requested a review from dmbaturin July 2, 2024 15:31

dmbaturin approved these changes Jul 30, 2024

View reviewed changes

c-po approved these changes Jul 30, 2024

View reviewed changes

c-po merged commit 80c45be into vyos:current Jul 30, 2024
11 of 12 checks passed

github-actions bot added the backport label Aug 1, 2024

This was referenced Aug 1, 2024

T6486: T6379: Rewrite generate openvpn client-config (backport #3747) #3925

Merged

T6486: T6379: Rewrite generate openvpn client-config (backport #3747) #3926

Merged

dmbaturin added a commit that referenced this pull request Aug 1, 2024

Merge pull request #3925 from vyos/mergify/bp/circinus/pr-3747

08bf14a

T6486: T6379: Rewrite generate openvpn client-config (backport #3747)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T6486: T6379: Rewrite generate openvpn client-config #3747

T6486: T6379: Rewrite generate openvpn client-config #3747

sever-sever commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024

dmbaturin left a comment

dmbaturin Jul 2, 2024

sever-sever Jul 2, 2024

dmbaturin Jul 3, 2024

sever-sever Jul 28, 2024

dmbaturin Jul 2, 2024

sever-sever Jul 2, 2024

dmbaturin Jul 3, 2024

sever-sever Jul 3, 2024

dmbaturin Jul 4, 2024

sever-sever Jul 4, 2024

dmbaturin Jul 8, 2024

sever-sever Jul 28, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024

dmbaturin left a comment

sever-sever commented Aug 1, 2024

mergify bot commented Aug 1, 2024 •

edited

Loading

T6486: T6379: Rewrite generate openvpn client-config #3747

T6486: T6379: Rewrite generate openvpn client-config #3747

Conversation

sever-sever commented Jul 2, 2024 • edited Loading

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

github-actions bot commented Jul 2, 2024 • edited Loading

github-actions bot commented Jul 2, 2024

dmbaturin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sever-sever Jul 28, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 2, 2024

Details

dmbaturin left a comment

Choose a reason for hiding this comment

sever-sever commented Aug 1, 2024

mergify bot commented Aug 1, 2024 • edited Loading

✅ Backports have been created

sever-sever commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

sever-sever Jul 28, 2024 •

edited

Loading

mergify bot commented Aug 1, 2024 •

edited

Loading