Echo.StartTLSFromFile: Add function that loads key pair from strings #1277
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1277 +/- ##
=========================================
+ Coverage 81.78% 81.99% +0.2%
=========================================
Files 26 26
Lines 1933 1944 +11
=========================================
+ Hits 1581 1594 +13
+ Misses 243 242 -1
+ Partials 109 108 -1
Continue to review full report at Codecov.
|
echo.go
Outdated
| // StartTLSFromFile starts an HTTPS server. | ||
| // If `fromFile` is `true`: `cert` and `key` are paths to files that contain the certificate and key. | ||
| // If `fromFile` is `false`: `cert` and `key` contain the certificate and key. | ||
| func (e *Echo) StartTLSFromFile(address string, cert, key string, fromFile bool) (err error) { |
There was a problem hiding this comment.
The name is misleading. It loads either from file or from the strings provided.
It would be nice if we could avoid adding another method to start TLS... I generally avoid using interface{} as much as possible, but can't help wondering if it wouldn't be appropriate here. If we change the signature of StartTLS() to StartTLS(address string, certFile, keyFile interface{}) then the callers could submit either string (which would mean path) or []byte (which would mean key/cert content). It would be backward compatible, it would continue to accept strings as input. But it would also support the new feature. Thoughts @mbana @im-kulikov @vishr ?
Either way, this sounds like a very useful feature :-) Now one can feed it straight from AWS Parameter Store or from Vault :-)
There was a problem hiding this comment.
func (e *Echo) StartTLSFromFile(address string, key string, reader io.Reader) (err error) {mb this is more useful?
There was a problem hiding this comment.
Hardly @im-kulikov as https://golang.org/pkg/crypto/tls/#Certificate only takes in either path to files or content of certs (but no io.Reader). So asking for that param means both more work for callers and more fork for the function itself, makes little sense.
There was a problem hiding this comment.
You can read from io.Reader key and cert and pass it to X509KeyPair..
https://golang.org/pkg/crypto/tls/#X509KeyPair
func X509KeyPair(certPEMBlock, keyPEMBlock []byte) (Certificate, error) {
There was a problem hiding this comment.
something like this:
func (e *Echo) StartTLSFromReader(address string, key, cert io.Reader) (err error) {There was a problem hiding this comment.
thank you. now I see what you mean.
There was a problem hiding this comment.
Great then :)
Also, giving it a 2nd look, I would actually introduce an unexported function to clean it up further:
func (e *Echo) StartTLS(address string, certFile, keyFile interface{}) (err error) {
var cert []byte
if cert, err = filepathOrContent(certFile); err != nil {
return
}
var key []byte
if key, err = filepathOrContent(keyFile); err != nil {
return
}
s := e.TLSServer
s.TLSConfig = new(tls.Config)
s.TLSConfig.Certificates = make([]tls.Certificate, 1)
if s.TLSConfig.Certificates[0], err = tls.X509KeyPair(cert, key); err != nil {
return
}
return e.startTLS(address)
}
func filepathOrContent(fileOrContent interface{}) (content []byte, err error) {
switch v := fileOrContent.(type) {
case string:
return ioutil.ReadFile(v)
case []byte:
return v, nil
default:
return nil, fmt.Errorf("invalid cert type, must be string or []byte")
}
}Yeah, I like this better.
There was a problem hiding this comment.
Thanks @alexaandru that looks good. I was worried about breaking backwards compatibility hence I introduced the new function.
echo_test.go
Outdated
| require.NoError(err) | ||
|
|
||
| // Prevent the test to fail after closing the servers | ||
| if err := e.StartTLS(":0", []byte(cert), []byte(key)); err != http.ErrServerClosed { |
There was a problem hiding this comment.
https://golang.org/pkg/io/ioutil/#ReadFile
func ReadFile(filename string) ([]byte, error)
[]byte(cert), []byte(key) is unnecessary conversion
See latest PR, I did a force push (sorry). |
echo.go
Outdated
| @@ -269,6 +270,7 @@ var ( | |||
| ErrRendererNotRegistered = errors.New("renderer not registered") | |||
| ErrInvalidRedirectCode = errors.New("invalid redirect status code") | |||
| ErrCookieNotFound = errors.New("cookie not found") | |||
| ErrInvalidCertType = errors.New("invalid cert type, must be string or []byte") | |||
There was a problem hiding this comment.
invalid cert or key type, must be string or []byte
echo_test.go
Outdated
| expectedErr error | ||
| name string | ||
| } | ||
| testCases := []testCase{ |
There was a problem hiding this comment.
mb:
testCases := []struct {
cert interface{}
key interface{}
expectedErr error
name string
}{
//...
}
| key: 1, | ||
| expectedErr: ErrInvalidCertType, | ||
| name: `InvalidCertAndKeyTypes`, | ||
| }, |
There was a problem hiding this comment.
pls add:
{
cert: "_fixture/certs/cert.pem",
key: "_fixture/certs/key.pem",
expectedErr: nil,
name: `ValidCertAndKeyFilePath`,
}
If `certFile` or `keyFile` is `string` the values are treated as file paths. If `certFile` or `keyFile` is `[]byte` the values are treated as the certificate or key as-is.
|
Looks good! :) |
|
Thanks a lot, folks. What shall I do now? I cannot see a merge button. |
|
@alexaandru / @vishr can we merge or you wait for something else? |
|
Thanks guys! |
echo.go-Echo. StartTLSFromFile:echo_test.go-Echo. StartTLSFromFile:TestEchoStartTLSFromFile.