> On March 28, 2017, 7:57 p.m., Robert Levas wrote: > > ambari-common/src/main/python/ambari_commons/network.py > > Lines 65-66 (original), 64-65 (patched) > > <https://reviews.apache.org/r/58006/diff/1/?file=1677953#file1677953line66> > > > > Maybe there should be a flag so that the caller of this can indicate > > whether they want this to fail or not. It may be desirable to not allow > > the connection if the server is not trusted. > > > > Also, this function will return `ssl.PROTOCOL_SSLv23` > > no matter what, so the caller has no indication that an error may have > > occurred. > > Aravindan Vijayan wrote: > I will make the check certificate step configurable. > > Sid Wagle wrote: > I am not sure about making cert check failure/sucess configurable. It > just sounds like an overkill. Why not check and warn if there is an issue > with the check. The daemon launching with sucess or failure will in any case > determine if the cert is valid and configured correctly. There is effectively > no stack script that does cert checks like this :-) > > Robert Levas wrote: > The daemon (serve-side of this equation?) cannot be trusted to to verify > it's own cert, this is why the client should do it. I think (basically) > silently warning the user the server cannot be trusted can be a problem in > some situations, depending on the level of paranoia a user may have. And if a > user has gone through the trouble of getting a _real_ SSL certificate (as > opposed to a self-signed SSL certificate), then they probably want to know if > the server's cert is not valid. > > Aravindan Vijayan wrote: > IMO, we should go with either printing warning, or failing fast. Making > it configurable just adds one more step of complexity which the user might > not really use.
How about failing fast and allow the caller to catch the exception and either log and continue or log and fail? I think this is bascially the functionailty that we currently have - which is being changed by this patch. It appears that `ambari_commons.network.get_http_connection` conditionally checks the server's SSL certificate if the `ca_certs` value is not `None`. So the caller is already indicating whether a validation check is required or not. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58006/#review170350 ----------------------------------------------------------- On March 28, 2017, 6:03 p.m., Aravindan Vijayan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58006/ > ----------------------------------------------------------- > > (Updated March 28, 2017, 6:03 p.m.) > > > Review request for Ambari, Dmytro Sen, Robert Levas, Sumit Mohanty, and Sid > Wagle. > > > Bugs: AMBARI-20600 > https://issues.apache.org/jira/browse/AMBARI-20600 > > > Repository: ambari > > > Description > ------- > > EXCEPTION TRACE > > File > "/var/lib/ambari-agent/cache/common-services/AMBARI_METRICS/0.1.0/package/scripts/metrics_grafana_util.py", > line 235, in create_grafana_admin_pwd > response = perform_grafana_get_call(GRAFANA_USER_URL, serverCall1) > File > "/var/lib/ambari-agent/cache/common-services/AMBARI_METRICS/0.1.0/package/scripts/metrics_grafana_util.py", > line 59, in perform_grafana_get_call > grafana_https_enabled, ca_certs) > File "/usr/lib/python2.6/site-packages/ambari_commons/network.py", line 49, > in get_http_connection > ssl_version = check_ssl_certificate_and_return_ssl_version(host, port, > ca_certs) > File "/usr/lib/python2.6/site-packages/ambari_commons/network.py", line 66, > in check_ssl_certificate_and_return_ssl_version > .format(host, port, ca_certs, str(ssl_error))) > resource_management.core.exceptions.Fail: Failed to verify the SSL > certificate for https://<host>:3000 with CA certificate in > /etc/security/ssl/test.cert. Error : [Errno 1] _ssl.c:492: error:14090086:SSL > routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed > > > PROBLEM > The Grafana util script makes HTTPS calls with the server endpoint to create > datasource, dashboards etc. For this call, it validates the server's > certificate with the CA certificate using the > https://docs.python.org/2/library/ssl.html#ssl.get_server_certificate call. > This call checks the certificate validity against a root certificate list. > The Grafana cert file (/configurations/ams-grafana-ini/cert_file) can be used > both by the Grafana server to start up in HTTPS as well as in this validation > step if the cert file is not a leaf certificate (for example a self signed > certificate). If there is a CA which issued the certificate for Grafana > HTTPS, then the ca bundle must be used to validate the server's certificate. > > FIX > Added a new parameter that takes in the ca_cert, defaulting to the cert file. > Grafana start should not fail if we are not able to validate the certificate, > but able to make HTTPS calls to the server. We will print out a warning > statement instead. > > > Diffs > ----- > > ambari-common/src/main/python/ambari_commons/network.py 6ab92b2 > > ambari-server/src/main/resources/common-services/AMBARI_METRICS/0.1.0/configuration/ams-grafana-ini.xml > b4570b7 > > ambari-server/src/main/resources/common-services/AMBARI_METRICS/0.1.0/package/scripts/metrics_grafana_util.py > a6a9779 > > ambari-server/src/main/resources/common-services/AMBARI_METRICS/0.1.0/package/scripts/params.py > 3276cc1 > > > Diff: https://reviews.apache.org/r/58006/diff/1/ > > > Testing > ------- > > Manually tested. > Python unit tests passed. > > > Thanks, > > Aravindan Vijayan > >
