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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 6 commits into from
+55 −9

Conversation

petervandoros
Copy link

@petervandoros petervandoros commented Jan 2, 2025 •
edited
Loading

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.

@petervandoros
Update connection.DuplicateCredentials function to set correct Server…
64e94b2 
…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.
@petervandoros
Use connection.DuplicateCredentials in cases where the connection key…
7bf4043 
… changes
@petervandoros
Extract TLS config key name generation to GetDBTLSConfigKey function
d88c53b
@petervandoros
Extract function to register a connection's TLS config with the mysql…
79c15d2 
… driver

This allows us to register TLS configuration is the various places
where connection configs are created and before they're used.
@petervandoros
Register TLS config when setting up master connection info
15daab8 
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]
@petervandoros
Register TLS config when setting up the throttler's connection info
0fa2033 
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 mentioned this pull request Jan 2, 2025
Open
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rashiq rashiq Awaiting requested review from rashiq rashiq is a code owner

@meiji163 meiji163 Awaiting requested review from meiji163 meiji163 is a code owner

@timvaillancourt timvaillancourt Awaiting requested review from timvaillancourt timvaillancourt is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@petervandoros