net/sslh: Initial plugin version

[agh1467]

This is to address the discussion about a plugin for sslh and resolves #1630. This is such a plugin.

• Includes setting listen addresses, and protocol targets.
• Includes some other advanced settings
• Service start/stop/restart control

[fichtner]

Suggested change

	PLUGIN_VERSION= 1.21c
	PLUGIN_REVISION= 1
	PLUGIN_VERSION= 0.1
	PLUGIN_DEVEL= yes

We do have separate versioning for software and actual plugin since both a different entities that can receive updates independently.

[agh1467]

I've changed it as you've put here.

I thought about the versioning for a while with respect to dnscrypt-proxy as well. That application has phased out configuration settings, renamed settings, and introduced new settings throughout the versions. Keeping the plugin version in lock step with the application version seemed like the most practical approach since it makes it clear to the user which version of the application is either being installed, or which version the plugin is compatible with, and eliminates the need to have a separate versioning schema for the plugin. Utilizing PLUGIN_REVISION would allow a simpler incremental versioning which could reset with each major version or increment forever, and there wouldn't be any trouble with defining a versioning schema.

Is there any guidance on this? A quick search only netted me the Hello World plugin example which doesn't detail versioning. With some guidance I think I could wrap my head around when each component should be versioned and which component gets versioned under which conditions.

[fichtner]

The problem is you can't be there for any update of dnscrypt-proxy which will make these versions differ and having a separate versioning for both plugin and dependent software makes it easier to see what people talk about. Sometimes people forget to name the revision, too. That being said if you have more than one software dependency how would you decide which software dependency gets to be the authoritative version number? ;)

Besides, revision is used as a means to not tamper with the version number, but you can't increment it "forever" as it makes no sense for hotfixing annotation. Yes, the idea comes from FreeBSD ports where you package a software and can't control the version number, but it's still clearer than not having a revision. The difference at the end of the day is 1.1.1 vs 1.1_1.

There's no document for the versioning aspects in our case. The unwritten rules are 0.x for development, 1.x and up for releases. Usually 2.x denotes a rewrite and some maintainers prefer 1.x.y and when we make quick bugfixes / hotfixes we use the revision instead to sort of denote that the fix may or may not be handled by the maintainer. More often than not, however, hotfixes are proper versions served by maintainers (hello @fraenki 😄). Revisions are cleared when the version changes.

One more thing to keep in mind here is that the makefile denotes the versioning and upgrade behaviour. We don't have any automatic means to update a plugin if this information wasn't modified (either version or revision in case version should stay the same). A year ago we did not even have a commit hash in the packaged plugin. But more on this in another comment here.

[agh1467]

Thanks for the explanation on versioning. Is there a place in the OPNsense docs this info could go? I could give a shot at writing something up. Maybe in the hello world example? https://docs.opnsense.org/development/examples/helloworld.html

[fichtner]

REDACTED

[fichtner]

Oh wait you mean to add a generic configuration... the question would be is this used somehwere else? That directory may not be the best place for it since no relation to configd then. Apologies for doing a linear review from file to file...

[agh1467]

Yup, that's exactly it. The intent here is to define pseudo-"super globals." I provide a broader explanation on the comment of the conf file itself.

An example of how this could be useful is, this file can be copied wholesale, change the file name, and only have to change this line, and the function name to adapt to a new service. I'd guess, it could be reduced further and eliminate the need for this file at all if this information can just be derived from the conf file. For simple services the three actions are probably enough.

Less changing values after copying/pasting the code to stand up a new plugin.

[fichtner]

Ok in any case the file needs to be moved. A more fitting location would be either /usr/local/etc/inc/plugins.inc.d/sshl/sshl.conf or /usr/local/opnsense/data/sshl/sshl.conf.

That being said we already have a plugin-bound meta information file that would be most fitting for these types of values, e.g.:

# cat /usr/local/opnsense/version/ddclient 
{
    "product_abi": "22.1",
    "product_arch": "amd64",
    "product_email": "ad@opnsense.org",
    "product_flavour": "LibreSSL",
    "product_hash": "be8192b82",
    "product_id": "os-ddclient-devel",
    "product_name": "ddclient",
    "product_version": "0.1",
    "product_website": "https://opnsense.org/"
}

I don't mind extending the opnsense-version utility to read manual values from it as long as we can avoid too much clutter. What I don't think is a great idea is to read this file on each file load of a model or plugin invoke, however. And I think @AdSchellevis will agree. For configuration purposes it's fine but for runtime always bouncing through the file or shell command is a performance killer.

[agh1467]

Since the practicality for this isn't represented in this small plugin I've decided to remove it and replace with hard coded strings.

[fichtner]

All things considered the question is if it's worth it. The pid file is usually also defined in the rc.d file of the service that comes with the port. Softcoding it in a second place seems less favourable than hardcoding it once here.

[agh1467]

I happened across this by accident since I copied this file from the dnscrypt-proxy plugin which uses pid as the key name for this value.
https://github.com/opnsense/plugins/blob/master/dns/dnscrypt-proxy/src/etc/inc/plugins.inc.d/dnscryptproxy.inc#L51

Since for dnscrypt-proxy, it happens that the service name and the process name actually match, everything works without issue. However, with sslh, the process names are one of either sslh-fork or sslh-select which doesn't match the 'name' value in the array, and thus it's not able to find the process by name. It only shows the service as stopped in the status column. So, for this case, the pidfile value is how is_process_running() is able to find the process and properly detect if it's running or not.

For this case, the pid file is a command line option/configuration option, and is explicitly defined in the configuration file. It's also defined in the conf for reference here. I also looked in the /usr/local/etc/rc.d/sslh but for sslh it didn't look like one was defined as I've seen in other services.

[fichtner]

It seems that is a bug worth fixing?

# git grep "'pid'"
dns/dnscrypt-proxy/src/etc/inc/plugins.inc.d/dnscryptproxy.inc:        'pid' => '/var/run/dnscrypt-proxy.pid'
net-mgmt/nrpe/src/etc/inc/plugins.inc.d/nrpe.inc:        'pid' => '/var/run/nrpe.pid'
net/ntopng/src/etc/inc/plugins.inc.d/ntopng.inc:        'pid' => '/var/run/ntopng/ntopng.pid'

Well, the 'name' value should match the process name (binary), at least that what is being used if no PID file is given. In the example you give it seems perfectly fine except for the key not being proper. Especially for plugins.inc/d content the file is always bound to be adapted by plugin maintainer.

[agh1467]

I'll take a look at the other plugins and submit PRs for those as well.

[fichtner]

further problems for translations: we don't add strings that offer markup since it could break the html/js (translators can introduce typos easily for their languages), is prone to XSS. Markup and layout inside translations remain a huge problem with no easy solution: opnsense/lang#1

[agh1467]

This makes me cry a little bit. I did this because it looks nice in the XML, and presents well on the page. I understand though. My first question, to which the answer is: You can't, is how do I do paragraphs?

I'd like to better understand this challenge, as I'd like to help if I can. I've seen the usage of gettext and _lang, but I don't see either being used here. I don't understand exactly how the translations work, any help there would be welcome.

I chose this style for the help messages because I've come across many help messages for settings in OPNsense where I could only assume that a particular setting is related to a setting in a service or application. Having this information available for anyone who is familiar with the application would be really helpful especially when it comes to looking up those settings in application documentation. OpenVPN was one standout experience I had with this because many of the settings are short-handed, but long in the UI, so it makes cross referencing difficult.

Not for here to discuss, but an idea which may help with the translation issues, utilizing pre defined styles

<help>
  <header>-p, --listen address:port</header>
  <body>Paragraph one.</body>
  <body>Paragraph two.</body>
</help>

Then do styling using the layout_partials. Would have to figure out which styles are useful, but that would be the basic idea. Can't do it with the current parseFormNode() but I've written one that can.

[fichtner]

You can't, is how do I do paragraphs?

You can, but we won't translate it for the mentioned reasons.

I'd like to better understand this challenge, as I'd like to help if I can.

Model strings are dynamically generated from XML content via https://github.com/opnsense/lang/blob/master/Scripts/plugins/models.py and automatically redirected through gettext() upon form generation. This is more of an oddity of how gettext works and does not work.

I chose this style for the help messages because I've come across many help messages for settings in OPNsense where I could only assume that a particular setting is related to a setting in a service or application.

Essentially you are writing inline documentation in a restricted scope (time and space). Most of elaborate documentation should be on docs.git and it's actually better to have one document for each plugin as we try with the ones that we as a core team introduce nowadays. It's not a bad thing, but having a shorter help in the GUI and a full documentation indexable by search engines has multiple advantages already: less strain on translations, simpler GUI, more time for users to do what they want to do while working with the GUI, full text search from search engine and documentation, examples and tutorials in documentation, more visibility for non-OPNsense users on the topic of the plugin.

Not for here to discuss, but an idea which may help with the translation issues, utilizing pre defined styles

That looks like what we had in mind. The downside (I'm sorry to play devil's advocate) is that while the string are now clean and well-formed we essentially end up with splitting a block of translation that belongs together into separate strings that may or may not be translated, may or may not be translated by the same persons at the same time. Context could get lost. That's why for the most part we would prefer the documentation over this.

It would be nice if you could dump your idea in that lang.git issue for further discussion. Thanks :)

[agh1467]

I recently dug into the opnsense/lang repo while researching something else, and I believe I've gained a better understanding of the challenge. I've changed them to just a plain text description.

You mentioned the docs git, and I hadn't considered writing plugin docs there. I'm thinking the README I've put together will be a good base for such documentation. I'm guessing this is where it would live? https://github.com/opnsense/docs/tree/master/source/manual/how-tos

[fichtner]

thanks so far. I don't see a reason why we can't add this soon after addressing review. My main concerns are maintainability for the translations. I know that other code doesn't do this better but anything other than that "other" code is good progress. :)

[agh1467]

Thanks all for the review and comments! I resolved the comments that were straight forward and changes were made. I left the others open which I thought there might be more to say.

[agh1467]

FYI, I'm planning on revisiting this soon, and re-working parts to address the topics brought up.

[fichtner]

most plugins are frontends. the opportunity here is to describe the software being configured in a nutshell :)

[agh1467]

I'll update the comment to describe the sslh service.

[fichtner]

can leave this in but doesn't look like functional addition. less is often more

[fichtner]

validation message here but no special validation of individual values?

[fichtner]

localhost:443 is probably already bound to web GUI btw

[agh1467]

For this, I considered the options that I'm aware of and can understand, but I'm having a challenge understanding the best approach. I could use something like a regular expression, but the fact that this supports a hostname and IP address will make this complicated to do in single mask.

I thought about a custom FieldType with a new validator using the CallbackValidator, the challenges would be similar, but I could at least split each CSV and explode on the colon (only reliable for IPv4), and evaluate each half of each entry separately. I feel that's still re-inventing the wheel when there exists HostValidator, and PortField already has a validator (though hard coded in the FieldType itself).

I think an option might be to re-factor the field into an ArrayField, and specify the hostname/IP, and port separately and validate each according to each field type. However, I'd prefer not to do that as it would require building the interface(s) to support the user input. I wasn't planning on going down that route until the next version of the plugin.

I'm not strong in the validators area, and I left this alone due to the complexity of specifying a single mask to handle the entire thing. Let me know how this could be approached in a reasonable manner.

[fichtner]

looks good, minor comments for release version progress

[fichtner]

@agh1467 if you ping again I will merge, up to you if you want to change something now or merge first

[coatmaker618]

Super interested 3rd party here who runs OPNSense & REALLY wants this feature. What is the state of this? If I'm reading/understanding this correctly, this change was approved but is not in the current build & is awaiting minor comments?

As an (overly) excited 3rd party, having this capability in OPNSense would be huge and I'm super thankful that it seems to be so close! I'm just making sure it's not lost just before the finish line!

[fichtner]

Merged, thanks!

[haarp]

I know I'm a few days late to the party, but regarding the removal of transparent mode: The official docs have a section for FreeBSD here, so it's not entirely impossible. Unfortunately it involves ipfw instead of pf. Maybe it can be added on top of pf or adapted.

[agh1467]

@haarp Thanks for citing that. I'll have to see if there is an equivalent functionality, and translate it accordingly. It could be added back in then.

[rbray89]

Any news on the transparent option? It's necessary for things like fail2ban to work properly to be able to resolve the remote IP

[thn80]

You wrote that the documentation reports this as a "Linux only" feature and that there is no provision for using this on FreeBSD.
I am wondering about this, because the documentation of sslh states for the Transparent Proxy "On Linux and FreeBSD you can use the --transparent option to request transparent proxying." (Source: https://github.com/yrutschle/sslh/blob/master/doc/tproxy.md).