search

dremio / dbt-dremio

dbt (data build tool) adapter for the Dremio
Apache License 2.0
44 stars 21 forks source link

Fix insecure request warning #146

Closed ravjotbrar closed 1 year ago

ravjotbrar commented 1 year ago

Summary

Fixed the insecure request warning by ensuring ssl verification is enabled in all of our api calls. In addition, I've added a new config option called verify_ssl so that this behaviour is configurable by the user.

Description

The new config option is set to true by default. I've added this option to the Parameters class, which we already pass to each api call, instead of passing ssl_verify as a separate option.

Test Results

Ran all functional tests against Dremio Cloud:

Changelog

Related Issue

https://github.com/dremio/dbt-dremio/issues/142

jlarue26 commented 1 year ago

@ravjotbrar how does the user change this new variable? Do you expect this to be an option when creating a profile when calling dbt init?

Should there be any unit tests added?

ravjotbrar commented 1 year ago

@ravjotbrar how does the user change this new variable? Do you expect this to be an option when creating a profile when calling dbt init?

Yup, I've added it in as part of the credentials. If nothing is specified it'll default to true.

Should there be any unit tests added?

Good point. I believe the component test that checks to see if the profile config is set needs to be updated.

ravjotbrar commented 1 year ago

@ArgusLi

Would it be better for us to roll verify_ssl into DremioAuthentication? Verifying SSL is technically authentication as it is server authentication and it would reduce the number of verify_ssl values in methods by 2 instances (I believe).

This is a great suggestion and one I do agree with, just not entirely sure if I should redesign and reimplement. @jlarue26 thoughts on whether this is worth the effort?

Should verify_ssl be always on when use_ssl is on? Certificate validation should always be on as it could be a hazard to have it off. Is just defaulting it to on enough?

What would you suggest here?

jlarue26 commented 1 year ago

Should verify_ssl be always on when use_ssl is on? Certificate validation should always be on as it could be a hazard to have it off. Is just defaulting it to on enough?

Actually I think we should default it to on and remove the option from profile_template. We can update the Wiki for instructions to not verify certificates by manually adding use_ssl=false in the profile.

Would it be better for us to roll verify_ssl into DremioAuthentication? Verifying SSL is technically authentication as it is server authentication and it would reduce the number of verify_ssl values in methods by 2 instances (I believe).

What would the effort be to make this change?

ravjotbrar commented 1 year ago

What would the effort be to make this change?

To move verify_ssl into dremio authentication, removing it as a profile option, testing, and re-review should take at least half a day of effort. But worst case, we should have this change merged in by Monday.

ArgusLi commented 1 year ago

Actually I think we should default it to on and remove the option from profile_template. We can update the Wiki for instructions to not verify certificates by manually adding use_ssl=false in the profile.

Do you mean by manually adding verify_ssl=false in the profile? use_ssl is for enabling/disabling encryption. I like this idea - disabling certificate validation is such a niche case that it makes sense to hide it from most users.