Skip to content

Commit fc104c9

Browse files
committed
fix(user_ldap): Harmonize parameter obfuscation and serialization accross logging methods
Debug log, profiler and ldap debug log had a different logic for sanitizing of parameters, aligning them. Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
1 parent 247a1e7 commit fc104c9

File tree

1 file changed

+31
-20
lines changed

1 file changed

+31
-20
lines changed

apps/user_ldap/lib/LDAP.php

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,16 @@
3737
use OCA\User_LDAP\DataCollector\LdapDataCollector;
3838
use OCA\User_LDAP\Exceptions\ConstraintViolationException;
3939
use OCP\IConfig;
40+
use OCP\ILogger;
4041
use OCP\Profiler\IProfiler;
42+
use OCP\Server;
4143
use Psr\Log\LoggerInterface;
4244

4345
class LDAP implements ILDAPWrapper {
4446
protected string $logFile = '';
4547
protected array $curArgs = [];
4648
protected LoggerInterface $logger;
49+
protected IConfig $config;
4750

4851
private ?LdapDataCollector $dataCollector = null;
4952

@@ -57,7 +60,8 @@ public function __construct(string $logFile = '') {
5760
$profiler->add($this->dataCollector);
5861
}
5962

60-
$this->logger = \OCP\Server::get(LoggerInterface::class);
63+
$this->logger = Server::get(LoggerInterface::class);
64+
$this->config = Server::get(IConfig::class);
6165
}
6266

6367
/**
@@ -316,42 +320,49 @@ protected function invokeLDAPMethod(string $func, ...$arguments) {
316320
return null;
317321
}
318322

323+
/**
324+
* Turn resources into string, and removes potentially problematic cookie string to avoid breaking logfiles
325+
*/
326+
private function sanitizeFunctionParameters(array $args): array {
327+
return array_map(function ($item) {
328+
if ($this->isResource($item)) {
329+
return '(resource)';
330+
}
331+
if (isset($item[0]['value']['cookie']) && $item[0]['value']['cookie'] !== '') {
332+
$item[0]['value']['cookie'] = '*opaque cookie*';
333+
}
334+
return $item;
335+
}, $args);
336+
}
337+
319338
private function preFunctionCall(string $functionName, array $args): void {
320339
$this->curArgs = $args;
321-
if(strcasecmp($functionName, 'ldap_bind') === 0 || strcasecmp($functionName, 'ldap_exop_passwd') === 0) {
340+
if (strcasecmp($functionName, 'ldap_bind') === 0 || strcasecmp($functionName, 'ldap_exop_passwd') === 0) {
322341
// The arguments are not key value pairs
323342
// \OCA\User_LDAP\LDAP::bind passes 3 arguments, the 3rd being the pw
324343
// Remove it via direct array access for now, although a better solution could be found mebbe?
325344
// @link https://github.com/nextcloud/server/issues/38461
326345
$args[2] = IConfig::SENSITIVE_VALUE;
327346
}
328347

329-
$this->logger->debug('Calling LDAP function {func} with parameters {args}', [
330-
'app' => 'user_ldap',
331-
'func' => $functionName,
332-
'args' => json_encode($args),
333-
]);
348+
if ($this->config->getSystemValue('loglevel') === ILogger::DEBUG) {
349+
/* Only running this if debug loglevel is on, to avoid processing parameters on production */
350+
$this->logger->debug('Calling LDAP function {func} with parameters {args}', [
351+
'app' => 'user_ldap',
352+
'func' => $functionName,
353+
'args' => $this->sanitizeFunctionParameters($args),
354+
]);
355+
}
334356

335357
if ($this->dataCollector !== null) {
336-
$args = array_map(function ($item) {
337-
if ($this->isResource($item)) {
338-
return '(resource)';
339-
}
340-
if (isset($item[0]['value']['cookie']) && $item[0]['value']['cookie'] !== "") {
341-
$item[0]['value']['cookie'] = "*opaque cookie*";
342-
}
343-
return $item;
344-
}, $this->curArgs);
345-
346358
$backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS);
347-
$this->dataCollector->startLdapRequest($functionName, $args, $backtrace);
359+
$this->dataCollector->startLdapRequest($functionName, $this->sanitizeFunctionParameters($args), $backtrace);
348360
}
349361

350362
if ($this->logFile !== '' && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) {
351-
$args = array_map(fn ($item) => (!$this->isResource($item) ? $item : '(resource)'), $this->curArgs);
352363
file_put_contents(
353364
$this->logFile,
354-
$functionName . '::' . json_encode($args) . "\n",
365+
$functionName . '::' . json_encode($this->sanitizeFunctionParameters($args)) . "\n",
355366
FILE_APPEND
356367
);
357368
}

0 commit comments

Comments
 (0)