Aligned TLS options with hydra: allow cert&key to be specified with file

[fredbi]

Signed-off-by: Frederic BIDON [email protected]

[aeneasr]

This is mixed up, should probably be the key?

[aeneasr]

Same here

[fredbi]

darn!

[aeneasr]

This should probably be at the top of this section?

[aeneasr]

if all four variables are empty, this will always fatal, no?

[fredbi]

yep. Let me fix this...

[fredbi]

ok I changed a bit to factorize this, in view of TLS addition to serve api

[aeneasr]

Can we improve this block? It does not have any tests and since it's difficult to read I'd love to see some reduction in complexity here. Also, I think that the naming of the variables is quite misleading and easy to misunderstand. A very simple way to improve this is with something like this:

var cert tls.Certificate
if certPath, keyPath := viper.GetString(), viper.GetString(); certPath == "" && keyPath == "" {
  cert, err := ...
  if err != nil ...
  return cert
}

if certString, keyString := viper.GetString(), viper.GetString(); certString== "" && keyString == "" {
  cert, err := ...
  if err != nil ...
  return cert
}

And then just check if cert != nil before ListenAndServe

[fredbi]

ok I'll do that. Please bear that the original section was terrible style, with variables shadowing at every corner... I'll try to add a test as well.

[aeneasr]

My comment was not intended to be critical of you. I don't care who wrote the code - I write terrible code all the time. But while we work on this, let's improve it too! :)

[fredbi]

I used a boolean because this is a struct: the func cannot return nil, and testing that the returned value is an empty structure is ugly. However, I agree the if cascade if bad.

[aeneasr]

You can return a pointer *tls.Certificate which can evaluate to nil instead

[fredbi]

why not. This just incurs some more changes in the existing code...

[fredbi]

@aeneasr how about this instead? [now with pointer]
This removes the fatalf() in func so I can add a unit test (fatalf() is carried on by caller).

func getTLSCertAndKey() (*tls.Certificate, error) {
	certString, keyString := viper.GetString("HTTP_TLS_CERT"), viper.GetString("HTTP_TLS_KEY")
	certPath, keyPath := viper.GetString("HTTP_TLS_CERT_PATH"), viper.GetString("HTTP_TLS_KEY_PATH")

	if certString == "" && keyString == "" && certPath == "" && keyPath == "" {
		// serve http
		return nil, nil
	}
	if certString != "" && keyString != "" {
		tlsCertBytes, err := base64.StdEncoding.DecodeString(certString)
		if err != nil {
			return nil, fmt.Errorf("unable to base64 decode the TLS certificate: %v", err)
		}
		tlsKeyBytes, err := base64.StdEncoding.DecodeString(keyString)
		if err != nil {
			return nil, fmt.Errorf("unable to base64 decode the TLS private key: %v", err)
		}

		cert, err := tls.X509KeyPair(tlsCertBytes, tlsKeyBytes)
		if err != nil {
			return nil, fmt.Errorf("unable to load X509 key pair: %v", err)
		}
		return &cert, nil
	}
	if certPath != "" && keyPath != "" {
		cert, err := tls.LoadX509KeyPair(certPath, keyPath)
		if err != nil {
			return nil, fmt.Errorf("unable to load X509 key pair from files: %v", err)
		}
		return &cert, nil
	}
	// serve http
	logger.Warnln("TLS requires both cert and key to be specified. Fall back to serving HTTP")
	return nil, nil
}

[aeneasr]

Nice! This looks much cleaner. I think the if logic can be nested:

func getTLSCertAndKey() (*tls.Certificate, error) {
	certString, keyString := viper.GetString("HTTP_TLS_CERT"), viper.GetString("HTTP_TLS_KEY")
	certPath, keyPath := viper.GetString("HTTP_TLS_CERT_PATH"), viper.GetString("HTTP_TLS_KEY_PATH")

	if len(certString) + len(keyString) + len(certPath) + len(keyPath) == 0 {
		// serve http
		return nil, nil
	} else if certString != "" && keyString != "" {
		tlsCertBytes, err := base64.StdEncoding.DecodeString(certString)
		if err != nil {
			return nil, fmt.Errorf("unable to base64 decode the TLS certificate: %v", err)
		}
		tlsKeyBytes, err := base64.StdEncoding.DecodeString(keyString)
		if err != nil {
			return nil, fmt.Errorf("unable to base64 decode the TLS private key: %v", err)
		}

		cert, err := tls.X509KeyPair(tlsCertBytes, tlsKeyBytes)
		if err != nil {
			return nil, fmt.Errorf("unable to load X509 key pair: %v", err)
		}
		return &cert, nil
	} else if certPath != "" && keyPath != "" {
		cert, err := tls.LoadX509KeyPair(certPath, keyPath)
		if err != nil {
			return nil, fmt.Errorf("unable to load X509 key pair from files: %v", err)
		}
		return &cert, nil
	}
	// serve http
	logger.Warnln("TLS requires both cert and key to be specified. Fall back to serving HTTP")
	return nil, nil
}

[fredbi]

len(certString) + len(keyString) + len(certPath) + len(keyPath) == 0
looks like an elegant construct... it is actually slower (i.e. less efficient) than the ... != "" &&...

[aeneasr]

Feel free to choose either one. You can also do certString + keyString + certPath + keyPath == "" iirc :D

[aeneasr]

Awesome! Thank you :)