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.
| # Only MacOSX and MSWin32 are supported | ||
| return unless $OSNAME =~ /^darwin|MSWin32$/; | ||
|
|
There was a problem hiding this comment.
You probably misunderstood my purpose: Mozilla::CA library is supported by default on all platform.
"ssl-keystore" is only there to permit usage of certificates deployed in keystore or keychain as related OS provides tools to handle such deployment.
On other OS, we don't have such usage and people still have other solutions. So don't enable "ssl-keystore" support on other OS and keep this code.
There was a problem hiding this comment.
From what I've seen, the Mozilla::CA isn't loaded because this line exit the function if OS is not macOS or Windows, but if Mozilla::CA is supported on Unix/Linux systems but this support isn't enable because the code exits the function before importing it.
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. |
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.