Improvements to ssl-keystore parameter #824
Conversation
…l-keystore As discussed in glpi-project#823, this PR adds the SystemRootCA for macOS and remove the ``ssl-keystore`` limitation on other systems as they could rely at least on Mozilla CA store.
eduardomozart
changed the title
There was a problem hiding this comment.
I'm not agree to enable "ssl-keystore" for other OS as I explain below in my comments.
I would prefer also to change the behavior of system public CA loading for MacOS so it has to be clearly chosen by the user. You (and people) should consider this other command run as time consuming and this is probably not a good thing in most cases.
lib/GLPI/Agent/HTTP/Client.pm
Outdated
| getAllLines( | ||
| command => "security find-certificate -a -p /System/Library/Keychains/SystemRootCertificates.keychain >> '$file'", | ||
| logger => $logger | ||
| ); | ||
| push @certs, IO::Socket::SSL::Utils::PEM_file2certs($file) |
There was a problem hiding this comment.
That was not my idea.
I prefer to change the command on a given "ssl-keystore" value.
Here you force the run of 2 commands where this is not required for most people as Mozilla::CA should provide the public CA certificates.
I would prefer something like:
my $command = "security find-certificate -a -p";
$command .= " /System/Library/Keychains/SystemRootCertificates.keychain"
if $self->{ssl_keystore} =~ /^system-ssl-ca$/i;
getAllLines(
command => "$command > '$file'",
logger => $logger
);
and even, on l.695, the test on Mozilla::CA should be changed to not load it if system public ca are used as this would be redundant:
if (($OSNAME ne 'darwin' || $self->{ssl_keystore} !~ /^system-ssl-ca$/i) && Mozilla::CA->require()) {
There was a problem hiding this comment.
The Mozilla CA may not include self-signed CA certs available on system store. I updated the macOS code to meet your expectations, but I still believe the system root CA store should be loaded instead of the current user (root) as it's useless since it contains only Apple CA's - I know the default CA store from the user can be changed but most users won't do that.
I updated the code to load Mozilla::CA only if @certs is empty (it means it didn't load the CAs from third party cert stores) or if it's macOS and user didn't provided the 'system-root-ca' parameter.
| # Like Mozilla::CA, but using certs from /etc/ssl/certs | ||
| if ($OSNAME !~ /^darwin|MSWin32$/) { | ||
| my $sslcacert = "/etc/ssl/certs/ca-certificates.crt"; | ||
| push @certs, IO::Socket::SSL::Utils::PEM_file2certs($sslcacert) | ||
| if -e $sslcacert; | ||
| } | ||
|
|
There was a problem hiding this comment.
As I said on my first comment, this is not required as this is still the default.
There was a problem hiding this comment.
I believe there should be cases where internal CA is used and some automation tool already added the self-signed certificate to system CA store and it's not a public certificate provided by Mozilla::CA. As this use case is supported on Windows and macOS (otherwise it wouldn't need to import the keystore/keychain from those systems), I don't see why don't include support for keystore of other systems too, only if the SSL library loads it by default?
To complete my comment, I think this case should only be used if people are in the case glpi server certificate public CA is not available in Mozilla::CA but it is in system public CA. And this should only happen in really rare cases. |
|
as I'm about to make a new release in the next few days, I preferred to merge manually your PR removing no more required changes. For me, on MacOSX, the keychain default support permits people to install glpi server certificate in the safe keychain of root user. Even if most people won't do that, this is a feature they can easily use. Now, the "ssl-keystore=system-ssl-ca" option is a security for users: Actually Mozilla::CA is updated during releases, but in the case the project dies in a far future, using this option may help people to continue using glpi-agent with server installed with SSL signed by a public CA not known at the time of glpi-agent release. |
|
That's great! |
|
It's a bit sad that the support for extracting CA trust store hasn't been approved for Unix/Linux and it relies only on Mozilla:CA since, as you'd said, if someday the GLPI-Agent dies and embedded Mozilla::CA becomes old, or if the user uses a self-signed CA imported to CA trust store, it will not be trusted by default. |
|
|
Yes, I know, but it requires manual intervention, while |
|
I'm not agree with you. "ssl-keystore" feature would also require a manual intervention in that hypothetical case. Adding an overload charging more certificates where this is not required most of the time and for most users was not an option. |
It depends. If it's the Let's Encrypt CA, it's updated by updating the package |
|
sorry, but PR comments thread is not a forum. The PR has been merged manually with some changes, end of the discussion. If you need to discuss on a point, please open a new discussion. |
Add SystemRootCA for macOS and support other OSes to Mozilla:CA in ssl-keystore. This also adds support for using certs from /etc/ssl/certs (Unix/Linux systems like Debian).
As discussed in #823, this PR adds the SystemRootCA for macOS and remove the
ssl-keystorelimitation on other systems as they could rely at least on Mozilla CA store.