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

MODULES-5483: Auth and FQDN certs => Fail #369

Merged
merged 5 commits into from
Aug 23, 2017

Conversation

ThoTischner
Copy link
Contributor

@ThoTischner ThoTischner commented May 16, 2017

This Pull request will add the following feature:

class {'::mongodb::server':
  ssl_invalid_hostnames => true
}

If SSL is enabled and the certs have the FQDN as DN, some mongo calls from this module will fail.
It is not possible to refactor the calls to use the FQDN, cause if AUTH is enabled the admin user will be created via 127.0.0.1 which will connect as admin by default without credentials.
Now it is possible to ignore such cert FQDN warnings.

The pull request #348 is also included for the ismaster fact and modified for the right selection of mongo parameters.

Copy link
Member

@hunner hunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, and needs a rebase. Looks like a great PR though!


if $?.success?
mongo_output = Facter::Core::Execution.exec("mongo --quiet #{mongoPort} --eval \"#{e}printjson(db.isMaster())\"")
JSON.parse(mongo_output.gsub(/\w+\(.+?\)/, '"foo"'))['ismaster'] ||= false
Facter::Core::Execution.exec("mongo --quiet #{ssl} #{sslkey} #{sslca} #{ipv6} #{mongoPort} --eval \"#{e}db.isMaster().ismaster\"")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this changes behavior such that previously it returned true or false and now would return 'true' or 'false' as strings. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic was implemented by #348 and i just copied it.
All i can say is that it works as expected:

[root@grldb01vp ~]# puppet facts | grep is_mas
    "mongodb_is_master": "false",
[root@grldb02vp ~]# puppet facts | grep is_mas
    "mongodb_is_master": "false",
[root@grldb03vp ~]# puppet facts | grep is_mas
    "mongodb_is_master": "true",

@@ -275,6 +275,7 @@ def self.mongo_command(command, host=nil, retries=4)

#Hack to avoid non-json empty sets
output = "{}" if output == "null\n"
output = "{}" if output == "\nnull\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this change? Could you describe it as part of the commit message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a mongo connection with --sslAllowInvalidHostnames is made, it prints a warning like:
2017-05-16T10:56:27.679+0200 warning: The server certificate does not match the host name 127.0.0.1
This warning gets filtered but there is still a newline and if the return is null, the content will be:
"\nnull\n"
So this just extends the already implemented logic to still work with the new feature.

@@ -73,6 +73,7 @@
$ssl_key = undef,
$ssl_ca = undef,
$ssl_weak_cert = false,
$ssl_invalid_hostnames = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are double spaces, and the = signs are not in alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #dc7754a

@@ -66,6 +66,7 @@
$ssl_key = $mongodb::server::ssl_key
$ssl_ca = $mongodb::server::ssl_ca
$ssl_weak_cert = $mongodb::server::ssl_weak_cert
$ssl_invalid_hostnames = $mongodb::server::ssl_invalid_hostnames
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #dc7754a

README.md Outdated
@@ -497,6 +497,10 @@ Default: <>
Set to true to disable mandatory SSL client authentication
Default: False

#####`ssl_invalid_hostnames`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put a space after the ##### to render the markdown correctly?

- sslAllowInvalidHostnames
AUTH and SSL support added
@ThoTischner
Copy link
Contributor Author

@tphoney Can you remove the needs rebase label, i rebased it to actual master.

@ThoTischner
Copy link
Contributor Author

Hi,

will this be merged soon?

@ThoTischner ThoTischner changed the title MODULES-2983: IPv6 for ismaster fact and sslAllowInvalidHostnames feature MODULES-5483: IPv6 for ismaster fact and sslAllowInvalidHostnames feature Aug 16, 2017
@ThoTischner ThoTischner changed the title MODULES-5483: IPv6 for ismaster fact and sslAllowInvalidHostnames feature MODULES-5483: Auth and FQDN certs => Fail Aug 16, 2017
@hunner hunner merged commit 31bd8fd into voxpupuli:master Aug 23, 2017
hunner added a commit that referenced this pull request Aug 23, 2017
MODULES-5483: Auth and FQDN certs => Fail
@wyardley wyardley added the enhancement New feature or request label Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants