You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/26 12:13:32 UTC

[GitHub] [incubator-superset] villebro opened a new pull request #9396: feat: add SSL certificate validation for Druid

villebro opened a new pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396
 
 
   ### CATEGORY
   
   Choose one
   
   - [ ] Bug Fix
   - [x] Enhancement (new features, refinement)
   - [ ] Refactor
   - [x] Add tests
   - [ ] Build / Development Environment
   - [x] Documentation
   
   ### SUMMARY
   This adds support for specifying SSL certificates on a per database basis.  This is done by adding a new field `server_cert` to the `dbs` table and using that in the `pydruid.connect()` call. Unfortunately Python doesn't yet support passing certificates either as strings or file-like objects (see this PR that has been 8 years in the making: https://bugs.python.org/issue16487 ), hence the certificate has to be persisted to disk ☹️ To avoid rewriting the certificate to disk every time it is used, the file path is made deterministic by naming it with the md5 of the certificate contents and only written to disk if it doesn't yet exist in temporary storage. A unit test has also been added to verify that only valid certificates are persisted to disk.
   
   ### Screenshot
   ![image](https://user-images.githubusercontent.com/33317356/77643753-97a9f000-6f68-11ea-81f6-da6722571fb7.png)
   
   ### TEST PLAN
   New unit tests + local testing
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [x] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### REVIEWERS
   @mistercrunch @john-bodley @dpgaspar 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398779928
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -959,3 +959,15 @@ def mutate_db_for_connection_test(database: "Database") -> None:
         :param database: instance to be mutated
         """
         return None
+
+    @staticmethod
+    def mutate_connection_args(
+        database: "Database", connect_args: Dict[str, Any]
+    ) -> None:
+        """
+        Some databases require passing additional non-standard parameters to database
+        connections, for example client certificates.
+
+        :param database: instance to be mutated
 
 Review comment:
   @mistercrunch I changed this so extras is parsed in `db_engine_specs`, so that we don't need to mutate the `Database` instance (this caused the mutations to get persisted in the db when saving the database).

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398641323
 
 

 ##########
 File path: superset/utils/core.py
 ##########
 @@ -1163,6 +1166,26 @@ def get_username() -> Optional[str]:
         return None
 
 
+def create_temporary_ssl_cert_file(certificate: str) -> str:
 
 Review comment:
   Good point. This was a remnant from when I thought these would really be temporary. I will update accordingly.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro merged pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro merged pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398616534
 
 

 ##########
 File path: superset/utils/core.py
 ##########
 @@ -1163,6 +1166,26 @@ def get_username() -> Optional[str]:
         return None
 
 
+def create_temporary_ssl_cert_file(certificate: str) -> str:
+    """
+    This creates a temporary certificate file that can be used to validate HTTPS
+    sessions. A certificate is only written to disk once; on subsequent calls,
+    only the path of the existing certificate is returned.
+
+    :param certificate: The contents of the certificate
+    :return: The path to the certificate file
+    """
+    filename = hashlib.md5(certificate.encode("utf-8")).hexdigest()
+    path = os.path.join(tempfile.gettempdir(), filename)
+    if not os.path.exists(path):
+        # Validate certificate prior to persisting to temporary directory
+        x509.load_pem_x509_certificate(certificate.encode("utf-8"), default_backend())
 
 Review comment:
   I'm open to suggestions on naming. `ValidationException`? `CertificateValidationException`?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398793310
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -959,3 +963,20 @@ def mutate_db_for_connection_test(database: "Database") -> None:
         :param database: instance to be mutated
         """
         return None
+
+    @staticmethod
+    def get_extra_params(database: "Database") -> Dict[str, Any]:
+        """
+        Some databases require adding elements to connection parameters,
+        like passing certificates to `extra`. This can be done here.
+
+        :param database: database instance from which to extract extras
+        """
 
 Review comment:
   nit: it's nice to add a `:raises:` on the docstring when a method can raise an exception

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398633382
 
 

 ##########
 File path: superset/utils/core.py
 ##########
 @@ -1163,6 +1166,26 @@ def get_username() -> Optional[str]:
         return None
 
 
+def create_temporary_ssl_cert_file(certificate: str) -> str:
+    """
+    This creates a temporary certificate file that can be used to validate HTTPS
+    sessions. A certificate is only written to disk once; on subsequent calls,
+    only the path of the existing certificate is returned.
+
+    :param certificate: The contents of the certificate
+    :return: The path to the certificate file
+    """
+    filename = hashlib.md5(certificate.encode("utf-8")).hexdigest()
+    path = os.path.join(tempfile.gettempdir(), filename)
 
 Review comment:
   Good idea, will add a config param.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398804830
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -959,3 +963,20 @@ def mutate_db_for_connection_test(database: "Database") -> None:
         :param database: instance to be mutated
         """
         return None
+
+    @staticmethod
+    def get_extra_params(database: "Database") -> Dict[str, Any]:
+        """
+        Some databases require adding elements to connection parameters,
+        like passing certificates to `extra`. This can be done here.
+
+        :param database: database instance from which to extract extras
+        """
 
 Review comment:
   Ah, missed that one, will update.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on issue #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#issuecomment-604707233
 
 
   Addressed overlooked comment by @dpgaspar and added error message when certificate is invalid:
   ![image](https://user-images.githubusercontent.com/33317356/77700643-3d874a00-6fbd-11ea-8e28-fd932a6e7267.png)
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] craig-rueda commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398626993
 
 

 ##########
 File path: superset/utils/core.py
 ##########
 @@ -1163,6 +1166,26 @@ def get_username() -> Optional[str]:
         return None
 
 
+def create_temporary_ssl_cert_file(certificate: str) -> str:
+    """
+    This creates a temporary certificate file that can be used to validate HTTPS
+    sessions. A certificate is only written to disk once; on subsequent calls,
+    only the path of the existing certificate is returned.
+
+    :param certificate: The contents of the certificate
+    :return: The path to the certificate file
+    """
+    filename = hashlib.md5(certificate.encode("utf-8")).hexdigest()
+    path = os.path.join(tempfile.gettempdir(), filename)
 
 Review comment:
   Can we make this dir configurable? Placing it in temp works, but users might want to be able to tune this. Maybe default to  temp?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398554699
 
 

 ##########
 File path: superset/utils/core.py
 ##########
 @@ -1163,6 +1166,26 @@ def get_username() -> Optional[str]:
         return None
 
 
+def create_temporary_ssl_cert_file(certificate: str) -> str:
+    """
+    This creates a temporary certificate file that can be used to validate HTTPS
+    sessions. A certificate is only written to disk once; on subsequent calls,
+    only the path of the existing certificate is returned.
+
+    :param certificate: The contents of the certificate
+    :return: The path to the certificate file
+    """
+    filename = hashlib.md5(certificate.encode("utf-8")).hexdigest()
+    path = os.path.join(tempfile.gettempdir(), filename)
+    if not os.path.exists(path):
+        # Validate certificate prior to persisting to temporary directory
+        x509.load_pem_x509_certificate(certificate.encode("utf-8"), default_backend())
 
 Review comment:
   Looking at the tests, I'm guessing this raises `ValueError` we could wrap and re-raise with a custom exception. Should `get_sqla_engine` catch these exceptions how are all these handled?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-io edited a comment on issue #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#issuecomment-604725271
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=h1) Report
   > Merging [#9396](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9eb5baddd6b647f60dbbf3c21fda0aca554a2f05&el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9396/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9396      +/-   ##
   ==========================================
   - Coverage   59.02%   59.01%   -0.02%     
   ==========================================
     Files         375      377       +2     
     Lines       12136    12162      +26     
     Branches     2993     3004      +11     
   ==========================================
   + Hits         7163     7177      +14     
   - Misses       4794     4805      +11     
   - Partials      179      180       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...c/dashboard/components/gridComponents/Markdown.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL01hcmtkb3duLmpzeA==) | `89.01% <0.00%> (-2.77%)` | :arrow_down: |
   | [superset-frontend/src/explore/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbHMuanN4) | `39.09% <0.00%> (-0.61%)` | :arrow_down: |
   | [superset-frontend/src/chart/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0LmpzeA==) | `10.41% <0.00%> (ø)` | |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (ø)` | |
   | [superset-frontend/src/welcome/Welcome.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3dlbGNvbWUvV2VsY29tZS5qc3g=) | `78.57% <0.00%> (ø)` | |
   | [superset-frontend/src/profile/components/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Byb2ZpbGUvY29tcG9uZW50cy9BcHAuanN4) | `100.00% <0.00%> (ø)` | |
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `66.05% <0.00%> (ø)` | |
   | [...perset-frontend/src/addSlice/AddSliceContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLmpzeA==) | `72.72% <0.00%> (ø)` | |
   | [...erset-frontend/src/profile/components/UserInfo.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Byb2ZpbGUvY29tcG9uZW50cy9Vc2VySW5mby5qc3g=) | `100.00% <0.00%> (ø)` | |
   | ... and [10 more](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=footer). Last update [9eb5bad...b1bf61d](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398803213
 
 

 ##########
 File path: superset/views/database/mixins.py
 ##########
 @@ -196,6 +204,9 @@ def _pre_add_update(self, database):
             check_sqlalchemy_uri(database.sqlalchemy_uri)
         self.check_extra(database)
         self.check_encrypted_extra(database)
+        database.server_cert = (
+            database.server_cert.strip() if database.server_cert else ""
+        )
 
 Review comment:
   "server_cert" is not being validated before persisting to the DB, are we relying on `testconn`. It would be nice to add `parse_ssl_cert` to the pre_add, pre_update hooks

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398701542
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -959,3 +959,15 @@ def mutate_db_for_connection_test(database: "Database") -> None:
         :param database: instance to be mutated
         """
         return None
+
+    @staticmethod
+    def mutate_connection_args(
+        database: "Database", connect_args: Dict[str, Any]
+    ) -> None:
+        """
+        Some databases require passing additional non-standard parameters to database
+        connections, for example client certificates.
+
+        :param database: instance to be mutated
 
 Review comment:
   Wait, connect_args is an attribute of the database isn't it?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398803213
 
 

 ##########
 File path: superset/views/database/mixins.py
 ##########
 @@ -196,6 +204,9 @@ def _pre_add_update(self, database):
             check_sqlalchemy_uri(database.sqlalchemy_uri)
         self.check_extra(database)
         self.check_encrypted_extra(database)
+        database.server_cert = (
+            database.server_cert.strip() if database.server_cert else ""
+        )
 
 Review comment:
   "server_cert" is not being validated before persisting to the DB, are we relying on `testconn`?. It would be nice to add `parse_ssl_cert` to the pre_add, pre_update hooks

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-io commented on issue #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#issuecomment-604725271
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=h1) Report
   > Merging [#9396](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9eb5baddd6b647f60dbbf3c21fda0aca554a2f05&el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9396/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9396      +/-   ##
   ==========================================
   - Coverage   59.02%   59.01%   -0.02%     
   ==========================================
     Files         375      376       +1     
     Lines       12136    12162      +26     
     Branches     2993     3004      +11     
   ==========================================
   + Hits         7163     7177      +14     
   - Misses       4794     4805      +11     
   - Partials      179      180       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...c/dashboard/components/gridComponents/Markdown.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL01hcmtkb3duLmpzeA==) | `89.01% <0.00%> (-2.77%)` | :arrow_down: |
   | [superset-frontend/src/explore/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbHMuanN4) | `38.80% <0.00%> (-0.90%)` | :arrow_down: |
   | [superset-frontend/src/welcome/Welcome.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3dlbGNvbWUvV2VsY29tZS5qc3g=) | `78.57% <0.00%> (ø)` | |
   | [superset-frontend/src/profile/components/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Byb2ZpbGUvY29tcG9uZW50cy9BcHAuanN4) | `100.00% <0.00%> (ø)` | |
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `66.05% <0.00%> (ø)` | |
   | [...perset-frontend/src/addSlice/AddSliceContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLmpzeA==) | `72.72% <0.00%> (ø)` | |
   | [...erset-frontend/src/profile/components/UserInfo.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Byb2ZpbGUvY29tcG9uZW50cy9Vc2VySW5mby5qc3g=) | `100.00% <0.00%> (ø)` | |
   | [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `54.62% <0.00%> (ø)` | |
   | [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `62.69% <0.00%> (ø)` | |
   | [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `11.11% <0.00%> (ø)` | |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-superset/pull/9396/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=footer). Last update [9eb5bad...3d6345c](https://codecov.io/gh/apache/incubator-superset/pull/9396?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398537191
 
 

 ##########
 File path: superset/migrations/versions/b5998378c225_add_certificate_to_dbs.py
 ##########
 @@ -0,0 +1,49 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add certificate to dbs
+
+Revision ID: b5998378c225
+Revises: 72428d1ea401
+Create Date: 2020-03-25 10:49:10.883065
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "b5998378c225"
+down_revision = "72428d1ea401"
+
+from typing import Dict
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.dialects.postgresql.base import PGDialect
+from sqlalchemy_utils import EncryptedType
+
+
+def upgrade():
+    kwargs: Dict[str, str] = {}
+    bind = op.get_bind()
+    if isinstance(bind.dialect, PGDialect):
+        kwargs["postgresql_using"] = "encrypted_extra::bytea"
+    op.add_column(
+        "dbs",
+        sa.Column("server_cert", EncryptedType(sa.Text()), nullable=True, **kwargs),
 
 Review comment:
   Would it make sense, while we're at it, to create a client_cert field also?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on issue #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#issuecomment-604588387
 
 
   All comments should now be addressed.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398637363
 
 

 ##########
 File path: superset/db_engine_specs/druid.py
 ##########
 @@ -47,3 +49,18 @@ class DruidEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
     def alter_new_orm_column(cls, orm_col: "TableColumn") -> None:
         if orm_col.column_name == "__time":
             orm_col.is_dttm = True
+
+    @classmethod
+    def mutate_connection_args(
+        cls, database: "Database", connect_args: Dict[str, Any]
+    ) -> None:
+        """
+        Some databases require passing additional non-standard parameters to database
+        connections, for example client certificates.
+
+        :param database: instance to be mutated
 
 Review comment:
   See previous comment.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398793825
 
 

 ##########
 File path: superset/db_engine_specs/druid.py
 ##########
 @@ -47,3 +52,26 @@ class DruidEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
     def alter_new_orm_column(cls, orm_col: "TableColumn") -> None:
         if orm_col.column_name == "__time":
             orm_col.is_dttm = True
+
+    @staticmethod
+    def get_extra_params(database: "Database") -> Dict[str, Any]:
+        """
+        For Druid, the path to a SSL certificate is placed in `connect_args`.
+
+        :param database: database instance from which to extract extras
 
 Review comment:
   nit: it's nice to add a :raises: on the docstring when a method can raise an exception

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398635854
 
 

 ##########
 File path: superset/utils/core.py
 ##########
 @@ -1163,6 +1166,26 @@ def get_username() -> Optional[str]:
         return None
 
 
+def create_temporary_ssl_cert_file(certificate: str) -> str:
 
 Review comment:
   This doesn’t feel temporary to me. Maybe the method and docstring should be updated.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398804830
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -959,3 +963,20 @@ def mutate_db_for_connection_test(database: "Database") -> None:
         :param database: instance to be mutated
         """
         return None
+
+    @staticmethod
+    def get_extra_params(database: "Database") -> Dict[str, Any]:
+        """
+        Some databases require adding elements to connection parameters,
+        like passing certificates to `extra`. This can be done here.
+
+        :param database: database instance from which to extract extras
+        """
 
 Review comment:
   Ah, missed that one, will update.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398615225
 
 

 ##########
 File path: tests/utils_tests.py
 ##########
 @@ -1221,3 +1222,33 @@ def test_build_extra_filters(self):
         )
         expected = []
         self.assertEqual(extra_filters, expected)
+
+    def test_ssl_certificate_validation(self):
+        valid_certificate = """-----BEGIN CERTIFICATE-----
+MIIDnDCCAoQCCQCrdpcNPCA/eDANBgkqhkiG9w0BAQsFADCBjzELMAkGA1UEBhMC
+VVMxEzARBgNVBAgMCkNhbGlmb3JuaWExEjAQBgNVBAcMCVNhbiBNYXRlbzEPMA0G
+A1UECgwGUHJlc2V0MRMwEQYDVQQLDApTa3Vua3dvcmtzMRIwEAYDVQQDDAlwcmVz
+ZXQuaW8xHTAbBgkqhkiG9w0BCQEWDmluZm9AcHJlc2V0LmlvMB4XDTIwMDMyNjEw
+NTE1NFoXDTQwMDMyNjEwNTE1NFowgY8xCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApD
+YWxpZm9ybmlhMRIwEAYDVQQHDAlTYW4gTWF0ZW8xDzANBgNVBAoMBlByZXNldDET
+MBEGA1UECwwKU2t1bmt3b3JrczESMBAGA1UEAwwJcHJlc2V0LmlvMR0wGwYJKoZI
+hvcNAQkBFg5pbmZvQHByZXNldC5pbzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC
+AQoCggEBAKNHQZcu2L/6HvZfzy4Hnm3POeztfO+NJ7OzppAcNlLbTAatUk1YoDbJ
+5m5GUW8m7pVEHb76UL6Xxei9MoMVvHGuXqQeZZnNd+DySW/227wkOPYOCVSuDsWD
+1EReG+pv/z8CDhdwmMTkDTZUDr0BUR/yc8qTCPdZoalj2muDl+k2J3LSCkelx4U/
+2iYhoUQD+lzFS3k7ohAfaGc2aZOlwTITopXHSFfuZ7j9muBOYtU7NgpnCl6WgxYP
+1+4ddBIauPTBY2gWfZC2FeOfYEqfsUUXRsw1ehEQf4uxxTKNJTfTuVbdgrTYx5QQ
+jrM88WvWdyVnIM7u7/x9bawfGX/b/F0CAwEAATANBgkqhkiG9w0BAQsFAAOCAQEA
+XYLLk3T5RWIagNa3DPrMI+SjRm4PAI/RsijtBV+9hrkCXOQ1mvlo/ORniaiemHvF
+Kh6u6MTl014+f6Ytg/tx/OzuK2ffo9x44ZV/yqkbSmKD1pGftYNqCnBCN0uo1Gzb
+HZ+bTozo+9raFN7OGPgbdBmpQT2c+LG5n+7REobHFb7VLeY2/7BKtxNBRXfIxn4X
++MIhpASwLH5X64a1f9LyuPNMyUvKgzDe7jRdX1JZ7uw/1T//OHGQth0jLiapa6FZ
+GwgYUaruSZH51ZtxrJSXKSNBA7asPSBbyOmGptLsw2GTAsoBd5sUR4+hbuVo+1ai
+XeA3AKTX/OdYWJvr5YIgeQ==
+-----END CERTIFICATE-----"""
+        path = create_temporary_ssl_cert_file(valid_certificate)
+        self.assertIn("5b0df668ac310be3f15c489f303d7d01", path)
 
 Review comment:
   Good idea, will change it.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398637058
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -959,3 +959,15 @@ def mutate_db_for_connection_test(database: "Database") -> None:
         :param database: instance to be mutated
         """
         return None
+
+    @staticmethod
+    def mutate_connection_args(
+        database: "Database", connect_args: Dict[str, Any]
+    ) -> None:
+        """
+        Some databases require passing additional non-standard parameters to database
+        connections, for example client certificates.
+
+        :param database: instance to be mutated
 
 Review comment:
   Nit. Missing `connect_args` params.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398613816
 
 

 ##########
 File path: superset/db_engine_specs/druid.py
 ##########
 @@ -47,3 +49,18 @@ class DruidEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
     def alter_new_orm_column(cls, orm_col: "TableColumn") -> None:
         if orm_col.column_name == "__time":
             orm_col.is_dttm = True
+
+    @classmethod
+    def mutate_connection_args(
+        cls, database: "Database", connect_args: Dict[str, Any]
+    ) -> None:
+        """
+        Some databases require passing additional non-standard parameters to database
+        connections, for example client certificates.
+
+        :param database: instance to be mutated
+        """
+        if database.server_cert:
+            connect_args["scheme"] = "https"
+            path = utils.create_temporary_ssl_cert_file(database.server_cert)
+            connect_args["ssl_verify_cert"] = path
 
 Review comment:
   Good idea. This raises `ValueError` if it doesn't validate, and it could be really helpful to have a specific `Exception` for that case, e.g. in the `Test Connection` where I suspect most people will hit errors.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398552248
 
 

 ##########
 File path: tests/utils_tests.py
 ##########
 @@ -1221,3 +1222,33 @@ def test_build_extra_filters(self):
         )
         expected = []
         self.assertEqual(extra_filters, expected)
+
+    def test_ssl_certificate_validation(self):
+        valid_certificate = """-----BEGIN CERTIFICATE-----
+MIIDnDCCAoQCCQCrdpcNPCA/eDANBgkqhkiG9w0BAQsFADCBjzELMAkGA1UEBhMC
+VVMxEzARBgNVBAgMCkNhbGlmb3JuaWExEjAQBgNVBAcMCVNhbiBNYXRlbzEPMA0G
+A1UECgwGUHJlc2V0MRMwEQYDVQQLDApTa3Vua3dvcmtzMRIwEAYDVQQDDAlwcmVz
+ZXQuaW8xHTAbBgkqhkiG9w0BCQEWDmluZm9AcHJlc2V0LmlvMB4XDTIwMDMyNjEw
+NTE1NFoXDTQwMDMyNjEwNTE1NFowgY8xCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApD
+YWxpZm9ybmlhMRIwEAYDVQQHDAlTYW4gTWF0ZW8xDzANBgNVBAoMBlByZXNldDET
+MBEGA1UECwwKU2t1bmt3b3JrczESMBAGA1UEAwwJcHJlc2V0LmlvMR0wGwYJKoZI
+hvcNAQkBFg5pbmZvQHByZXNldC5pbzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC
+AQoCggEBAKNHQZcu2L/6HvZfzy4Hnm3POeztfO+NJ7OzppAcNlLbTAatUk1YoDbJ
+5m5GUW8m7pVEHb76UL6Xxei9MoMVvHGuXqQeZZnNd+DySW/227wkOPYOCVSuDsWD
+1EReG+pv/z8CDhdwmMTkDTZUDr0BUR/yc8qTCPdZoalj2muDl+k2J3LSCkelx4U/
+2iYhoUQD+lzFS3k7ohAfaGc2aZOlwTITopXHSFfuZ7j9muBOYtU7NgpnCl6WgxYP
+1+4ddBIauPTBY2gWfZC2FeOfYEqfsUUXRsw1ehEQf4uxxTKNJTfTuVbdgrTYx5QQ
+jrM88WvWdyVnIM7u7/x9bawfGX/b/F0CAwEAATANBgkqhkiG9w0BAQsFAAOCAQEA
+XYLLk3T5RWIagNa3DPrMI+SjRm4PAI/RsijtBV+9hrkCXOQ1mvlo/ORniaiemHvF
+Kh6u6MTl014+f6Ytg/tx/OzuK2ffo9x44ZV/yqkbSmKD1pGftYNqCnBCN0uo1Gzb
+HZ+bTozo+9raFN7OGPgbdBmpQT2c+LG5n+7REobHFb7VLeY2/7BKtxNBRXfIxn4X
++MIhpASwLH5X64a1f9LyuPNMyUvKgzDe7jRdX1JZ7uw/1T//OHGQth0jLiapa6FZ
+GwgYUaruSZH51ZtxrJSXKSNBA7asPSBbyOmGptLsw2GTAsoBd5sUR4+hbuVo+1ai
+XeA3AKTX/OdYWJvr5YIgeQ==
+-----END CERTIFICATE-----"""
+        path = create_temporary_ssl_cert_file(valid_certificate)
+        self.assertIn("5b0df668ac310be3f15c489f303d7d01", path)
 
 Review comment:
   Small suggestion, we could calculate the cert's MD5 here instead of relying on the hardcoded value. Would it make sense to place `valid_certificate` on the fixtures test folder?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398536762
 
 

 ##########
 File path: superset/db_engine_specs/druid.py
 ##########
 @@ -47,3 +49,18 @@ class DruidEngineSpec(BaseEngineSpec):  # pylint: disable=abstract-method
     def alter_new_orm_column(cls, orm_col: "TableColumn") -> None:
         if orm_col.column_name == "__time":
             orm_col.is_dttm = True
+
+    @classmethod
+    def mutate_connection_args(
+        cls, database: "Database", connect_args: Dict[str, Any]
+    ) -> None:
+        """
+        Some databases require passing additional non-standard parameters to database
+        connections, for example client certificates.
+
+        :param database: instance to be mutated
+        """
+        if database.server_cert:
+            connect_args["scheme"] = "https"
+            path = utils.create_temporary_ssl_cert_file(database.server_cert)
+            connect_args["ssl_verify_cert"] = path
 
 Review comment:
   Is it ok here to fail with `KeyError` ? If yes, we could create a specific exception wrap and re-raise then on the "abstract" for `mutate_connection_args` document `:raises: FooExcetion` 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398702794
 
 

 ##########
 File path: tests/utils_tests.py
 ##########
 @@ -1221,3 +1222,33 @@ def test_build_extra_filters(self):
         )
         expected = []
         self.assertEqual(extra_filters, expected)
+
+    def test_ssl_certificate_validation(self):
+        valid_certificate = """-----BEGIN CERTIFICATE-----
+MIIDnDCCAoQCCQCrdpcNPCA/eDANBgkqhkiG9w0BAQsFADCBjzELMAkGA1UEBhMC
+VVMxEzARBgNVBAgMCkNhbGlmb3JuaWExEjAQBgNVBAcMCVNhbiBNYXRlbzEPMA0G
+A1UECgwGUHJlc2V0MRMwEQYDVQQLDApTa3Vua3dvcmtzMRIwEAYDVQQDDAlwcmVz
+ZXQuaW8xHTAbBgkqhkiG9w0BCQEWDmluZm9AcHJlc2V0LmlvMB4XDTIwMDMyNjEw
+NTE1NFoXDTQwMDMyNjEwNTE1NFowgY8xCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApD
+YWxpZm9ybmlhMRIwEAYDVQQHDAlTYW4gTWF0ZW8xDzANBgNVBAoMBlByZXNldDET
+MBEGA1UECwwKU2t1bmt3b3JrczESMBAGA1UEAwwJcHJlc2V0LmlvMR0wGwYJKoZI
+hvcNAQkBFg5pbmZvQHByZXNldC5pbzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC
+AQoCggEBAKNHQZcu2L/6HvZfzy4Hnm3POeztfO+NJ7OzppAcNlLbTAatUk1YoDbJ
+5m5GUW8m7pVEHb76UL6Xxei9MoMVvHGuXqQeZZnNd+DySW/227wkOPYOCVSuDsWD
+1EReG+pv/z8CDhdwmMTkDTZUDr0BUR/yc8qTCPdZoalj2muDl+k2J3LSCkelx4U/
+2iYhoUQD+lzFS3k7ohAfaGc2aZOlwTITopXHSFfuZ7j9muBOYtU7NgpnCl6WgxYP
+1+4ddBIauPTBY2gWfZC2FeOfYEqfsUUXRsw1ehEQf4uxxTKNJTfTuVbdgrTYx5QQ
+jrM88WvWdyVnIM7u7/x9bawfGX/b/F0CAwEAATANBgkqhkiG9w0BAQsFAAOCAQEA
+XYLLk3T5RWIagNa3DPrMI+SjRm4PAI/RsijtBV+9hrkCXOQ1mvlo/ORniaiemHvF
+Kh6u6MTl014+f6Ytg/tx/OzuK2ffo9x44ZV/yqkbSmKD1pGftYNqCnBCN0uo1Gzb
+HZ+bTozo+9raFN7OGPgbdBmpQT2c+LG5n+7REobHFb7VLeY2/7BKtxNBRXfIxn4X
++MIhpASwLH5X64a1f9LyuPNMyUvKgzDe7jRdX1JZ7uw/1T//OHGQth0jLiapa6FZ
+GwgYUaruSZH51ZtxrJSXKSNBA7asPSBbyOmGptLsw2GTAsoBd5sUR4+hbuVo+1ai
+XeA3AKTX/OdYWJvr5YIgeQ==
+-----END CERTIFICATE-----"""
+        path = create_temporary_ssl_cert_file(valid_certificate)
+        self.assertIn("5b0df668ac310be3f15c489f303d7d01", path)
 
 Review comment:
   NIT: something like this could go in a `fixtures.py` module

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398611834
 
 

 ##########
 File path: superset/migrations/versions/b5998378c225_add_certificate_to_dbs.py
 ##########
 @@ -0,0 +1,49 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add certificate to dbs
+
+Revision ID: b5998378c225
+Revises: 72428d1ea401
+Create Date: 2020-03-25 10:49:10.883065
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "b5998378c225"
+down_revision = "72428d1ea401"
+
+from typing import Dict
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.dialects.postgresql.base import PGDialect
+from sqlalchemy_utils import EncryptedType
+
+
+def upgrade():
+    kwargs: Dict[str, str] = {}
+    bind = op.get_bind()
+    if isinstance(bind.dialect, PGDialect):
+        kwargs["postgresql_using"] = "encrypted_extra::bytea"
+    op.add_column(
+        "dbs",
+        sa.Column("server_cert", EncryptedType(sa.Text()), nullable=True, **kwargs),
 
 Review comment:
   I did but took it out, as `pydruid` doesn't support it yet. However, I opened a [PR](https://github.com/druid-io/pydruid/pull/201) for that, and will add the client cert field once that PR is merged.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398714482
 
 

 ##########
 File path: superset/db_engine_specs/base.py
 ##########
 @@ -959,3 +959,15 @@ def mutate_db_for_connection_test(database: "Database") -> None:
         :param database: instance to be mutated
         """
         return None
+
+    @staticmethod
+    def mutate_connection_args(
+        database: "Database", connect_args: Dict[str, Any]
+    ) -> None:
+        """
+        Some databases require passing additional non-standard parameters to database
+        connections, for example client certificates.
+
+        :param database: instance to be mutated
 
 Review comment:
   Yes, or more specifically it's in `database.extra`, which is in JSON format. The reason it's being passed as a separate argument is that it's already been parsed out of `extra`. We could optionally inject this into `extra`, too, but that might be more confusing, as then we'd have to `json.parse`, mutate and `json.dumps` again.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398549381
 
 

 ##########
 File path: superset/views/database/mixins.py
 ##########
 @@ -196,6 +204,9 @@ def _pre_add_update(self, database):
             check_sqlalchemy_uri(database.sqlalchemy_uri)
         self.check_extra(database)
         self.check_encrypted_extra(database)
+        database.server_cert = (
+            database.server_cert.strip() if database.server_cert else ""
+        )
 
 Review comment:
   I think we should validate the certificate here also, it would give a better user experience, instead of failing when a connection is made

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9396: feat: add SSL certificate validation for Druid
URL: https://github.com/apache/incubator-superset/pull/9396#discussion_r398614238
 
 

 ##########
 File path: superset/views/database/mixins.py
 ##########
 @@ -196,6 +204,9 @@ def _pre_add_update(self, database):
             check_sqlalchemy_uri(database.sqlalchemy_uri)
         self.check_extra(database)
         self.check_encrypted_extra(database)
+        database.server_cert = (
+            database.server_cert.strip() if database.server_cert else ""
+        )
 
 Review comment:
   Good idea, I'll break it out into a separate util function.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] brachipa edited a comment on pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
brachipa edited a comment on pull request #9396:
URL: https://github.com/apache/incubator-superset/pull/9396#issuecomment-637577258


   Hi, 
   I know that this PR is already closed, but may I just raise issue if I can?
   
   I can't configure db connection with cert, it gave me exception: 
   ```
   ERROR:superset.views.core:Unexpected error connect() got an unexpected keyword argument 'ssl_verify_cert'
   ```
   I see in your code that you set in this field, the cert path. and I print the path into the logs and see the correct file name,I also check, and this file is saved in my file system.
   
   (also setting this, doesn't really help, it gives same error, seems like you ignore that property
   `"engine_params": {"connect_args": {"scheme": "https", "ssl_verify_cert": false}}`
   
   So why does it fail? seems like something sqlalchemy.
   Any idea?
   (LMK if you prefer to open issue on it)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] brachipa commented on pull request #9396: feat: add SSL certificate validation for Druid

Posted by GitBox <gi...@apache.org>.
brachipa commented on pull request #9396:
URL: https://github.com/apache/incubator-superset/pull/9396#issuecomment-637577258


   Hi, 
   I know that this PR is already closed, but may I just raise issue if I can?
   
   I can't configure db connection with cert, it gave me exception: 
   ```
   ERROR:superset.views.core:Unexpected error connect() got an unexpected keyword argument 'ssl_verify_cert'
   ```
   I see in your code that you set in this field, the cert path. and I print the path into the logs and see the correct file name,I also check, and this file is saved in my file system.
   
   (also setting this, doesn't really help, it gives same error, seems like you ignore that property
   `"engine_params": {"connect_args": {"scheme": "https", "ssl_verify_cert": false}}`
   
   So why does it fail? seems like something sqlalchemy.
   Any idea?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org