Skip to content

Add switch to configure database SSL connection#80

Merged
kbarber merged 3 commits into
puppetlabs:masterfrom
stdietrich:add_ssl_switch
Oct 19, 2013
Merged

Add switch to configure database SSL connection#80
kbarber merged 3 commits into
puppetlabs:masterfrom
stdietrich:add_ssl_switch

Conversation

@stdietrich

Copy link
Copy Markdown
Contributor

Hi,

this patch adds a switch, to optionally use SSL for PostgreSQL database connections. This adds only the switch, setting up the appropriate trust- and keystores has to be done outside of this module (e.g. puppetlabs-java_ks).
By default, all database connections will be without SSL.
I have disabled the database connection validation, if SSL is enabled. The psql command expects the certificates in the .postgresql folder of the executing user. Otherwise you would have to store the certificates multiple times on the machine.

Regards,
Stefan

@kbarber-jenkins-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@stdietrich

Copy link
Copy Markdown
Contributor Author

I have updated my changes, because the if statement to disable the database connection validation was wrong.

@kbarber

kbarber commented Sep 25, 2013

Copy link
Copy Markdown
Contributor

test this please

@kbarber-jenkins-bot

Copy link
Copy Markdown

Merged build triggered. (Status: PENDING, Details: null)

@kbarber-jenkins-bot

Copy link
Copy Markdown

Merged build started. (Status: PENDING, Details: http://box.bob.sh:8080/job/puppetlabs-puppetdb/253/)

@kbarber-jenkins-bot

Copy link
Copy Markdown

Merged build finished. (Status: FAILURE, Details: http://box.bob.sh:8080/job/puppetlabs-puppetdb/253/)

@kbarber

kbarber commented Sep 25, 2013

Copy link
Copy Markdown
Contributor

@stdietrich this is throwing errors on the rspec-system tests, and the travis tests seem to have blocked also. How about you rebase, and push your changes again and lets see if this clears things. When you push again to this PR it should start the system tests automatically, so make sure you check out the results to see if they are still failing and/or fix them.

@stdietrich

Copy link
Copy Markdown
Contributor Author

Yes, you are right. Just rebased my changes on latest master and run the system spec tests. They are indeed failing, I will have a look into this and fix the errors.

@stdietrich

Copy link
Copy Markdown
Contributor Author

Found the culprit.
"?ssl=false" does not work as expected, although I remember it working.
The spec:system tests are now running without errors. Please have look at the changes :)

@kbarber

kbarber commented Sep 26, 2013

Copy link
Copy Markdown
Contributor

retest this please

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this because validate_db_connection doesn't support SSL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

validate_db_connection does support SSL, because it uses psql. psql expects certificates to be located in a .postgresql folder inside the home folder of the executing user.
Now one would have to store the certificates multiple times on the machine, once for the validate_db_connection and another time inside the JKS.
To circumvent this, I disabled DB validation if SSL is enabled.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So I like this feature, I really do - but can't help but think this is incomplete because of our inability to use validate_db_connection. I do however realise that getting everything working is going to be a hassle, but it feels like a bad line to draw. Maybe I'm just being pedantic, but I would like to find a better incremental release of this feature somehow. Let me ponder some more, but I don't suppose you have any ideas on doing something that is a stop-gap?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you insist on validate_db_connection, then there is only one way to solve this :)
The paths to the certificates are hardcoded in the libpq C library, which is used by psql.
So, you have to store the certificates multiple times or symlinking the contents of the .postgres folder to a different location.
This should probably handled by the puppetlabs-postgresql module.
Let me know, if you come up with any other solution.

@kbarber-jenkins-bot

Copy link
Copy Markdown

Merged build triggered. (Status: PENDING, Details: null)

@kbarber-jenkins-bot

Copy link
Copy Markdown

Merged build started. (Status: PENDING, Details: http://box.bob.sh:8080/job/puppetlabs-puppetdb/256/)

@kbarber-jenkins-bot

Copy link
Copy Markdown

Merged build finished. (Status: SUCCESS, Details: http://box.bob.sh:8080/job/puppetlabs-puppetdb/256/)

@kbarber

kbarber commented Oct 4, 2013

Copy link
Copy Markdown
Contributor

The unit tests are failing atm @stdietrich

@stdietrich

Copy link
Copy Markdown
Contributor Author

@kbarber
Yes, but they are even failing for the current master branch without my changes. This looks like an issue with the current rewrite of the puppetlabs-postgresql module. Configuration of the server with config_hash is no longer supported to configure the server.

@kbarber

kbarber commented Oct 18, 2013

Copy link
Copy Markdown
Contributor

@stdietrich this should be better now, I've just asked Travis to re-run your tests.

Comment thread manifests/server.pp

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, should have picked this up before, this needs to be documented now in README.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also as per convention it needs to be available in the main class puppetdb as well, I think that is what is normally documented. For good or bad.

PostgreSQL supports secure connections through SSL. For PuppetDB
to connect with SSL, "?ssl=true" has to be specified on the
connection string.

This patch adds such a switch, by default PuppetDB will not use SSL
to connect to the database.
@stdietrich

Copy link
Copy Markdown
Contributor Author

@kbarber
I have updated my changes according to your comments and rebased onto latest master.
Unit and system tests are working for me without errors.
Did you come up with a solution regarding the database validation problem?

@kbarber

kbarber commented Oct 19, 2013

Copy link
Copy Markdown
Contributor

retest this please

@kbarber-jenkins2

Copy link
Copy Markdown

Merged build triggered.

@kbarber-jenkins2

Copy link
Copy Markdown

Merged build started.

@kbarber-jenkins2

Copy link
Copy Markdown

Merged build finished.

@kbarber-jenkins2

Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results: http://box.bob.sh:8080/job/puppetlabs-puppetdb/277/

@kbarber

kbarber commented Oct 19, 2013

Copy link
Copy Markdown
Contributor

I've raised this to cover the postgresql change:

puppetlabs/puppetlabs-postgresql#287

But I'm going to allow this merge, since it gets us partly there and might provide enough impetus for someone to fix it completely.

kbarber added a commit that referenced this pull request Oct 19, 2013
Add switch to configure database SSL connection
@kbarber kbarber merged commit c09ac02 into puppetlabs:master Oct 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants