Allow peer verification for command transports #1284
Conversation
oxzi
left a comment
oxzi
left a comment
There was a problem hiding this comment.
Thanks for this change and thanks for the review request.
So far, I have only taken a quick look and it seems to be a reasonable minimal fix for the issue. However, I am unsure about the usability.
| $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'), |
There was a problem hiding this comment.
| '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'), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fair enough. I just wanted the CA to be mentioned here as otherwise users would be confused.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 'required' => false, | ||
| 'label' => t('Verify Peer'), | ||
| 'description' => t('Path to a certificate file to verify the Icinga master\'s api certificate'), | ||
| 'placeholder' => t('Leave empty to disable peer verification'), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Stalled for now. Secure by default won't be a thing to not break existing setups. In case we ever decide to apply TOFU logic on the full product stack, we will reconsider this. The input however should be a textarea, allowing to paste a certificate in. |
|
And the input should support paths as well, similar to #1089 |
fixes #1048