Refactor tls configs #564

Lagovas · 2022-08-17T23:54:23Z

Here is added function for registering flags for tls configs and final construction tls.Config structs. First candidate for use is Redis. This update will help easily register similar flags to generate config with ocsp/crl usage, same wording and naming. Additionally, it allows generate flags for components that works as client or database which represented in naming (as we do now) and extra flags (like SNI for clients).

Why clients may need separate configuration for different components? For example, if they have several cloud solutions with TLS support, with own CA and server's certificates. For example, if service deployed on AWS, uses AWS's database, uses Elastic cloud for search features and cloud managed Hashicorp Vault.

There are left base common flags (tls_key/tls_cert/tls_auth/tls_ca) that works as one-place configuration for all configs that allows to specify only them in the simplest cases.

Also, remove the previously deprecated tls_db_sni parameter that breaks general design of TLS configuration. And much easier to remove it instead of supporting edge-case.

Additionally such configuration extended current client/database parameter separation with separate ocsp/crl parameters.

P.S. it is re-created PR from local to repo branch.

Checklist

Change is covered by automated tests
The coding guidelines are followed
Public API has proper documentation in the Acra documentation site or has PR on documentation repository
with new changes
CHANGELOG.md is updated (in case of notable or breaking changes)
CHANGELOG_DEV.md is updated
Benchmark results are attached (if applicable)
Example projects and code samples are up-to-date (in case of API changes)

…rvice

… default value

extend namers with group part for ocsp/crl back previous names for ocsp/crl flags

…s-configs

Zhaars · 2022-08-18T08:34:46Z

network/ocsp.go

+		if val == nil {
+			checkOnlyLeafCert = false
+		} else {
+			if !ok {


Hmm, is it even possible when the getter returns not-nil value but the ok will be false?

hm. you are right. actually is not. but if we will leave:

val, _ := getter.Get().(*bool) if val == nil { checkOnlyLeafCert = false } else { checkOnlyLeafCert = *val }

as for me, it will not be so obvious after several month why it did so. And I will need spend a time to understand that it is legal)

Zhaars · 2022-08-18T08:58:58Z

network/crl.go

+		crlCacheSize = val
+	}
+	if f := flags.Lookup(namerFunc(name, "cache_time", "crl")); f != nil {
+		getter, ok := f.Value.(flag.Getter)


Don't you think to use just some Parse methods for flag parsing?

crlCacheSize, err = strconv.ParseUint(f.Value.String(), 10, 0) if err != nil { log.WithField("value", getter.Get()).Fatalf("Can't cast %s to integer value", namerFunc(name, "cache_time", "crl")) }

It will reduce the amount of code plus we can avoid these runtime interface castings.

you are right. will do

Zhaars · 2022-08-18T09:09:25Z

network/tls_wrapper.go

+// incorrect -tls_database_ocsp_url:
+
+// NamerFunc func compile final parameter name for specified service name
+type NamerFunc func(serviceName, parameterName, groupName string) string


Looks like too generic name for me. Maybe we can rename it to something like CLIParamNameConstructorFunc or something that says that this function constructs the final name for CLI parameters for a specific service. What do you think?

agree, will rename

iamnotacake · 2022-08-18T09:59:48Z

CHANGELOG_DEV.md

@@ -1,3 +1,9 @@
+# 0.94.0 - 2022-08-18
+- Removed deprecated `--tls_db_sni` flag. Now is available only `--tls_database_sni`.


Suggested change

- Removed deprecated `--tls_db_sni` flag. Now is available only `--tls_database_sni`.

- Removed deprecated `--tls_db_sni` flag. Now only `--tls_database_sni` is available.

agree, will do it in next batch of commits to run CI once

Lagovas

applied your suggestions, @Zhaars , @iamnotacake

Lagovas · 2022-08-18T23:30:45Z

network/ocsp.go

+		if val == nil {
+			checkOnlyLeafCert = false
+		} else {
+			if !ok {


hm. you are right. actually is not. but if we will leave:

val, _ := getter.Get().(*bool) if val == nil { checkOnlyLeafCert = false } else { checkOnlyLeafCert = *val }

as for me, it will not be so obvious after several month why it did so. And I will need spend a time to understand that it is legal)

Lagovas · 2022-08-18T23:46:13Z

network/crl.go

+		crlCacheSize = val
+	}
+	if f := flags.Lookup(namerFunc(name, "cache_time", "crl")); f != nil {
+		getter, ok := f.Value.(flag.Getter)


you are right. will do

Lagovas · 2022-08-18T23:49:42Z

network/tls_wrapper.go

+// incorrect -tls_database_ocsp_url:
+
+// NamerFunc func compile final parameter name for specified service name
+type NamerFunc func(serviceName, parameterName, groupName string) string


agree, will rename

Lagovas · 2022-08-18T23:59:04Z

CHANGELOG_DEV.md

@@ -1,3 +1,9 @@
+# 0.94.0 - 2022-08-18
+- Removed deprecated `--tls_db_sni` flag. Now is available only `--tls_database_sni`.


agree, will do it in next batch of commits to run CI once

Zhaars

lgtm

Lagovas added 8 commits August 12, 2022 04:01

add helpers to register flags and compile tls.Config for specified se…

5d4c866

…rvice

use new functions for tls.Config compilation for acra-server

5dd02cf

update integration test related to testing tls auth type

16b4a43

log serials as hex to be compatible with openssl error messages

aad42d8

fix naming for client/database parameters

1c1cb37

fix handling not specified tls_*_auth parameters with overriding with…

da48fef

… default value

remove deprecated tls_db_sni flag

006bd61

extend namers with group part for ocsp/crl back previous names for ocsp/crl flags

Merge remote-tracking branch 'origin/master' into lagovas/refactor-tl…

a3dd9df

…s-configs

Lagovas requested review from iamnotacake, G1gg1L3s and Zhaars August 17, 2022 23:54

update changelog_dev

cf6e372

Lagovas mentioned this pull request Aug 18, 2022

Add docker image for redis with ssl [2/3] #565

Merged

7 tasks

Zhaars reviewed Aug 18, 2022

View reviewed changes

iamnotacake reviewed Aug 18, 2022

View reviewed changes

Lagovas added 3 commits August 19, 2022 02:47

use same logic of parsing that flag package do to simplify and optimize

b1eda5e

rename NamerFunc -> CLIParamNameConstructorFunc

6e3cffa

apply suggestions in changelog_dev.md

ffdbfd1

Lagovas commented Aug 18, 2022

View reviewed changes

Lagovas requested review from Zhaars and iamnotacake August 18, 2022 23:59

Zhaars approved these changes Aug 19, 2022

View reviewed changes

Lagovas merged commit 58d7548 into master Aug 22, 2022

Lagovas deleted the lagovas/refactor-tls-configs branch August 22, 2022 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor tls configs #564

Refactor tls configs #564

Lagovas commented Aug 17, 2022

Zhaars Aug 18, 2022

Lagovas Aug 18, 2022

Zhaars Aug 18, 2022

Lagovas Aug 18, 2022

Zhaars Aug 18, 2022 •

edited

Lagovas Aug 18, 2022

iamnotacake Aug 18, 2022

Lagovas Aug 18, 2022

Lagovas left a comment

Lagovas Aug 18, 2022

Lagovas Aug 18, 2022

Lagovas Aug 18, 2022

Lagovas Aug 18, 2022

Zhaars left a comment

		@@ -1,3 +1,9 @@
		# 0.94.0 - 2022-08-18
		- Removed deprecated `--tls_db_sni` flag. Now is available only `--tls_database_sni`.

Refactor tls configs #564

Refactor tls configs #564

Conversation

Lagovas commented Aug 17, 2022

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhaars Aug 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lagovas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhaars left a comment

Choose a reason for hiding this comment

Zhaars Aug 18, 2022 •

edited