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

[dev.icinga.com #9725] Add SSL support for the IdoMysqlConnection feature #3178

Closed
icinga-migration opened this issue Jul 24, 2015 · 17 comments

Comments

Projects
None yet
1 participant
@icinga-migration
Copy link
Member

commented Jul 24, 2015

This issue has been migrated from Redmine: https://dev.icinga.com/issues/9725

Created by jorfermo on 2015-07-24 07:15:21 +00:00

Assignee: mfriedrich
Status: Resolved (closed on 2016-05-03 13:15:03 +00:00)
Target Version: 2.5.0
Last Update: 2016-05-03 13:15:03 +00:00 (in Redmine)

Backport?: Not yet backported
Include in Changelog: 1

There's no option to configure SSL for IDO-DB MYSQL

http://docs.icinga.org/icinga2/latest/doc/module/icinga2/chapter/object-types#objecttype-idomysqlconnection

Changesets

2016-02-08 12:13:07 +00:00 by leeclemens 43e1167

Add SSL support to MySQL connections via IDO

refs #9725

2016-02-09 10:49:48 +00:00 by jflach 49ddf92

Replace have_ssl with enable_ssl

refs #9725

2016-05-03 13:01:32 +00:00 by leeclemens 7050529

Add SSL support for the IdoMysqlConnection feature

fixes #9725

Signed-off-by: Michael Friedrich <michael.friedrich@netways.de>
@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2016

Updated by leeclemens on 2016-01-21 02:46:32 +00:00

I have started working on this

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2016

Updated by leeclemens on 2016-01-23 03:18:33 +00:00

#66

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2016

Updated by mfriedrich on 2016-01-26 17:59:04 +00:00

  • Status changed from New to Assigned
  • Assigned to set to jflach

Thanks.

@jean

Please review this and push it to a feature branch.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2016

Updated by leeclemens on 2016-02-01 23:14:47 +00:00

I just wanted to add as a reference, changes were suggested in #65 . Unfortunately I hadn't realized you could not reopen a PR after Force Push, so I created the current PR.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2016

Updated by jflach on 2016-02-08 12:11:16 +00:00

Please review this and push it to a feature branch.
Done, we still need to test it though

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2016

Updated by jflach on 2016-02-09 10:49:42 +00:00

I have reviewed the patch (I still couldn't really test it though).
Something I added after talking about it with gunnarbeutner is the enable_ssl parameter. In case someone wants to use SSL but does not need to add any CAs or keyfiles because those are already in the system.

I'd be happy to hear your thoughts about this leeclemens.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2016

Updated by leeclemens on 2016-03-06 19:16:49 +00:00

I am not sure how one would hope to use SSL without any CAs or keyfiles. The patch I submitted simply passes the required arguments to mysql_ssl_set(). That function does not provide any other options but those my change to Icinga simply passes through.

Are you suggesting increasing the scope of Icinga to search the OS for viable CA and private key files to potentially use and subsequently pass through to mysql_ssl_set() ? I think this should be left to a opt-in user configuration, as it is now. User didn't specify a key file, don't attempt SSL. User specified a specific key file, use that key file while attempting an SSL connection.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2016

Updated by leeclemens on 2016-03-09 17:59:04 +00:00

I just wanted to clarify, you can use SSL (not x509) by specifying a CA file or CA Path - but you still need to specify something (even system trusted roots can be stored in various locations). If you want to use x509, obviously you need to provide a private key and certificate signed. This functionality is built in to the mysql library(ies), not performed by any code here.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

Updated by mfriedrich on 2016-03-11 20:44:55 +00:00

  • Target Version set to 2.5.0
@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2016

Updated by leeclemens on 2016-03-26 17:38:18 +00:00

Is there any way this can be bumped to 2.4.5? There isn't much logic involved here other than permitting some addition config options and if any are set, passing them to mysql's connector - which does the actual processing of that information and either connects or does not.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2016

Updated by mfriedrich on 2016-04-11 21:42:05 +00:00

We generally try to avoid new config options and features in bugfix releases. That branch also needs a further review before merging it to master, some parts must be modified (e.g. removing the ido-mysql.conf additions, they normally irritate users who are not familiar with such extra attributes). Furthermore enable_ssl as local variable should be enableSsl, or similar.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

Updated by leeclemens on 2016-04-13 23:01:15 +00:00

I had only seen reference to schema changes and since this change should ignore the absence of new config options, I thought it could be squeezed in; but I understand not including config changes as well.

Thank you for pointing out "enable_ssl", I thought it was previously mentioned as a hypothetical (as to enable some auto-ssl functionality). By searching for previous references I just noticed the "Associated revisions" section with the usage of enable_ssl and it seems I (may have****) have misinterpreted what @jflach was proposing earlier. My apologies for that misunderstanding.

However, I'm not sure what value the additional variable adds, since

(ssl_key || ssl_cert || ssl_ca || ssl_capath || ssl_cipher)

will still be evaluated with or without the additional configuration option, and adding the enable_ssl configuration adds an additional call. Looking at the associated revision, it seems the only changes (ignoring .md) are to add a config option and add a function call and evaluate

&& !enable_ssl

If someone wants to use SSL: set any/all SSL variables as appropriate. If they set an SSL variable and it subsequently doesn't work (simply passed through to the MySQL driver...no logic involved), they shouldn't have set that SSL variable and an error will be displayed.

My question here in a technical forum: What value does the addition of "enable_ssl" configuration option add for simplicity of configuration or performance of execution? (I actually questioned this months ago, although under the wrong pretense, but never received a response). The same variables are evaluated either way and it only helps as some sort of 'safety' check, I suppose? (Must change two variables instead of one to set the CA Path.)

If I am missing something or not understanding something, please let me know.

**The usage of "but does not need to add any CAs or keyfiles" makes me think the user wouldn't need to specify any path to a trusted CA or keyfile, which the MySQL driver does not support. Adding driver-level support for MySQL's driver directly in to Icinga seems silly.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

Updated by leeclemens on 2016-04-13 23:02:56 +00:00

#66

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

Updated by leeclemens on 2016-04-16 19:00:28 +00:00

I have created a new branch which uses the concept of 'enable_ssl' as a configuration option, but avoids additional processing if it is set to false. Therefor, the only impact to users not using SSL should be a single reading of a configuration value and boolean test.

I generally do not check out other user's feature branches, so I'm not sure if there is a better way for me to continue my changes with jflach's "Associated revisions" here "Revision 49ddf92". With some guidance, I would be happy to re-create my latest update as a continuation of Revision 49ddf92.

That said, I have a new branch with a combination of these changes, which I believe addresses all of our concerns with this change:
#77

This would supersede #66

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

Updated by leeclemens on 2016-04-16 19:22:57 +00:00

Need to fix issues with that revision though, it seems.
I copied a new line treating it as a string, passing now.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

Updated by mfriedrich on 2016-05-03 13:14:48 +00:00

  • Subject changed from MYSQL IDO DB doesn't allow SSL connections to Add SSL support for the IdoMysqlConnection feature
  • Assigned to changed from jflach to mfriedrich

I've reviewed the changed and partially updated the code and documentation/example configuration (i.e. not adding those options everywhere as users just copy paste everything). I've further removed those additional checks when enable_ssl is not set but other options - that's up to the user explicitly enabling this option, but still having the other attributes set. Merged to master, thanks.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

Updated by leeclemens on 2016-05-03 13:15:03 +00:00

  • Status changed from Assigned to Resolved
  • Done % changed from 0 to 100

Applied in changeset 7050529.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.