Regenerate certificates to use SANs instead of Common Name #2461

albertteoh · 2020-09-09T11:52:22Z

Signed-off-by: albertteoh albert.teoh@logz.io

Which problem is this PR solving?

Proposed fix to a failing test caused by Go 1.15 dropping support for Common Names.

Please let me know if there is a better way of generating certs using SANs or if any configurations appear spurious, as this is pretty new to me.

Closes #2435

Short description of the changes

Regenerate certificates using Subject Alternative Names as per the recommendation in crypto/x509: Go 1.15 will break default connections to AWS RDS golang/go#39568 (comment).
Update README with instructions to generate the current set of certificates.
Add a copy of existing Travis jobs to be executed against Go 1.15 to avoid introduced regressions.

albertteoh · 2020-09-09T12:33:03Z

.travis.yml

@@ -7,6 +7,44 @@ dist: bionic

 matrix:
  include:
+  # Go 1.15


I wasn't able to get matrix expansion working here, but figured it's pretty important to avoid silent regressions on Go 1.15 builds.

Was hoping we could simply add:

go: - 1.14.x - 1.15.x

and remove the references to 1.14.x in the matrix.include list, but it seems go can't be expanded into the matrix.

Suggestions welcome!

Since the build now passes for 1.15, I suggest we just update 1.14 -> 1.15, instead of duplicating (the CI already runs for too long, we don't have capacity to double the number of steps).

We may want to add one extra step to run unit tests & lint with go tip (but I would do it in a separate PR).

Okay, updated to 1.15.
Where can I learn more about "go tip"?

I've seen it used elsewhere, e.g. https://github.com/opentracing/opentracing-go/blob/d34af3eaa63c4d08ab54863a4bdd0daa45212e12/.travis.yml#L7

Would an additional "tip" build version result in doubling the number of steps (1.15 + tip)?

codecov · 2020-09-09T12:56:58Z

Codecov Report

Merging #2461 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2461      +/-   ##
==========================================
+ Coverage   95.57%   95.59%   +0.01%     
==========================================
  Files         208      208              
  Lines       10690    10690              
==========================================
+ Hits        10217    10219       +2     
+ Misses        401      399       -2     
  Partials       72       72

Impacted Files	Coverage Δ
...lugin/sampling/strategystore/adaptive/processor.go	`100.00% <0.00%> (+0.79%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5508165...f361cda. Read the comment docs.

yurishkuro · 2020-09-09T17:44:38Z

pkg/config/tlscfg/testdata/README.md

@@ -1,35 +1,60 @@
 # Example Certificate Authority and Certificate creation for testing

-The following commands were used to create the CA, server and client's certificates and keys
+The following commands were used to create the CA, server and client's certificates and keys. These certificates use the Subject Alternative Name extension rather than the Common Name, which will be unsupported in Go 1.15.


just curious:

couldn't we just move all of this into a script, instead of a run book?

is there a flag to for "accept defaults" to make the script even less interactive?

Good point, and yes, there's a way to accept defaults. I've modified this README file into a bash script.

yurishkuro · 2020-09-09T17:46:12Z

.travis.yml

@@ -7,6 +7,44 @@ dist: bionic

 matrix:
  include:
+  # Go 1.15


Since the build now passes for 1.15, I suggest we just update 1.14 -> 1.15, instead of duplicating (the CI already runs for too long, we don't have capacity to double the number of steps).

We may want to add one extra step to run unit tests & lint with go tip (but I would do it in a separate PR).

Signed-off-by: albertteoh <albert.teoh@logz.io>

pkg/config/tlscfg/testdata/gen-certs.sh

yurishkuro · 2020-09-10T00:32:59Z

pkg/config/tlscfg/testdata/gen-certs.sh

+# Generate config files.
+# The server name (under alt_names in the ssl.conf) is `example.com`. (in accordance to [RFC 2006](https://tools.ietf.org/html/rfc2606))
+source gen-ssl-conf.sh example.com ssl.conf
+source gen-ssl-conf.sh wrong.com wrong-ssl.conf


perhaps instead of writing to the current dir (and accidentally checking in), we could use temp dir

tmp_dir=$(mktemp -d -t certificates) clean_up () { ARG=$? rm -rf $tmp_dir exit $ARG } trap clean_up EXIT

yurishkuro · 2020-09-10T00:34:26Z

pkg/config/tlscfg/testdata/gen-certs.sh

+             -CA example-CA-cert.pem \
+             -CAkey example-CA-key.pem \
+             -CAcreateserial \
+             -extfile ssl.conf


how about writing PEM files into $tmp_dir and then copying into the right place in the source tree?

yurishkuro · 2020-09-10T00:35:41Z

pkg/config/tlscfg/testdata/gen-ssl-conf.sh

+    printf "A script to generate SSL configuration files for testing purposes.\n\n"
+    printf "Usage: ssl-conf-gen.sh DOMAIN_NAME OUTPUT_FILE\n\n"
+    printf "Example: ssl-conf-gen.sh example.com ssl.conf\n"
+    return


should this be exit -1 instead?

I believe negative numbers are not supported as return codes in bash. But good catch that it should return a non-0 return code.

return was intentional because this script is sourced, which means it runs within the parent shell.

The added benefit of source-ing is that the set -ex that you suggested above will also apply in this script so commands in this script are printed to STDOUT and will also trigger an early exit if something fails.

If developers want to call this script separately, it can be run with source gen-ssl-conf.sh <args...>.

yurishkuro · 2020-09-10T00:37:18Z

pkg/config/tlscfg/testdata/README.md

@@ -1,35 +0,0 @@
-# Example Certificate Authority and Certificate creation for testing


I would keep the README with some basic instruction how to regenerate certificates, preferably via a make command.

README.md and Makefile added.

I was actually referring to the main Makefile. If it's just in this dir then I would simply provide instructions for running the script directly.

pkg/config/tlscfg/testdata/README.md

pkg/config/tlscfg/testdata/gen-certs.sh

yurishkuro · 2020-09-10T20:03:47Z

pkg/config/tlscfg/testdata/README.md

@@ -1,35 +0,0 @@
-# Example Certificate Authority and Certificate creation for testing


I was actually referring to the main Makefile. If it's just in this dir then I would simply provide instructions for running the script directly.

Signed-off-by: albertteoh <albert.teoh@logz.io>

yurishkuro

🎉 🎉 🎉

yurishkuro · 2020-09-11T00:08:44Z

one of the commits is not signed (probably the merge). You may want to squash into one:

https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md#missing-sign-offs

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh requested a review from a team as a code owner September 9, 2020 11:52

albertteoh requested a review from objectiser September 9, 2020 11:52

albertteoh commented Sep 9, 2020

View reviewed changes

yurishkuro reviewed Sep 9, 2020

View reviewed changes

albertteoh added 3 commits September 10, 2020 09:50

Regenerate certificates to use SANs instead of Common Name

3b88935

Signed-off-by: albertteoh <albert.teoh@logz.io>

Remove 1.14.x jobs; runbook -> automated script

c9fd172

Signed-off-by: albertteoh <albert.teoh@logz.io>

Fix indents

4c71d06

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh force-pushed the gencerts-1 branch from 68a3c94 to 4c71d06 Compare September 9, 2020 23:50

yurishkuro reviewed Sep 10, 2020

View reviewed changes

Add Makefile and README. Add dry-run option

f5d5bfd

Signed-off-by: albertteoh <albert.teoh@logz.io>

yurishkuro approved these changes Sep 11, 2020

View reviewed changes

Address review comments

f361cda

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh force-pushed the gencerts-1 branch from bffee26 to f361cda Compare September 11, 2020 00:12

yurishkuro merged commit a8e83cd into jaegertracing:master Sep 12, 2020

albertteoh mentioned this pull request Sep 21, 2020

Go 1.15 compatiblity open-telemetry/opentelemetry-collector#1820

Closed

yurishkuro mentioned this pull request Jul 18, 2023

Upgrade Kafka consumer to IBM/sarama driver #4591

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regenerate certificates to use SANs instead of Common Name #2461

Regenerate certificates to use SANs instead of Common Name #2461

albertteoh commented Sep 9, 2020

albertteoh Sep 9, 2020

yurishkuro Sep 9, 2020

albertteoh Sep 9, 2020

yurishkuro Sep 10, 2020

albertteoh Sep 10, 2020

codecov bot commented Sep 9, 2020 •

edited

Loading

yurishkuro Sep 9, 2020

albertteoh Sep 10, 2020

yurishkuro Sep 9, 2020

yurishkuro Sep 10, 2020

yurishkuro Sep 10, 2020

yurishkuro Sep 10, 2020

albertteoh Sep 10, 2020 •

edited

Loading

yurishkuro Sep 10, 2020

albertteoh Sep 10, 2020

yurishkuro Sep 10, 2020

yurishkuro Sep 10, 2020

yurishkuro left a comment

yurishkuro commented Sep 11, 2020

		@@ -1,35 +0,0 @@
		# Example Certificate Authority and Certificate creation for testing

Regenerate certificates to use SANs instead of Common Name #2461

Regenerate certificates to use SANs instead of Common Name #2461

Conversation

albertteoh commented Sep 9, 2020

Which problem is this PR solving?

Short description of the changes

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

codecov bot commented Sep 9, 2020 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

albertteoh Sep 10, 2020 • edited Loading

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

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Sep 11, 2020

codecov bot commented Sep 9, 2020 •

edited

Loading

albertteoh Sep 10, 2020 •

edited

Loading