Add SSL mode capability by pg_parameters

[nabbar]

Hello,

I have trying to add ssl mode in you code to allow use the ssl connection into the cluster or with client throw proxy. The idea is to allow a master to have ssl on and if the master is ssl on, then the replication will use the sslmode=require. The ssl is enable by configuration in pg_parameters from init spec.

Add SSL mode capability :

• use spec config to set cert, key, ca files, ciphers list, ...
• check in pg parameters if cert, key, ... files are provided and if yes, check that files are reading capability
• set pg prameters ssl=on if config is ok, otherwise set it to off
• change the connection params to load the pg params ssl value or return disable
• add hostssl instead host in pg_hba.conf
• enable ssl connection from followed configuration
• enable ssl connection for sync processes

I have tryed to make less modification as possible, but this feature take some lines. I think my code can perfectible, but this is a first implementation of this feature.

For now, this feature is just allow value for connection as disable or require.
I can work for the other value (allow, prefer, verify...).

Regards,

Nicolas.

[sgotti]

@nabbar Thanks for the PR. Before doing a full code review I'd like to share some thoughts and questions:

• Currently (without this patch) it's already possible to connect from a client in ssl mode. Additionally, if your client want to use sslmode=verify-full, the cert CN must be set to the FQDN you use to connect to your proxies (the load balancer, keepalived or the k8s service). You can place the certs outside the PGDATA but also inside it since they'll be the same for the above reason.

  All of this isn't currently documented but it should be.

• The communication between the postgres instances is currently with ssl disabled.

So if I understand it correctly this patch tries to enable ssl communication between the instances right?

But I also noticed that it tries to disable hba access without ssl when ssl is enabled. About this change I'd do this only for instances communication leaving client communication possible also for non ssl.

P.S. Another future improvement (that could be done after this PR), if we want to be able to define different certificate paths for instance, will be to add new keeper options to set them (overriding pgparameters options).

[nabbar]

Hello,

Currently (without this patch) it's already possible to connect from a client in ssl mode. Additionally, if your client want to use sslmode=verify-full, the cert CN must be set to the FQDN you use to connect to your proxies (the load balancer, keepalived or the k8s service). You can place the certs outside the PGDATA but also inside it since they'll be the same for the above reason.

All of this isn't currently documented but it should be.

The communication between the postgres instances is currently with ssl disabled.

Yes, I am agree with you

So if I understand it correctly this patch tries to enable ssl communication between the instances right?

Yes exactly. It's not just for the client, I can do an evol to allow ssl and not ssl connection by client, but the major benefice of this PR is the ssl between instances of postgres.

But I also noticed that it tries to disable hba access without ssl when ssl is enabled. About this change I'd do this only for instances communication leaving client communication possible also for non ssl.

Yes, I have done this. And when I wrote this response, I understand that could be better... much much better.
We can up this system to allow the 2 mode of working (when ssl is enable) :

• a flag to specify the connection ssl mode
• mode ssl allow: pg_hba have 'host' and 'hostssl' for remote connection
• mode ssl require: pg_hba have only 'hostssl' for remote connection

And I can duplicate this flag to have a mode of communication from instances and a mode of communication from client, like etcd config (difference between peer and client connections).
(I will trying to connect on gitter.im if you any question more :) )

[sgotti]

@nabbar Thanks. I'll prefer to do little changes per PR. So what about just adding the ability to use ssl between instances in this PR (without chaning the generated hba) and then add additional options/tuning in future PRs?

[nabbar]

Hello,

I'll prefer to do little changes per PR. So what about just adding the ability to use ssl between instances in this PR (without chaning the generated hba) and then add additional options/tuning in future PRs?

sure, no problem. I think this will be more readable in 2 PR.

[nabbar]

Hello,

Sorry for the time to update.
So, I have remove the pg_hba from this PR and I will send you a new PR just for the pg_hba.

In add, I have made some optimization in the code to having less line of change from master and remove some useless line of my code.

Now, if the "ssl=on" is not find in spec init, the ssl mode don't change, even if a cert/key file are specified. If the "ssl=on" is find, I use now your method to check if this mandatory files for PG instance are specified, readable and valid.

Indeed, when I remove the ssl config in pg_hba, I have changed the sslmode=require to sslmode=allow when ssl is enable. The slave cannot connect to master with sslmode=require if the pg_hba of master have no hostssl. Before next PR, the user can add hostssl manually by spec to enable the ssl connection.

[sgotti]

@nabbar sorry but I probably lost this last comment. If you have time to work on this PR, can you please rebase so I can review it?

[nabbar]

@sgotti Hi, no problem :) It's done, I have rebase.

[sgotti]

@nabbar Thanks! After thinking about this a bit I think it's going to change too many things in one step. For example adding stolon sides checks on the tls configuration and the related logic to set the sslmode are in part duplicating the postgres checks (and will create some issues if they differs, for this reason we are not checking the postgres parameters).

I'll start with little steps. For example I opened PR #501 that will just remove the hardcoded ssl disabling and will set it to prefer so, if ssl is enabled it'll also be used by replication.

Then, if needed we could improve it in little steps to also add option to set the replication conn sslmode, the pg_hba part etc... Thoughts?

[nabbar]

@sgotti ok, and yes I need to set the replication into a ssl connection