Skip to content
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.

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

Remove the ssl_verification field from the Webhook model #15387

Open
jeremystretch opened this issue Mar 8, 2024 · 5 comments
Open

Remove the ssl_verification field from the Webhook model #15387

jeremystretch opened this issue Mar 8, 2024 · 5 comments
Labels
status: under review Further discussion is needed to determine this issue's scope and/or implementation type: deprecation Removal of existing functionality or behavior

Comments

@jeremystretch
Copy link
Member

Proposed Changes

Remove the ssl_verification field from the Webhook model. If enabled, this bypasses validation of the SSL certificate on the remote server.

Justification

Disabling certificate validation presents a legitimate security concern, as it exposes outgoing webhooks to interception by a third party.

Impact

NetBox will need to be able to validate the SSL certificate of any HTTPS-enabled remote receivers.

@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation type: deprecation Removal of existing functionality or behavior labels Mar 8, 2024
@peteeckel
Copy link
Contributor

peteeckel commented Mar 9, 2024

While I absolutely agree that SSL verification should be the default and enabled whenever possible, I am pretty sure that removing the option to disable SSL verification will cause a lot of problems in many environments.

Many of my customers, especially the smaller ones with understaffed IT departments, use their internal CAs or even self-signed certificates for internal systems (which in my experience are the majority of target servers for Webhooks), and don't have a sensible way of distributing their CAs root certificates to all systems. While that's certainly not optimal, it's unfortunately the status quo. The fact that it's not exactly straightforward to add your own CA for systems using requests and certifi does not help either, as they don't use the system trust store.

Removing the option of disabling SSL verification will break Webhooks in these cases, and the quick and dirty solution to this is to disable TLS altogether and communicate in clear text, which is much worse than unvalidated TLS.

My two cents: Display a visible warning when ssl_verification is disabled, but leave it in place.

@jeremystretch
Copy link
Member Author

jeremystretch commented Mar 9, 2024

The fact that it's not exactly straightforward to add your own CA for systems using requests and certifi does not help either, as they don't use the system trust store.

The webhook model also provides a mechanism for indicating the path to a specific CA file via its ca_file_path field, so I don't think we can consider this an argument against removing the ssl_verification field.

@SaschaSchwarzK
Copy link

I would agree that it should be a configurable setting, default can be false.
In general I am all for it to make things secure, but forcing security upon netbox users will lead to a lot of frustration.
In large companies the management of certificates quickly becomes a resource intense tasks which only few have successfully automated.
This change would then also require to have proper certificate management for every test and development instance.

@bitcollector1
Copy link

I always laugh at my teammates who ignore SSL in all their scripts because it's too hard ;)

@ITJamie
Copy link
Contributor

ITJamie commented May 6, 2024

If someone gets in a situation where a cert has expired (or is using a self-signed cert) on a remote server if there is no way to bypass this they might instead choose to send in plaintext over HTTP. which would be worse than sending traffic to a https endpoint with an expired or self-signed cert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: under review Further discussion is needed to determine this issue's scope and/or implementation type: deprecation Removal of existing functionality or behavior
Projects
None yet
Development

No branches or pull requests

5 participants