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

Fix TLS verify error when gh-ost discovers the replication master #1487

Merged
merged 7 commits into from
Feb 24, 2025

Conversation

petervandoros
Copy link
Contributor

@petervandoros petervandoros commented Jan 2, 2025

A Pull Request should be associated with an Issue.

Related issue: #1259

Description

This PR resolves TLS certificate verification failures when gh-ost discovers, or is forced to used, different master server hosts to what's specified by the --host arg.

Example error:

x509: certificate is valid for [old hostname], not [newly found hostname]

There are three reasons that, when combined, cause this error:

  1. The DuplicateCredentials function doesn't update the TLS config's ServerName property to match the connection's updated Key/Hostname.
  2. Only the inspector's database TLS configuration is registered with the MySQL driver during initialisation. The TLS configuration for other connections are not registered.
  3. The config key used to register the TLS config with the mysql driver is hard coded to ghost.

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

Considerations

  • I'm relatively new to Go, so please help me correct any Go idioms I missed or got wrong 😄 .
  • Even though I updated and added tests I thought appropriate, I didn't add tests for verifying the TLS config because I'm unsure how or if that's possible. I'm open to some direction on this.
  • Even though I have tested this PR in our test environment in AWS using Aurora 3 with MySQL 8.0 compatibility, I don't expect the changes to cause any issues in other environments or MySQL versions.

…Name property

Ensure the ServerName TLS property matches the new connection
instance key hostname to avoid TLS verify errors like the following:

2025-01-02 02:07:26 FATAL tls: failed to verify certificate: x509: certificate is valid for [old host], not [new host]

This is only one part of the fix for this issue. The second part,
registering TLS Config with the mysql driver, will come in
subsequent commits.
… driver

This allows us to register TLS configuration is the various places
where connection configs are created and before they're used.
This ensures that the master's TLS config has been registered with
the mysql driver before any connections are attempted.

This is the second part of resolving the following TLS verify
error:

2025-01-02 02:07:26 FATAL tls: failed to verify certificate: x509: certificate is valid for [old host], not [new host]
This ensures that the throttler's TLS config has been registered with
the mysql driver before any connections are attempted.

This is the second part of resolving the following TLS verify
error:

2025-01-02 02:07:26 FATAL tls: failed to verify certificate: x509: certificate is valid for [old host], not [new host]
@petervandoros petervandoros changed the title Pv/fix tls verify error Fix TKS verify error when gh-ost discovers the master server Jan 3, 2025
@petervandoros petervandoros changed the title Fix TKS verify error when gh-ost discovers the master server Fix TLS verify error when gh-ost discovers the master server Jan 3, 2025
@petervandoros petervandoros changed the title Fix TLS verify error when gh-ost discovers the master server Fix TLS verify error when gh-ost discovers the replication master Jan 3, 2025
@meiji163
Copy link
Contributor

This LGTM, thanks for the fix!

@meiji163 meiji163 merged commit d8672f7 into github:master Feb 24, 2025
8 checks passed
@petervandoros
Copy link
Contributor Author

Thank you!

When could we expect the changes be released in a new version? (no urgency, just asking for internal planning)

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.

2 participants