Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When creating an OpenVPN server in v2 of the API, there is no option to generate a TLS Key, as there was in v1 #570

Closed
Coffee-Processing-Unit opened this issue Sep 26, 2024 · 3 comments · Fixed by #574
Labels
enhancement Issues or PRs that enhance existing features

Comments

@Coffee-Processing-Unit
Copy link

Expected behavior

As there was in v1 of the API when creating an OpenVPN server, there will be a way to specify the TLS key will be autogenerated.
In v2, the TLS key can either not exist or imported from outside.

Root Cause

In the v1 code, the TLS would have been auto-generated (with pfsense's openvpn_create_key function) based on some conditions:

private function __validate_tls() {
        # Check for our optional `TLS` payload value
        if (isset($this->initial_data["tls"])) {
            # Auto generate TLS key if empty
            if ($this->initial_data["tls"] == "" or $this->initial_data["tls"] == "true") {
                $this->validated_data["tls"] = base64_encode(openvpn_create_key());
            } else {
                $this->validated_data["tls"] = base64_encode($this->initial_data["tls"]);
            }
        }
    }

In v2, this functionality does not exist. I suggest adding the same functionality to the "validate_tls" function in the case the tls field is empty ("") and not null. Also, add to this field's definition the allow_empty: true property, for this to be possible.
For example:

public function validate_tls(string $tls): string {
	# this if clause is the code addition
        if ($tls == "") {
            $auto_generated_tls = base64_encode(openvpn_create_key())
            return $auto_generated_tls
        }

        # Ensure this TLS key begins with the OpenVPN key prefix and suffix
        if (!str_contains($tls, self::STATIC_KEY_PREFIX) and !str_contains($tls, self::STATIC_KEY_SUFFIX)) {
            throw new ValidationError(
                message: 'Field `tls` must be a valid OpenVPN TLS key.',
                response_id: 'OPENVPN_SERVER_TLS_INVALID_KEY',
            );
        }

        return $tls;
    }
);

Affected Endpoints:
/api/v2/vpn/openvpn/server

@jaredhendrickson13
Copy link
Owner

I'll have these adjustments out in v2.2.0 but I've had to rework them a bit. The main issue here is the tls field acts as both a method of assigning a TLS key, as well as sort of boolean that enables TLS for the server. This implicit behavior is somewhat confusing and goes against RESTful principals. Adding an empty string into the mix just furthers the problem.

Instead, I've added a new use_tls boolean field that is populated based on whether a tls value is stored in the config. I've put the related TLS fields behind this field so they will only apply when use_tls is true. This better matches the behavior in the webConfigurator, allows the tls field to use a default value safely, and should hopefully clear up potentially confusion.

v2.2.0 should be out later this week. A new pre-release build should be going out tonight if you want to try it out.

@Coffee-Processing-Unit
Copy link
Author

Thank you.
To ensure I understand you correctly - if use_tls will be false, then will the TLS be auto-generated?

@jaredhendrickson13
Copy link
Owner

Not quite, use_tls will match the behavior of this setting:

Screenshot 2024-09-30 at 8 47 21 AM

If use_tls is false, the related TLS settings will become unavailable like they do in the UI. If this is set to true and there is not a tis value assigned (or null is provided) a new TLS key will be generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or PRs that enhance existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants