-
Notifications
You must be signed in to change notification settings - Fork 435
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
feat: add TDS8.0 Support for tedious #1522
Conversation
* fix type check for `options.encrypt` * syntax fix
Codecov Report
@@ Coverage Diff @@
## master #1522 +/- ##
==========================================
+ Coverage 80.65% 80.80% +0.15%
==========================================
Files 92 92
Lines 4663 4684 +21
Branches 857 864 +7
==========================================
+ Hits 3761 3785 +24
+ Misses 628 621 -7
- Partials 274 278 +4
|
…into NewTLSSupport
…into NewTLSSupport
ci: add latest build of sql server 2022
.github/workflows/nodejs.yml
Outdated
@@ -193,8 +193,7 @@ jobs: | |||
Push-Location C:\temp | |||
|
|||
Invoke-WebRequest -Uri https://download.microsoft.com/download/c/c/9/cc9c6797-383c-4b24-8920-dc057c1de9d3/SQL2022-SSEI-Dev.exe -OutFile SQL2022-SSEI-Dev.exe | |||
./SQL2022-SSEI-Dev.exe /ACTION=Download /MEDIAPATH=C:\temp /MEDIATYPE=CAB /QUIET | |||
|
|||
Start-Process -Wait -FilePath ./SQL2022-SSEI-Dev.exe -ArgumentList /ACTION=Download, /MEDIAPATH=C:\temp, /MEDIATYPE=CAB, /QUIET | |||
Start-Process -Wait -FilePath ./SQLServer2022-DEV-x64-ENU.exe -ArgumentList /qs, /x:setup | |||
|
|||
.\setup\setup.exe /q /ACTION=Install /INSTANCENAME=MSSQLSERVER /FEATURES=SQLEngine /UPDATEENABLED=0 /SQLSVCACCOUNT='NT SERVICE\MSSQLSERVER' /SQLSYSADMINACCOUNTS='BUILTIN\ADMINISTRATORS' /TCPENABLED=1 /NPENABLED=0 /IACCEPTSQLSERVERLICENSETERMS /SQLCOLLATION=SQL_Latin1_General_CP1_CI_AS /USESQLRECOMMENDEDMEMORYLIMITS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: We can use /SECURITYMODE='SQL'
to enable mixed authentication on installation, and use /SAPWD='...'
to set the password. That would allow us to skip changing these settings (and the server restart).
src/connection.ts
Outdated
secureContext: secureContext, | ||
servername: this.config.options.serverName ? this.config.options.serverName : this.config.server, | ||
}; | ||
const encryptsocket = tls.connect(encryptOptions, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if someone connects using strict
encryption to a SQL Server instance that does not support it?
How do we need to handle that, and how do we signal to the user that something is wrong? How do we help them understand what is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user uses a strict to connect to a SQL server instance that does not support it, a socket error will be returned as : " Client network socket disconnected before secure TLS connection was established"
Server side message:
"The login packet used to open the connection is structurally invalid; the connection has been closed. Please contact the vendor of the client library. [CLIENT: 172.28.192.1]
"
what do you think that we consume this message internally, and throw a custom error with a bit more information, so we can guide user through this
src/connection.ts
Outdated
this.transitionTo(this.STATE.SENT_PRELOGIN); | ||
}); | ||
|
||
console.log('using TDS 8.0 strict TLS encryption'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this might be some leftover log output.
src/connection.ts
Outdated
socket.on('error', (error) => { this.socketError(error); }); | ||
socket.on('close', () => { this.socketClose(); }); | ||
socket.on('end', () => { this.socketEnd(); }); | ||
socket.setKeepAlive(true, KEEP_ALIVE_INITIAL_DELAY); | ||
|
||
this.messageIo = new MessageIO(socket, this.config.options.packetSize, this.debug); | ||
this.messageIo.on('secure', (cleartext) => { this.emit('secure', cleartext); }); | ||
this.messageIo = new MessageIO(socket, this.config.options.packetSize, this.debug); | ||
this.messageIo.on('secure', (cleartext) => { this.emit('secure', cleartext); }); | ||
|
||
this.socket = socket; | ||
this.socket = socket; | ||
|
||
this.closed = false; | ||
this.debug.log('connected to ' + this.config.server + ':' + this.config.options.port); | ||
this.closed = false; | ||
this.debug.log('connected to ' + this.config.server + ':' + this.config.options.port); | ||
|
||
this.sendPreLogin(); | ||
this.transitionTo(this.STATE.SENT_PRELOGIN); | ||
this.sendPreLogin(); | ||
this.transitionTo(this.STATE.SENT_PRELOGIN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this duplication?
33078d9
to
a9524dc
Compare
🎉 This PR is included in version 16.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR added logics for supporting TDS8.0.
Co-authored with @arthurschreiber
Co-authored with @mShan0