Skip to content

Allow peer verification for command transports #1284

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow peer verification for command transports #1284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
746c3bb
ApiCommandTransport: Support proper peer verification
nilmerg Sep 29, 2025
bfb7734
ApiTransportForm: Allow to configure a certificate for peer verification
nilmerg Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions application/forms/ApiTransportForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@
use Icinga\Module\Icingadb\Command\Transport\CommandTransport;
use Icinga\Module\Icingadb\Command\Transport\CommandTransportException;
use Icinga\Web\Session;
use ipl\Validator\CallbackValidator;
use ipl\Validator\X509CertValidator;
use ipl\Web\Common\CsrfCounterMeasure;
use ipl\Web\Compat\CompatForm;
use Throwable;

class ApiTransportForm extends CompatForm
{
Expand All @@ -34,6 +37,41 @@ protected function assemble()
'description' => t('Hostname or address of the Icinga master')
]);

$this->addElement('text', 'caPath', [
'required' => false,
'label' => t('Verify Peer'),
'description' => t('Path to a certificate file to verify the Icinga master\'s api certificate'),
Copy link
Member

@oxzi oxzi Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'description' => t('Path to a certificate file to verify the Icinga master\'s api certificate'),
'description' => t('Path to a certificate or CA file to verify the Icinga 2 master\'s API certificate'),

Copy link
Member Author

@nilmerg nilmerg Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a CA file, it's a CA certificate, but then I figured it must not only be a certificate authority's certificate, but the peer's certificate. So any certificate, which is why I dropped CA from the description.

Copy link
Member

@oxzi oxzi Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I just wanted the CA to be mentioned here as otherwise users would be confused.

Copy link
Member Author

@nilmerg nilmerg Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused? By leaving it open for interpretation what actually is required? Don't users then use what they're familiar with and that's the CA's certificate?

Copy link
Member

@oxzi oxzi Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would imply that all users are familiar with their setup..

Furthermore, the certificate might be renewed automatically by Icinga 2. The CA, however, is static. Thus, for most setups, the Icinga 2 CA should be preferred.

'placeholder' => t('Leave empty to disable peer verification'),
Copy link
Member

@oxzi oxzi Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would result in an "insecure by default" workflow, unlike other SSL/TLS UIs, such as the Redis UI.

One neat idea would to allow some TOFU logic, storing the remote CA for the first connection.

Copy link
Member Author

@nilmerg nilmerg Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redis is insecure by default? You have to toggle TLS.

My gut feeling about the "TOFU logic" is that no-one expects Icinga Web to do such a thing, doesn't care and then wonders why something does not work anymore they never ever configured. Not saying that's bad, that's just not something Icinga offers in general.

Copy link
Member

@oxzi oxzi Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redis is insecure by default? You have to toggle TLS.

Yes. But if you enable TLS, validation is enforced, as specified.

However, the Icinga 2 API is TLS-only. Thus, I would argue it is "secure by default", unless you deliberately choose to misconfigure your TLS client. But if a configuration nudges a user to disable validation, then this would be, IMHO, an improper certificate validation (CWE-295) just with extra steps.

Edit: Your concerns about the TOFU logic seems valid. I just that this would ease setting this up.

Copy link
Member Author

@nilmerg nilmerg Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, problem is, it'd be a breaking change. Everyone upgrading won't be able to send any commands. Is this really that critical to risk setting up users?

Copy link
Member

@oxzi oxzi Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, when upgrading, the setup did no prior validation, so one can argue that there is no reason to change this. However, for new setups, starting with actually checking certificates would be nice.

'validators' => [new CallbackValidator(function (?string $value, CallbackValidator $validator) {
if (empty($value)) {
return true;
}

if (! file_exists($value) || ! is_readable($value)) {
$validator->addMessage(t('The specified certificate file does not exist or is not readable'));

return false;
}

try {
$cert = file_get_contents($value);
} catch (Throwable $e) {
$validator->addMessage(t('Failed to read certificate file: %s', $e->getMessage()));

return false;
}

$x509Validator = new X509CertValidator();
if (! $x509Validator->isValid($cert)) {
$validator->addMessages($x509Validator->getMessages());

return false;
}

return true;
})]
]);

// TODO: Don't rely only on browser validation
$this->addElement('number', 'port', [
'required' => true,
Expand Down
39 changes: 35 additions & 4 deletions library/Icingadb/Command/Transport/ApiCommandTransport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ class ApiCommandTransport implements CommandTransportInterface
*/
protected $host;

/**
* API Ca Path
*
* @var ?string
*/
protected $caPath;

/**
* API password
*
Expand Down Expand Up @@ -114,6 +121,30 @@ public function setHost(string $host): self
return $this;
}

/**
* Get the API Ca Path
*
* @return ?string
*/
public function getCaPath(): ?string
{
return $this->caPath;
}

/**
* Set the API Ca Path
*
* @param ?string $caPath
*
* @return $this
*/
public function setCaPath(?string $caPath): self
{
$this->caPath = $caPath;

return $this;
}

/**
* Get the API password
*
Expand Down Expand Up @@ -243,10 +274,10 @@ protected function sendCommand(IcingaApiCommand $command)
$response = (new Client(['timeout' => static::SEND_TIMEOUT]))
->post($this->getUriFor($command->getEndpoint()), [
'auth' => [$this->getUsername(), $this->getPassword()],
'verify' => $this->getCaPath() ?: false,
'headers' => $headers,
'json' => $command->getData(),
'http_errors' => false,
'verify' => false
'http_errors' => false
]);
} catch (GuzzleException $e) {
if (str_starts_with(ltrim($e->getMessage()), 'cURL error 28:')) {
Expand Down Expand Up @@ -324,9 +355,9 @@ public function probe()
$response = (new Client(['timeout' => static::SEND_TIMEOUT]))
->get($this->getUriFor(''), [
'auth' => [$this->getUsername(), $this->getPassword()],
'verify' => $this->getCaPath() ?: false,
'headers' => ['Accept' => 'application/json'],
'http_errors' => false,
'verify' => false
'http_errors' => false
]);
} catch (GuzzleException $e) {
throw new CommandTransportException(
Expand Down
1 change: 1 addition & 0 deletions library/Icingadb/Command/Transport/CommandTransportConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class CommandTransportConfig extends IniRepository
// API options
'host',
'port',
'caPath',
'username',
'password'
]
Expand Down
Loading