Remove option to skip certificate validation to prevent insecure configurations
Skipping certificate validation will allow MITM on every single request and not give any real security over just running unencrypted queries. On top of that, there is no way to configure this behavior from within PHP except of setting environment variables, which is really hacky and has side effects on other requests. fixes #9607
This commit is contained in:
parent
23f0686936
commit
9e40f5f2c7
|
@ -81,22 +81,6 @@ class LdapResourceForm extends Form
|
||||||
)
|
)
|
||||||
);
|
);
|
||||||
|
|
||||||
if (isset($formData['encryption']) && $formData['encryption'] !== 'none') {
|
|
||||||
// TODO(jom): Do not show this checkbox unless the connection is actually failing due to certificate errors
|
|
||||||
$this->addElement(
|
|
||||||
'checkbox',
|
|
||||||
'reqcert',
|
|
||||||
array(
|
|
||||||
'required' => true,
|
|
||||||
'label' => $this->translate('Require Certificate'),
|
|
||||||
'description' => $this->translate(
|
|
||||||
'When checked, the LDAP server must provide a valid and known (trusted) certificate.'
|
|
||||||
),
|
|
||||||
'value' => 1
|
|
||||||
)
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
$this->addElement(
|
$this->addElement(
|
||||||
'text',
|
'text',
|
||||||
'root_dn',
|
'root_dn',
|
||||||
|
|
|
@ -122,13 +122,6 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
*/
|
*/
|
||||||
protected $rootDn;
|
protected $rootDn;
|
||||||
|
|
||||||
/**
|
|
||||||
* Whether to load the configuration for strict certificate validation or the one for non-strict validation
|
|
||||||
*
|
|
||||||
* @var bool
|
|
||||||
*/
|
|
||||||
protected $validateCertificate;
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Whether the bind on this connection has already been performed
|
* Whether the bind on this connection has already been performed
|
||||||
*
|
*
|
||||||
|
@ -176,7 +169,6 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
$this->bindPw = $config->bind_pw;
|
$this->bindPw = $config->bind_pw;
|
||||||
$this->rootDn = $config->root_dn;
|
$this->rootDn = $config->root_dn;
|
||||||
$this->port = $config->get('port', 389);
|
$this->port = $config->get('port', 389);
|
||||||
$this->validateCertificate = (bool) $config->get('reqcert', true);
|
|
||||||
|
|
||||||
$this->encryption = $config->encryption;
|
$this->encryption = $config->encryption;
|
||||||
if ($this->encryption !== null) {
|
if ($this->encryption !== null) {
|
||||||
|
@ -957,16 +949,9 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
$info = new Inspection('');
|
$info = new Inspection('');
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($this->encryption === static::STARTTLS || $this->encryption === static::LDAPS) {
|
|
||||||
$this->prepareTlsEnvironment();
|
|
||||||
}
|
|
||||||
|
|
||||||
$hostname = $this->hostname;
|
$hostname = $this->hostname;
|
||||||
if ($this->encryption === static::LDAPS) {
|
if ($this->encryption === static::LDAPS) {
|
||||||
$info->write('Connect using LDAPS');
|
$info->write('Connect using LDAPS');
|
||||||
if (! $this->validateCertificate) {
|
|
||||||
$info->write('Skip certificate validation');
|
|
||||||
}
|
|
||||||
$hostname = 'ldaps://' . $hostname;
|
$hostname = 'ldaps://' . $hostname;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -983,9 +968,6 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
if ($this->encryption === static::STARTTLS) {
|
if ($this->encryption === static::STARTTLS) {
|
||||||
$this->encrypted = true;
|
$this->encrypted = true;
|
||||||
$info->write('Connect using STARTTLS');
|
$info->write('Connect using STARTTLS');
|
||||||
if (! $this->validateCertificate) {
|
|
||||||
$info->write('Skip certificate validation');
|
|
||||||
}
|
|
||||||
if (! ldap_start_tls($ds)) {
|
if (! ldap_start_tls($ds)) {
|
||||||
throw new LdapException('LDAP STARTTLS failed: %s', ldap_error($ds));
|
throw new LdapException('LDAP STARTTLS failed: %s', ldap_error($ds));
|
||||||
}
|
}
|
||||||
|
@ -998,30 +980,6 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
return $ds;
|
return $ds;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Set up how to handle StartTLS connections
|
|
||||||
*
|
|
||||||
* @throws LdapException In case the LDAPRC environment variable cannot be set
|
|
||||||
*/
|
|
||||||
protected function prepareTlsEnvironment()
|
|
||||||
{
|
|
||||||
// TODO: allow variable known CA location (system VS Icinga)
|
|
||||||
if (Platform::isWindows()) {
|
|
||||||
putenv('LDAPTLS_REQCERT=never');
|
|
||||||
} else {
|
|
||||||
if ($this->validateCertificate) {
|
|
||||||
$ldap_conf = $this->getConfigDir('ldap_ca.conf');
|
|
||||||
} else {
|
|
||||||
$ldap_conf = $this->getConfigDir('ldap_nocert.conf');
|
|
||||||
}
|
|
||||||
|
|
||||||
putenv('LDAPRC=' . $ldap_conf); // TODO: Does not have any effect
|
|
||||||
if (getenv('LDAPRC') !== $ldap_conf) {
|
|
||||||
throw new LdapException('putenv failed');
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Create an LDAP entry
|
* Create an LDAP entry
|
||||||
*
|
*
|
||||||
|
@ -1103,6 +1061,13 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
try {
|
try {
|
||||||
$ds = $this->prepareNewConnection($insp);
|
$ds = $this->prepareNewConnection($insp);
|
||||||
} catch (Exception $e) {
|
} catch (Exception $e) {
|
||||||
|
if ($this->encryption === 'starttls') {
|
||||||
|
// The Exception does not return any proper error messages in case of certificate errors. Connecting
|
||||||
|
// by STARTTLS will usually fail at this point when the certificate is unknown,
|
||||||
|
// so at least try to give some hints.
|
||||||
|
$insp->write('NOTE: There might be an issue with the chosen encryption. Ensure that the LDAP-Server ' .
|
||||||
|
'supports STARTTLS and that the LDAP-Client is configured to accept its certificate.');
|
||||||
|
}
|
||||||
return $insp->error($e->getMessage());
|
return $insp->error($e->getMessage());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -1116,6 +1081,13 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
'***' /* $this->bindPw */
|
'***' /* $this->bindPw */
|
||||||
);
|
);
|
||||||
if (! $success) {
|
if (! $success) {
|
||||||
|
// ldap_error does not return any proper error messages in case of certificate errors. Connecting
|
||||||
|
// by LDAPS will usually fail at this point when the certificate is unknown, so at least try to give
|
||||||
|
// some hints.
|
||||||
|
if ($this->encryption === 'ldaps') {
|
||||||
|
$insp->write('NOTE: There might be an issue with the chosen encryption. Ensure that the LDAP-Server ' .
|
||||||
|
' supports LDAPS and that the LDAP-Client is configured to accept its certificate.');
|
||||||
|
}
|
||||||
return $insp->error(sprintf('%s failed: %s', $msg, ldap_error($ds)));
|
return $insp->error(sprintf('%s failed: %s', $msg, ldap_error($ds)));
|
||||||
}
|
}
|
||||||
$insp->write(sprintf($msg . ' successful'));
|
$insp->write(sprintf($msg . ' successful'));
|
||||||
|
@ -1137,12 +1109,4 @@ class LdapConnection implements Selectable, Inspectable
|
||||||
}
|
}
|
||||||
return $insp;
|
return $insp;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Reset the environment variables set by self::prepareTlsEnvironment()
|
|
||||||
*/
|
|
||||||
public function __destruct()
|
|
||||||
{
|
|
||||||
putenv('LDAPRC');
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue