Support data-ciphers and data-ciphers-fallback options

[rvm-peercode]

Pull Request (PR) description

Support data-ciphers and data-ciphers-fallback options for OpenVPN 2.5.

This Pull Request (PR) fixes the following issues: n/a

[smortex]

Some minor data types issues. I added in-line comments to address them.

I would suggest to stick to String in this PR to match the rest of the module for now, and if you are up to it work on improving the module so that OpenVPN use the default values (i.e. no parameter is output) when no explicit value is provided instead of when a '' is passed in another PR that will go in a new major release.

[smortex]

Optional with a default value cannot be forced to undef. Since we test the value against '', it make sense to only allow String:

Suggested change

	Optional[String] $data_ciphers = 'AES-256-GCM:AES-128-GCM',
	String $data_ciphers = 'AES-256-GCM:AES-128-GCM',

[smortex]

Since we test the value against '' bellow, it make sense to only allow String:

Suggested change

	Optional[String] $data_ciphers_fallback = undef,
	String $data_ciphers_fallback = '',

Admitedly this is inelegant, but it is the may this module currently duplicate all config.

[yakatz]

I would also like to see this. A separate PR could change the style.

[rvm-peercode]

I would suggest to stick to String in this PR to match the rest of the module for now

Just a quick question: if the vars are not Optional, how would that work with OpenVPN versions that don't support these yet? I added them as optional so as not to break config for OpenVPN 2.4 and before.

[smortex]

An optional parameter with a default value cannot be forced to "back" to undef: it will take the default value:

class foo (
  Optional[String] $x = 'foo',
) {
  warning("x=${x}")
}

class { 'foo':
  x => undef, # This will print "x=foo"
}

If the parameters are new, indeed adding them to the config may cause trouble with older versions. My recommendation then would be make sure these parameters are only output when the user provided a value for them. This would mean defaulting to '' (what the module seems to currently do) or undef (preferred because it is more correct semantically speaking and allow better type checking).

class foo (
  Optional[String[1]] $x = undef,
  String              $y = '',
) {
  # ...
}

and the corresponding template:

<% if $x { %>
x = <%= $x %>
<% } %>
<% if $y != '' { %>
y = <%= $y %>
<% } %>

[alaunay]

@rvm-peercode if you're still around, can you please try to rebase ? Thanks !

[yakatz]

This is potentially a breaking change. Can it default to the same value as ciphers instead of a hard coded value that could be different than what the user expects?

[rvm-peercode]

The manpage of my OpenVPN (2.6.14-0ubuntu0.24.04.3) states that the default for data-ciphers is

AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305 when Chacha20-Poly1305 is available and otherwise AES-256-GCM:AES-128-GCM.

I've used the default here, and the value is only added to the config if changed from the default, so if the user explicitly specifies something else. I though that should ensure it's not a breaking change. Please let me know if you want it differently.

[rvm-peercode]

BTW It's perfectly fine if you modify this as you see fit, it's been a while since I've worked with this and I don't really have the time to polish it to a large extent I'm afraid.

[yakatz]

I would also like to see this. A separate PR could change the style.