-
Notifications
You must be signed in to change notification settings - Fork 590
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
FreeRADIUS - put CRL on a new line (Bug #7491), add some messages for missing certificates #344
Conversation
@@ -4517,6 +4531,15 @@ function freeradius_validate_eap($post, &$input_errors) { | |||
$input_errors[] = "The 'Maximum Sessions Tracking Per Server' field must contain an integer value."; | |||
} | |||
|
|||
// Certificates for TLS | |||
if ($post['ssl_ca_cert'] == 'none') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If "None" is not valid why even offer it in the drop-down? Does that break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well probably because showing random first thing returned by the lookup did not seem like the best idea around? (Generic problem with these drop-downs in pfS.) Been there for ages, personally I have no clue what's been the idea with this. The package has never worked without certificates, originally it's been bootstrapping some self-signed one there using the removed bundled certificate "manager".
It could be renamed to "not configured" or even " " but there needs to be some default empty placeholder, otherwise users will end up saving a random cert/CA combo that doesn't match each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK. I didn't know it was generating its own if 'None' was selected before.
If these are a hard requirement now, I think I'd prefer to see the TLS settings moved to the Settings page. Or maybe have "None" trigger generation and selection of a new self-signed cert entry in the cert manager, to bridge the old/new behavior. When they return to the page it would show something like "FreeRADIUS-auto-" for the name, similar to the automatic GUI cert. That seems like the path of least resistance for users. That or maybe fall back to selecting the same cert as the GUI?
Either way I'd prefer not to make the user go out of their way if we can avoid it. I can work on that if you'd like, but it might be early next week before I can get to it. We can import this in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about grabbing the code from the stunnel package to generate a self-signed certificate that shows in the pfSense cert manager on install (https://github.com/pfsense/FreeBSD-ports/blob/devel/security/pfSense-pkg-stunnel/files/usr/local/pkg/stunnel.inc#L90), but yeah, I'd rather get out something that at least points users to the required configuration now and polish things later.
Definitely a good idea to move the settings from the EAP tab, kinda weird place for that.
Also added some messages and input validation regarding EAP certificates here (see https://redmine.pfsense.org/issues/7479).