Require SSL connection to database if MariaDB SSL is configured #1289

jsuchome · 2017-09-20T10:42:20Z

With this settings, non-SSL access is forbidden for the affected
users.

This is based on @s-t-e-v-e-n-k patch.

NOTE:

db_maker access does not work for insecure connection, requires this fix in rubygem-mysql2:
Make sure ssl is enabled if only :sslverify is set brianmario/mysql2#889
currently present in our rubygem package

rhafer · 2017-09-20T15:48:21Z

chef/cookbooks/database/libraries/provider_database_mysql_user.rb

+            if new_resource.connection[:ssl][:insecure]
+              client_options[:sslverify] = false
+            else
+              client_options[:sslca] = new_resource.connection[:ssl][:ca_certs]


I think you explicitly need to set client_options[:sslverify] = true at least the mysqlclient docs mention that it is off by default. (I didn't try that though)

Ah, true indeed

rhafer

The change itself looks good now. But we can't merge it before we have feedback on the ruby-mysql2 patch referenced above. Or we need to rework the code to disable ssl for the ruby part when certificate verification is not possible.

rhafer

Looks good. Though the comments from the previous review still apply.

jsuchome · 2017-09-21T15:28:35Z

Rebased (no code change)

rhafer

This seems to be missing keystone at least.

rhafer

This seems to be missing keystone at least.

jsuchome · 2017-09-22T12:46:30Z

This seems to be missing keystone at least.

Added

jsuchome · 2017-09-25T10:09:39Z

@rhafer as your patch is currently living in build service, this PR could be merged actually.

jsuchome · 2017-10-02T08:23:26Z

Current blocker is neutron behavior, see bug https://bugzilla.suse.com/show_bug.cgi?id=1060405

rsalevsky · 2017-10-02T12:31:47Z

@jsuchome Would splitting the neutron part in a separate PR help unblocking this PR?

houndci-bot · 2017-10-02T12:43:23Z

chef/cookbooks/neutron/recipes/database.rb

@@ -63,6 +63,8 @@
    host "%"
    privileges db_settings[:privs]
    provider db_settings[:user_provider]
+# FIXME: does not work now, see bsc#1060405
+#    require_ssl db_settings[:connection][:ssl][:enabled]


Layout/CommentIndentation: Incorrect indentation detected (column 0 instead of 4).

houndci-bot · 2017-10-02T12:43:24Z

chef/cookbooks/neutron/recipes/database.rb

@@ -63,6 +63,8 @@
    host "%"
    privileges db_settings[:privs]
    provider db_settings[:user_provider]
+# FIXME: does not work now, see bsc#1060405
+#    require_ssl db_settings[:connection][:ssl][:enabled]


Layout/CommentIndentation: Incorrect indentation detected (column 0 instead of 4).

jsuchome · 2017-10-02T12:45:11Z

@rsalevsky ok, I've removed neutron part in last commit ... but I'm not sure how severe the problem is, maybe we're gonna have to rework it again

With this settings, non-SSL access is forbidden for the affected users.

Old review

houndci-bot · 2017-11-02T15:37:06Z

chef/cookbooks/horizon/recipes/server.rb

@@ -333,6 +333,7 @@
    only_if { !ha_enabled || CrowbarPacemakerHelper.is_cluster_founder?(node) }
 end

+database_ssl = db_settings[:connection][:ssl][:enabled] && !db_settings[:connection][:ssl][:insecure]


Metrics/LineLength: Line is too long. [101/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

s-t-e-v-e-n-k

I am happy with these changes, on the proviso that the SSL CI job passes as well.

rhafer · 2017-11-03T14:51:01Z

chef/cookbooks/horizon/recipes/server.rb

@@ -333,6 +333,7 @@
    only_if { !ha_enabled || CrowbarPacemakerHelper.is_cluster_founder?(node) }
 end

+database_ssl = db_settings[:connection][:ssl][:enabled] && !db_settings[:connection][:ssl][:insecure]


Please add a comment with an explanation why we're not enabling ssl for horizon in "insecure" mode.

houndci-bot · 2017-11-03T15:05:36Z

chef/cookbooks/horizon/recipes/server.rb

@@ -333,6 +333,9 @@
    only_if { !ha_enabled || CrowbarPacemakerHelper.is_cluster_founder?(node) }
 end

+# We do not require SSL connectopn for horizon user in case of insecure DB setup,
+# because horizon's django uses a library (MySQLdb) that does not support insecure connection.
+database_ssl = db_settings[:connection][:ssl][:enabled] && !db_settings[:connection][:ssl][:insecure]


Metrics/LineLength: Line is too long. [101/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

houndci-bot · 2017-11-03T15:05:37Z

chef/cookbooks/horizon/recipes/server.rb

@@ -333,6 +333,9 @@
    only_if { !ha_enabled || CrowbarPacemakerHelper.is_cluster_founder?(node) }
 end

+# We do not require SSL connectopn for horizon user in case of insecure DB setup,
+# because horizon's django uses a library (MySQLdb) that does not support insecure connection.
+database_ssl = db_settings[:connection][:ssl][:enabled] && !db_settings[:connection][:ssl][:insecure]


Metrics/LineLength: Line is too long. [101/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

Would you mind addressing this as well?

Also, do not require SSL connection for insecure setup. It seems that MySQLdb library used by django is not able to start SSL connection without proper certificate verification.

jsuchome added the needs backport to SOC7 (stable/4.0) label Sep 20, 2017

jsuchome requested a review from s-t-e-v-e-n-k September 20, 2017 10:42

jsuchome mentioned this pull request Sep 20, 2017

mysql: SSL setup for client-server traffic #1280

Merged

jsuchome added the do not merge yet label Sep 20, 2017

jsuchome force-pushed the require-ssl branch 2 times, most recently from 8268b62 to b516726 Compare September 20, 2017 12:03

jsuchome requested review from rhafer and rsalevsky September 20, 2017 12:04

nicolasbock previously approved these changes Sep 20, 2017

View reviewed changes

rhafer suggested changes Sep 20, 2017

View reviewed changes

jsuchome dismissed nicolasbock’s stale review via c14aca2 September 20, 2017 16:01

jsuchome force-pushed the require-ssl branch from b516726 to c14aca2 Compare September 20, 2017 16:01

rhafer previously approved these changes Sep 21, 2017

View reviewed changes

jsuchome dismissed rhafer’s stale review via 9801d33 September 21, 2017 11:13

rhafer previously approved these changes Sep 21, 2017

View reviewed changes

rsalevsky previously approved these changes Sep 21, 2017

View reviewed changes

jsuchome dismissed stale reviews from rsalevsky and rhafer via a806f8c September 21, 2017 15:28

jsuchome force-pushed the require-ssl branch from 9801d33 to a806f8c Compare September 21, 2017 15:28

jsuchome force-pushed the require-ssl branch from a806f8c to 3209d91 Compare September 22, 2017 09:05

rsalevsky previously approved these changes Sep 22, 2017

View reviewed changes

rhafer reviewed Sep 22, 2017

View reviewed changes

rhafer previously requested changes Sep 22, 2017

View reviewed changes

jsuchome dismissed rsalevsky’s stale review via f1a9a17 September 22, 2017 12:46

jsuchome force-pushed the require-ssl branch from 3209d91 to f1a9a17 Compare September 22, 2017 12:46

jsuchome removed the do not merge yet label Sep 25, 2017

jsuchome requested a review from rsalevsky September 25, 2017 16:09

jsuchome force-pushed the require-ssl branch from f1a9a17 to 9f9fdbc Compare October 2, 2017 12:43

houndci-bot reviewed Oct 2, 2017

View reviewed changes

jsuchome force-pushed the require-ssl branch from 9f9fdbc to 4f463a0 Compare October 2, 2017 12:44

dirkmueller added needs backport to SOC7 and removed needs backport to SOC7 (stable/4.0) labels Oct 5, 2017

rhafer added the wip label Oct 20, 2017

jsuchome added 3 commits November 2, 2017 09:10

database: Support SSL connection for db_maker user from ruby library

c27bb4c

all: Require SSL connection to database if MariaDB SSL is configured

ffa6a3f

With this settings, non-SSL access is forbidden for the affected users.

mysql: Do not start SSL connection when bootstrapping

fd9cc22

jsuchome force-pushed the require-ssl branch from 4f463a0 to fd9cc22 Compare November 2, 2017 08:10

jsuchome removed the wip label Nov 2, 2017

houndci-bot reviewed Nov 2, 2017

View reviewed changes

s-t-e-v-e-n-k previously approved these changes Nov 3, 2017

View reviewed changes

rhafer suggested changes Nov 3, 2017

View reviewed changes

jsuchome dismissed s-t-e-v-e-n-k’s stale review via c08695c November 3, 2017 15:05

jsuchome force-pushed the require-ssl branch from 0ecc627 to c08695c Compare November 3, 2017 15:05

houndci-bot reviewed Nov 3, 2017

View reviewed changes

horizon: Adapt local_settings for SSL connection to database

763a89f

Also, do not require SSL connection for insecure setup. It seems that MySQLdb library used by django is not able to start SSL connection without proper certificate verification.

jsuchome force-pushed the require-ssl branch from c08695c to 763a89f Compare November 3, 2017 15:23

rhafer approved these changes Nov 3, 2017

View reviewed changes

rsalevsky approved these changes Nov 7, 2017

View reviewed changes

jsuchome merged commit 7604774 into crowbar:master Nov 7, 2017

jsuchome removed the needs backport to SOC7 label Nov 7, 2017

jsuchome mentioned this pull request Nov 7, 2017

[stable/4.0] Require SSL connection to database if MariaDB SSL is configured #1413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require SSL connection to database if MariaDB SSL is configured #1289

Require SSL connection to database if MariaDB SSL is configured #1289

jsuchome commented Sep 20, 2017 •

edited

Loading

rhafer Sep 20, 2017

jsuchome Sep 20, 2017

rhafer left a comment

rhafer left a comment

jsuchome commented Sep 21, 2017

rhafer left a comment

rhafer left a comment

jsuchome commented Sep 22, 2017

jsuchome commented Sep 25, 2017

jsuchome commented Oct 2, 2017

rsalevsky commented Oct 2, 2017

houndci-bot Oct 2, 2017

houndci-bot Oct 2, 2017

jsuchome commented Oct 2, 2017

houndci-bot Nov 2, 2017

s-t-e-v-e-n-k left a comment

rhafer Nov 3, 2017

jsuchome Nov 3, 2017

houndci-bot Nov 3, 2017

houndci-bot Nov 3, 2017

rhafer Nov 3, 2017

Require SSL connection to database if MariaDB SSL is configured #1289

Require SSL connection to database if MariaDB SSL is configured #1289

Conversation

jsuchome commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhafer left a comment

Choose a reason for hiding this comment

rhafer left a comment

Choose a reason for hiding this comment

jsuchome commented Sep 21, 2017

rhafer left a comment

Choose a reason for hiding this comment

rhafer left a comment

Choose a reason for hiding this comment

jsuchome commented Sep 22, 2017

jsuchome commented Sep 25, 2017

jsuchome commented Oct 2, 2017

rsalevsky commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuchome commented Oct 2, 2017

Choose a reason for hiding this comment

s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsuchome commented Sep 20, 2017 •

edited

Loading