Feature #90: adding TLS mutual authentication to vertica-sql-go #117
Conversation
server.cnf is not needed
.github/workflows/ci.yaml
Outdated
| - name: Run tests | ||
| run: | | ||
| go test -race . -args --locator localhost:5433 --user dbadmin | ||
| go test -race . -args --locator localhost:5433 --user dbadmin --use_prepared_statements=0 | ||
| # go test -race . -args --locator localhost:5433 --user dbadmin --use_prepared_statements=0 |
There was a problem hiding this comment.
Would you please uncomment this line for test coverage?
connection.go
Outdated
| connectionLogger.Info("enabling SSL/TLS custom mode") | ||
| config, ok := tlsConfigs.get(sslFlag) | ||
| if !ok { | ||
| err := fmt.Errorf("tls config %s not registered", sslFlag) |
There was a problem hiding this comment.
I think it is better to provide guidance about how to register with "RegisterTLSConfig()" in this error message.
There was a problem hiding this comment.
done (referred to the README.md which has an example on how to register and use a custom config)
| err := fmt.Errorf("unsupported tlsmode flag: %s - should be 'server', 'server-strict' or 'none'", sslFlag) | ||
| connectionLogger.Error(err.Error()) | ||
| return err | ||
| connectionLogger.Info("enabling SSL/TLS custom mode") |
There was a problem hiding this comment.
Adding a comment to indicate this is the implementation for "mutual ssl mode" will be helpful for other developers in the future.
There was a problem hiding this comment.
Done. Good suggestion, I agree that "custom TLS" doesn't have any hint that it is used for mutual ssl mode.
driver_test.go
Outdated
| // adminDB := openConnection(t, "test_connection_closed_pre") | ||
| // defer closeConnection(t, adminDB, "test_connection_closed_post") | ||
| // const userQuery = "select 1 as test" | ||
| // func TestConnectionClosure(t *testing.T) { |
There was a problem hiding this comment.
Need to undo extra whitespaces for this function.
resources/tests/genCerts.sh
Outdated
| : ${ORG:=SQL} | ||
| : ${CN:=localhost} | ||
|
|
||
| : ${CERT_LOC:=./resources/tests/ssl} |
There was a problem hiding this comment.
I'm sorry, I don't understand the question. Github shows me a box with OBJ in it before the question mark.
Are you asking about the value of these fields, or the construct to get their values, or...
There was a problem hiding this comment.
Github shows me a box with OBJ in it. Is this a meaningful value or a typo?
There was a problem hiding this comment.
Weird. It must be an artifact of copy/paste. Invisible in my editors.
Do you see it anywhere else besides the end of line 10?
There was a problem hiding this comment.
That's the only place I saw this symbol. It is visible in Sublime with UTF-8 encoding from my end. Probably this is a matter of the setting of your editor.
|
|
README.md
Outdated
| } | ||
| tlsConfig := &tls.Config{RootCAs: rootCertPool, ServerName: host} | ||
| vertigo.RegisterTLSConfig("myCustomName", tlsConfig) | ||
| } |
|
Merged. Thanks for the contribution! |
|
@sitingren |
Hi @haihongren This PR has been included in release |
|
@sitingren many thanks 👍 |
Feature #90
Add support for custom TLS configurations implemented in the same structure as mysql Go client.
@mjskier contributed as well
Some code taken from @ehrt74 and this PR will replace #91