Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the ServerName for TLS configuration #988

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

dbussink
Copy link
Contributor

When TLS hostname validation used for the MySQL connection, the ServerName property needs to be set so that it knows which name to validate on the certificate.

Without this option and with InsecureSkipVerify set to false, validation will error here with a fatal error otherwise:

FATAL tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config

I'm not sure if there's a good way to additional test this specific change, as there's not a lot of extensive tests around the TLS handling and setup methods like UseTLS for example.

I think this change is also necessary to be able to complete #521.

When TLS hostname validation used for the MySQL connection, the
ServerName property needs to be set so that it knows which name to
validate on the certificate.

Without this option and with InsecureSkipVerify set to false, validation
will error here with a fatal error otherwise:

```
FATAL tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config
```
Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ryansimmen
Copy link
Member

@shlomi-noach how can we get the gh-ost-build-deploy-tarball test to pass? Why didn't it kick off automatically?

@timvaillancourt timvaillancourt added this to the v1.1.2 milestone Jun 17, 2021
@shlomi-noach
Copy link
Contributor

shlomi-noach commented Jun 17, 2021

how can we get the gh-ost-build-deploy-tarball test to pass?

Heh, probably .ci build gh-ost/tls-set-server-name if I remember correctly 😛

Why didn't it kick off automatically?

Dunno? Possibly only a PR submitted by an owner kicks that specific build? Could be some new configuration.

@shlomi-noach
Copy link
Contributor

Rather:

.ci build gh-ost-build-deploy-tarball/tls-set-server-name

@timvaillancourt timvaillancourt temporarily deployed to production/mysql_role=ghost_testing June 17, 2021 11:08 Inactive
@timvaillancourt timvaillancourt merged commit 40acde0 into master Jun 17, 2021
@timvaillancourt timvaillancourt deleted the tls-set-server-name branch June 17, 2021 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants