installer/pkg/config-generator/tls: generate root CA's with go by trawler · Pull Request #3316 · coreos/tectonic-installer

trawler · 2018-06-28T15:22:48Z

Fixes #INST-1106

coreosbot · 2018-06-28T15:22:50Z

Can one of the admins verify this patch?

squat

@trawler this is looking even better now. There are still a few points I'd like to fix.

squat · 2018-06-28T15:54:54Z

installer/pkg/config-generator/tls.go

+	if c.CA.RootCAKeyPath == "" {
+		key, err = generatePrivateKey(clusterDir)
+		if err != nil {
+			return fmt.Errorf("failed to generate private key: %s", err)


as a rule, we like to format errors with %v

squat · 2018-06-28T15:55:51Z

installer/pkg/config-generator/tls.go

+
+		keyDst := filepath.Join(clusterDir, rootCAKeyPath)
+		if err := copyFile(c.CA.RootCAKeyPath, keyDst); err != nil {
+			return fmt.Errorf("failed to write file (%s)", err)


did you mean to pass the file destination for the (%s)? otherwise lets be consistent and wrap the error like : %s

squat · 2018-06-28T15:57:25Z

installer/pkg/config-generator/tls.go

+
+	// If RootCACertPath is not provided, generate a cert
+	if c.CA.RootCACertPath == "" {
+		_, err := generateRootCA(clusterDir, key)


nit: since you are ignoring the non-error return value, we can make this a little bit nicer with:

if _, err := generateRootCA(clusterDir, key); err != nil { ... }

squat · 2018-06-28T15:58:03Z

installer/pkg/config-generator/tls.go

+		}
+		certDst := filepath.Join(clusterDir, rootCACertPath)
+		if err := copyFile(c.CA.RootCACertPath, certDst); err != nil {
+			return fmt.Errorf("failed to write file (%s)", err)


same question here as above concerning the (%s)

squat · 2018-06-28T16:01:03Z

installer/pkg/config-generator/utils.go

+
+	// Validate that file content is a valid certificate
+	if err := validate.Certificate(pem); err != nil {
+		return "", fmt.Errorf("provided file does not seem to contain a valid certificate (%s): %s", path, err)


for consistency please use %v to format errors.

squat · 2018-06-28T16:10:34Z

installer/pkg/validate/validate.go

+	if block == nil {
+		return errors.New("failed to parse certificate pem")
+	}
+	_, err := x509.ParseCertificate(block.Bytes)


nit: we can use

if _, err := x509.ParseCertificate(block.Bytes); err != nil { ... }

squat · 2018-06-28T16:14:03Z

installer/pkg/validate/validate.go

 	}

-	trimmed := strings.TrimSpace(v)
+	r := strings.NewReader(v)


between lines 383 and 389 it looks like we went from a string, to a reader, to a byte array. It would be easier, and error-free, to go directly from a string to a byte array like:

block, _ := pem.Decode([]byte(v))

squat · 2018-06-28T16:16:10Z

installer/pkg/validate/validate_test.go

 	const invalidMsg = "invalid certificate"
 	const privateKeyMsg = "invalid certificate (appears to be a private key)"
+	const badPem = "failed to parse certificate pem"
+	const certificate = `-----BEGIN CERTIFICATE-----


nit: for better maintainability, we could consider generating a throwaway certificate here instead of hardcoding a hand-generated one. not a blocker.

squat · 2018-06-28T16:16:16Z

installer/pkg/validate/validate_test.go


 func TestPrivateKey(t *testing.T) {
 	const invalidMsg = "invalid private key"
+	const privateKey = `-----BEGIN RSA PRIVATE KEY-----


same as above

squat · 2018-06-28T16:17:01Z

installer/pkg/workflow/utils.go


 	tectonicSystemConfigFilePath := filepath.Join(tectonicPath, tectonicSystemFileName)
 	return writeFile(tectonicSystemConfigFilePath, tectonicSystem)
+


we can delete this extra newline after the return

squat

This PR is getting a lot more solid. I found a few bugs with the current implementation and I also have a few design suggestions. Please take a look.

squat · 2018-06-29T12:44:08Z

installer/pkg/config-generator/tls.go

+	if c.CA.RootCACertPath == "" {
+		if generateRootCA(clusterDir, key); err != nil {
+			return fmt.Errorf("failed to create a certificate: %v", err)
+


this newline here is a little funny

squat · 2018-06-29T12:45:09Z

installer/pkg/config-generator/tls.go

+
+	// If RootCACertPath is not provided, generate a cert
+	if c.CA.RootCACertPath == "" {
+		if generateRootCA(clusterDir, key); err != nil {


this is not actually checking the error returned by the generateRootCA func, but rather the error from the parent scope.

squat · 2018-06-29T12:49:30Z

installer/pkg/config-generator/tls.go

+// If no file paths were provided, the certs will be auto-generated
+func (c *ConfigGenerator) GenerateTLSConfig(clusterDir string) error {
+	var key *rsa.PrivateKey
+	var err error


there should be no need to have these block-scoped variables here. for long functions it can lead to shady scenarios where we unsuspectingly shadow the variables or forget to assign variables since the variable is already declared (as happened below where the error from generateRootCA was not actually being checked; if this variable was not declared here then the compiler would have caught that bug).

squat · 2018-06-29T12:52:21Z

installer/pkg/config-generator/tls.go

+	var err error
+
+	// If RootCAKeyPath is not provided, generate a key
+	if c.CA.RootCAKeyPath == "" {


I think it would be easier to follow the logic of this long function if both the private key generation and certificate generation were done close together and if the key file copying and the cert file copying were done together. WDYT? This if statement could be:

if c.CA.RootCAKeyPath == "" && c.CA.RootCACertPath == "" { // generate key and certificate } else { // copy key and certificates }

squat · 2018-06-29T12:58:03Z

installer/pkg/config-generator/utils.go

+	defer f.Close()
+
+	w := bufio.NewWriter(f)
+	if _, err := fmt.Fprintln(w, content); err != nil {


Do you know why are we writing to the writer using fmt instead of using w.WriteString [0]?

[0] https://golang.org/pkg/bufio/#Writer.WriteString

squat · 2018-06-29T13:02:32Z

installer/pkg/workflow/install.go

+func generateTLSConfigStep(m *metadata) error {
+	newTLSPath := filepath.Join(m.clusterDir, newTLSPath)
+	if err := os.MkdirAll(newTLSPath, os.ModeDir|0755); err != nil {
+		return fmt.Errorf("failed to create TLS directory at %s", newTLSPath)


should we wrap the returned error here? or is it not necessary?

squat · 2018-06-29T14:09:10Z

installer/pkg/workflow/install.go

 	}
 }

+// InstallTLSNewWorkflow Generates the TLS certificates Using go, instead of TF


nit: this sentence has some funny casing.

squat · 2018-06-29T14:11:11Z

installer/pkg/workflow/install.go

 }
+
+func generateTLSConfigStep(m *metadata) error {
+	newTLSPath := filepath.Join(m.clusterDir, newTLSPath)


you are shadowing a constant here, which is a little funny. Also, as far as this function is concerned, the newTLSPath is just the path, right? This func doesn't know about anything but the new TLS stuff anyways so there is no need to further qualify the variable name in this lexical context.

squat · 2018-06-29T14:15:49Z

installer/pkg/validate/validate.go


 	// Don't let users hang themselves
-	if isMatch(`-BEGIN [\w-]+ PRIVATE KEY-`, trimmed) {
+	if err := PrivateKey(trimmed); isMatch("PRIVATE", trimmed) && err == nil {


I don't think we need to use the isMatch test anymore. The PrivateKey test you rewrote is quite thorough enough. I do not think that the matching provides any extra benefit and it is extra noise.

squat · 2018-06-29T14:22:08Z

installer/pkg/config/validate.go

+		}
+		fallthrough
+	case c.CA.RootCACertPath != "":
+		if err := validate.FileExists(c.CA.RootCACertPath); err != nil {


I think that the validation of the certificate file and private key file should occur here in the validation phase rather than (or maybe in addition to) the workflow stage. The reason for this is that we want to catch user errors as early as possible so that a user can't even initialize an installer config if it is invalid. Currently, a user could point their CA cert to an arbitrary file, run the install command, and then get an error halfway through. WDYT?

Yes, should be pretty much analogous to ignition validation https://github.com/coreos/tectonic-installer/blob/master/installer/pkg/config/validate.go#L296

squat

Looks great. Just a handful of small comments

squat · 2018-07-02T14:23:10Z

installer/pkg/config/validate.go

+	return errs
+}
+
+// validateCAKey validates and returns the content of the private key file


this validation func does not return any content of any file, only an error if any was encountered.

squat · 2018-07-02T14:23:17Z

installer/pkg/config/validate.go

+	return nil
+}
+
+// validateCACert validates and returns the content of the certificate file


same as above

squat · 2018-07-02T14:26:00Z

installer/pkg/validate/validate_test.go

+	const badPem = "failed to parse certificate"
+
+	// throwaway rsa key
+	rsaKey, _ := tls.GeneratePrivateKey()


I think this got lost in the last round of reviews. We should be checking the errors returned by any func even in tests rather than ignoring them. If we do get an unexpected error in the setup, then we can call: t.Fatalf("failed to generate private key: %v", err) and the test suite will fail gracefully.

squat · 2018-07-02T14:26:09Z

installer/pkg/validate/validate_test.go

+	const invalidMsg = "failed to parse private key"
+
+	// throw-away rsa key
+	rsaKey, _ := tls.GeneratePrivateKey()


enxebre · 2018-07-03T09:22:06Z

installer/pkg/workflow/fixtures/terraform.tfvars

  "tectonic_aws_worker_root_volume_type": "gp2",
-  "tectonic_libvirt_network_if": "osbr0",
-  "tectonic_libvirt_resolver": "8.8.8.8",
+  "tectonic_base_domain": "tectonic-ci.de",


Is this just changing order? it'd be useful to proactively add context for that when that's the case :)

This code is descended from terraformPrepareStep, which landed in 1b4bb62 (Cli tool, 2018-02-05, coreos/tectonic-installer#2806). The workspace version was factored out into copyFile in 2b82fbe (installer: Integrate multistep cli with configuration, 2018-02-16, coreos/tectonic-installer#2960). The config-generator version was copy/pasted (or independently developed?) in 1d54899 (installer/pkg/config-generator/tls: generate root CA's with go, 2018-06-28, coreos/tectonic-installer#3316). This commit DRYs that up by pulling the duplicate code out into a shared package. I've also changed O_RDWR to O_WRONLY. The O_RDWR is originally from 1b4bb62, but we do not require the ability to read from the target file. Also add a unit test to exercise this code.

trawler requested review from spangenberg and squat June 28, 2018 15:22

squat suggested changes Jun 28, 2018

View reviewed changes

trawler force-pushed the move_tls_from_terraform_to_installer branch 3 times, most recently from 720c559 to b41bc7d Compare June 29, 2018 14:11

squat suggested changes Jun 29, 2018

View reviewed changes

trawler force-pushed the move_tls_from_terraform_to_installer branch 3 times, most recently from b9e13d7 to f584107 Compare July 2, 2018 13:42

squat suggested changes Jul 2, 2018

View reviewed changes

trawler force-pushed the move_tls_from_terraform_to_installer branch 2 times, most recently from 6231500 to 2a09acc Compare July 3, 2018 08:38

spangenberg previously approved these changes Jul 3, 2018

View reviewed changes

enxebre reviewed Jul 3, 2018

View reviewed changes

enxebre previously approved these changes Jul 3, 2018

View reviewed changes

installer/pkg/config-generator/tls: generate root CA's with go

1d54899

trawler dismissed stale reviews from enxebre and spangenberg via 1d54899 July 3, 2018 09:55

trawler force-pushed the move_tls_from_terraform_to_installer branch from 2a09acc to 1d54899 Compare July 3, 2018 09:55

enxebre approved these changes Jul 3, 2018

View reviewed changes

squat approved these changes Jul 3, 2018

View reviewed changes

trawler merged commit 07f7b42 into coreos:master Jul 3, 2018


		tectonicSystemConfigFilePath := filepath.Join(tectonicPath, tectonicSystemFileName)
		return writeFile(tectonicSystemConfigFilePath, tectonicSystem)

Conversation

trawler commented Jun 28, 2018

Uh oh!

coreosbot commented Jun 28, 2018

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants