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

Add hostname to MySQLConnection.connect() #317

Merged
merged 5 commits into from
Apr 12, 2023
Merged

Conversation

programVeins
Copy link
Contributor

Prior to this, hostname in MySQLConnection.connect() was optional and defaulted to nil. This resulted in an error where upon entering url for connection string, apple's swift-nio-ssl threw: NIOSSLExtraError.failedToValidateHostname: Couldn't find <none> in certificate from peer By adding hostname from user given configuration, this is fixed.

Fixes #311

@0xTim 0xTim requested a review from gwynne February 12, 2023 14:17
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

This will probably all get overhauled when I finish reworking MySQLNIO, but there's no reason not to have this fix in the meantime 👍

@gwynne gwynne added bug Something isn't working semver-patch Internal changes only labels Feb 12, 2023
@gwynne gwynne added this to Awaiting Review in Vapor 4 via automation Feb 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2023

Codecov Report

Merging #317 (f0cceac) into main (6245a07) will increase coverage by 0.96%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #317      +/-   ##
==========================================
+ Coverage   57.55%   58.52%   +0.96%     
==========================================
  Files           8        8              
  Lines         344      352       +8     
==========================================
+ Hits          198      206       +8     
  Misses        146      146              
Impacted Files Coverage Δ
Sources/MySQLKit/MySQLConfiguration.swift 26.15% <100.00%> (+8.91%) ⬆️
Sources/MySQLKit/MySQLConnectionSource.swift 95.23% <100.00%> (+0.23%) ⬆️

@0xTim
Copy link
Member

0xTim commented Feb 12, 2023

It's always macOS 😒

@mannuch mannuch mentioned this pull request Apr 3, 2023
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Revised approval - the existing fix was incomplete due to missing logic for handling IP addresses correctly in the presence of a TLS configuration. The in-progress rewrite of MySQLNIO includes correct handling of the issue, but rather than make this wait another month for that, I tacked on a temporary fix in MySQLConfiguration.

Note: The fix is based on the code for handling the same concern in PostgresNIO, with the usual shoutout to @fabianfett for his fantastic work therein, of course! Even if it is a bit annoying to have to drop down to the POSIX API this way just 'cause NIOBSDSocket.inet.pton() isn't public... right, Fabian? 😉

Edit: For context, the macOS tests were previously failing due to a thrown NIOSSLExtraError.invalidSNIHostname when a hostname string contained an IP address (in this case 127.0.0.1); the issue was not macOS-specific, it just manifested there as a side effect of how the CI is set up.

@gwynne gwynne merged commit 5c520c5 into vapor:main Apr 12, 2023
Vapor 4 automation moved this from Awaiting Review to Done Apr 12, 2023
@VaporBot
Copy link

These changes are now available in 4.6.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-patch Internal changes only
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

Hostname Validation Error
5 participants