Separate TLS config for AcraConnector and DB #419

ilammy · 2020-09-26T09:20:16Z

Currently AcraServer uses only one TLS configuration for everything. This means, in particular, that both AcraConnector and DB are expected to use the same private CA, if root certificates are not available in the system certificate store. This is not always the case, and it would be nice to allow separate TLS configurations.

This PR adds several new options:

New option	Old option	Purpose
`tls_client_ca`	`tls_ca`	path to CAfile with trusted root certificates for AcraConnector verification
`tls_client_cert`	`tls_cert`	path to server certificate presented to AcraConnectors
`tls_client_key`	`tls_key`	path to private key to the certificate above
`tls_client_auth`	`tls_auth`	TLS authentication level when accepting AcraConnector connections

New option	Old option	Purpose
`tls_database_ca`	`tls_ca`	path to CAfile with trusted root certificates for database verification
`tls_database_cert`	`tls_cert`	path to client certificate used when connecting to database
`tls_database_key`	`tls_key`	path to private key to the certificate above
`tls_database_auth`	`tls_auth`	TLS authentication level when connecting to database
`tls_database_sni`	`tls_db_sni`	server name expected when connecting to database (renamed option)

New options override old options. The old options still remains not deprecated, they might be useful as a shortcut.

This required a refactoring to split the TLS configuration, ~~so in the future it will be possible to have more specific configuration (think, different allowed authentication modes, cryptosuites, TLS versions, etc.)~~ which allowed to easily split the configuration.

There are no integration tests for this option currently. (Do we need them?) I have tested it manually, with recently updated certificates. The timing could not have been better...

We are going to need more values from ProxySetting so instead of accepting additional arguments for them, take ProxySetting directly in constructors.

It is a reasonable option for AcraConnector and database to have different TLS configurations. Instead of using a single one, teach proxies to use two independent configurations from settings.

Now, actually accept and store separate configurations in settings used by connection proxies.

In addition to common "--tls_ca" option, accept "--tls_ca_client" and "--tls_ca_database" options to configure individual sides of the proxied connection. If new options are specified, they take priority. This makes it possible for AcraConnector and database to have completely separate private CAs issue their certificates.

vixentael

Looks inspiring

shadinua · 2020-09-26T11:25:19Z

Great, thanks!

Will it be too complex to add a separate configuration for tls_auth as well? This would cover most of the inconvenience with the TLS configuration.

ilammy · 2020-09-26T12:17:34Z

Will it be too complex to add a separate configuration for tls_auth as well?

Will do, refactoring should ~~make~~ have made that trivial.

In addition to CA options, add "tls_auth_client" and "tls_auth_database" which override the common "tls_auth" option, setting the level of TLS connection authentication.

ilammy · 2020-09-26T13:18:55Z

@shadinua, here you go. There are now new options tls_auth_client and tls_auth_database which override the basic level set by tls_auth (by default, the highest one).

For example, you might leave the default value of RequireAndVerifyClientCert for AcraConnector, but allow the database to just present any client certificate with --tls_auth_database=2. Or validate client certificates if given, but don't care about AcraConnector ones: --tls_auth=3 --tls_auth_client=0.

The common option is still there in case we'd need to add more TLS configs later, like with CA. Right now it can make more sense to configure client and database explicitly, rather than configuring this, say, for the client and for everyone else.

cmd/acra-server/acra-server.go

decryptor/mysql/response_proxy.go

Suggested-by: vixentael <vixentael@users.noreply.github.com>

Use common "tls_client" and "tls_database" namespaces for new specific TLS options.

The old option is still accepted but deprecated. The new "tls_database_sni" option will override "tls_db_sni".

Instead of using database hostname or SNI, just skip the server name entirely. AcraConnector's TLS configuration is used to accept connections from AcraConnectors, so ServerName is irrelevant there.

Introduce "tls_{client,database}_{cert,key}" options too, allowing AcraServer to use different certifcates for AcraConnector and database connections. With this, TLS configurataion is now completely separated. It is possible to configure AcraConnector and database TLS connections separately if necessary, but there is also a common option set if that is more convenient.

ilammy · 2020-09-28T12:34:16Z

After some discussion we have decided to split the remaining options as well. That is, it is possible to set the keys and certificates independently as well. That way we can have different AcraConnector and database TLS configurations if necessary (and there are still common options in case the configuration can be shared).

Given that, the original names for new options were suboptimal. They have been updated for better namespacing:

tls_client_ca, tls_client_cert, tls_client_key, tls_client_auth
tls_database_ca, tls_database_cert, tls_database_key, tls_database_auth, tls_database_sni

The tls_db_sni option has been renamed into tls_database_sni for consistency. (The old one is still accepted.)

ilammy · 2020-09-28T12:42:14Z

I don't quite like that tls_client_cert option specifies server certificate to be used by AcraServer (mandatory parameter), but it's consistent with the original naming idea.

CHANGELOG_DEV.md

shadinua

Thanks a lot from infrastructure team! :)

Co-authored-by: vixentael <vixentael@users.noreply.github.com>

vixentael · 2020-09-28T13:52:56Z

Looks amazing, let's merge and use!

decryptor/base/proxy.go

Suggested-by: Anatolii Lishchynskyi <anatolii.lishchynskyi@cossacklabs.com>

ilammy added 4 commits September 26, 2020 11:47

Accept ProxySetting when constructing proxies

76d4241

We are going to need more values from ProxySetting so instead of accepting additional arguments for them, take ProxySetting directly in constructors.

Use separate TLS configs in proxies

b673c75

It is a reasonable option for AcraConnector and database to have different TLS configurations. Instead of using a single one, teach proxies to use two independent configurations from settings.

Store separate TLS configs in ProxySetting

17115b3

Now, actually accept and store separate configurations in settings used by connection proxies.

ilammy added enhancement acra-server labels Sep 26, 2020

ilammy requested review from vixentael, Lagovas, shadinua and iamnotacake September 26, 2020 09:20

ilammy force-pushed the split-certs branch from 58f2782 to 85c3241 Compare September 26, 2020 09:25

vixentael approved these changes Sep 26, 2020

View reviewed changes

Split TLS authentication options

ffe1b4f

In addition to CA options, add "tls_auth_client" and "tls_auth_database" which override the common "tls_auth" option, setting the level of TLS connection authentication.

ilammy changed the title ~~Separate TLS CAs for AcraConnector and DB~~ Separate TLS config for AcraConnector and DB Sep 26, 2020

vixentael approved these changes Sep 26, 2020

View reviewed changes

cmd/acra-server/acra-server.go Outdated Show resolved Hide resolved

cmd/acra-server/acra-server.go Outdated Show resolved Hide resolved

vixentael reviewed Sep 26, 2020

View reviewed changes

decryptor/mysql/response_proxy.go Show resolved Hide resolved

Improved option descriptions

6e2851d

Suggested-by: vixentael <vixentael@users.noreply.github.com>

Lagovas approved these changes Sep 28, 2020

View reviewed changes

ilammy added 4 commits September 28, 2020 15:27

Rename options for better namespacing

c65b2c1

Use common "tls_client" and "tls_database" namespaces for new specific TLS options.

Rename "tls_db_sni" => "tls_database_sni" for consistency

58ee7b0

The old option is still accepted but deprecated. The new "tls_database_sni" option will override "tls_db_sni".

Do not setup ServerName in AcraConnector TLS config

390ce22

Instead of using database hostname or SNI, just skip the server name entirely. AcraConnector's TLS configuration is used to accept connections from AcraConnectors, so ServerName is irrelevant there.

vixentael approved these changes Sep 28, 2020

View reviewed changes

CHANGELOG_DEV.md Outdated Show resolved Hide resolved

shadinua approved these changes Sep 28, 2020

View reviewed changes

Update CHANGELOG_DEV.md

3a8c656

Co-authored-by: vixentael <vixentael@users.noreply.github.com>

iamnotacake reviewed Sep 28, 2020

View reviewed changes

decryptor/base/proxy.go Outdated Show resolved Hide resolved

Lagovas approved these changes Sep 28, 2020

View reviewed changes

Improve documentation comments

2ec0614

Suggested-by: Anatolii Lishchynskyi <anatolii.lishchynskyi@cossacklabs.com>

ilammy merged commit c9c8825 into cossacklabs:master Sep 28, 2020

ilammy deleted the split-certs branch September 28, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate TLS config for AcraConnector and DB #419

Separate TLS config for AcraConnector and DB #419

ilammy commented Sep 26, 2020 •

edited

vixentael left a comment

shadinua commented Sep 26, 2020

ilammy commented Sep 26, 2020 •

edited

ilammy commented Sep 26, 2020 •

edited

ilammy commented Sep 28, 2020

ilammy commented Sep 28, 2020

shadinua left a comment •

edited

vixentael commented Sep 28, 2020

Separate TLS config for AcraConnector and DB #419

Separate TLS config for AcraConnector and DB #419

Conversation

ilammy commented Sep 26, 2020 • edited

vixentael left a comment

Choose a reason for hiding this comment

shadinua commented Sep 26, 2020

ilammy commented Sep 26, 2020 • edited

ilammy commented Sep 26, 2020 • edited

ilammy commented Sep 28, 2020

ilammy commented Sep 28, 2020

shadinua left a comment • edited

Choose a reason for hiding this comment

vixentael commented Sep 28, 2020

ilammy commented Sep 26, 2020 •

edited

ilammy commented Sep 26, 2020 •

edited

ilammy commented Sep 26, 2020 •

edited

shadinua left a comment •

edited