babelfish-for-postgresql / babelfish_extensions

Babelfish for PostgreSQL provides the capability for PostgreSQL to work with applications written for Microsoft SQL Server. Babelfish understands the SQL Server wire-protocol and T-SQL, the Microsoft SQL Server query and procedural language, so you don’t have to switch database drivers or rewrite all of your application queries.
https://babelfishpg.org/
Apache License 2.0
274 stars 93 forks source link

jTDS: support logins without PRELOGIN message #2860

Open staticlibs opened 1 month ago

staticlibs commented 1 month ago

Description

This PR is a part of a change originally implemented in #2320.

This change modifies the TDS login allowing to receive LOGIN7 as the first client message instead of PRELOGIN. It allows to successfully open a connection to Babelfish using jTDS driver without SSL. When the first message is PRELOGIN - then login logic is unchanged.

Copying review comments from #2320:

  1. Let's add a comment why we may not expecting TDS_PRELOGIN as the first request.

Added clarifying comment.

2. The check should be
if (TdsReadNextBuffer() != EOF && TdsCheckMessageType(TDS_PRELOGIN))

Modified the check.

3. If the prelogin message is not sent, what's the SSL behavior - TDS_ENCRYPT_NOT_SUP or TDS_ENCRYPT_OFF? OFF means the login will still happen in an encrypted manner.

If the first message is LOGIN7 then SSL behaviour is TDS_ENCRYPT_OFF (0x00). Login does not happen in encrypted manner in this case - password is sent by client in plain text in LOGIN7 message.

4. According the code, the loadEncryption defaults to 0 which is TDS_ENCRYPT_OFF. So, we're freeing the ssl structure towards the end of the function without setting up the socket. Also, according to the code, the login is probably not happening in an encrypted manner.

This is correct, loadEncryption was 0 by default, added a branch to explicitly set it to TDS_ENCRYPT_OFF (0x00).

Actually, let's not modify the SSL code at all. Something like as following (after confirming the SSL behavior):

SSL behaviour with jTDS is the following (assuming babelfishpg_tds.tds_ssl_encrypt = 'false'):

  1. ssl=off (in jTDS URL, also default): PRELOGIN is not sent, loadEncryption = TDS_ENCRYPT_OFF (0x00)

  2. ssl=request: PRELOGIN is sent with TDS_ENCRYPT_OFF (0x00), loadEncryption = TDS_ENCRYPT_OFF (0x00)

  3. ssl=require or ssl=authenticate: PRELOGIN is sent with TDS_ENCRYPT_ON (0x01), loadEncryption = TDS_ENCRYPT_ON (0x01)

we can have more safer check by checking whether client has ssl=off. If that's the case then only we should skip pre-login msg. Throw an FATAL error otherwise and should terminate connection

M, it seems that we don't have such information at the point of the check. First message (either PRELOGIN or LOGIN7) is always sent without SSL. Client SSL-request value is included in the PRELOGIN message itself.

Issues Resolved

2137

Test Scenarios Covered

JDBC test suite changes to support jTDS will follow in a separate PR.

Check List

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Alex Kasko alex@staticlibs.net

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 11161455702

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
contrib/babelfishpg_tds/src/backend/tds/tdslogin.c 16 22 72.73%
<!-- Total: 16 22 72.73% -->
Files with Coverage Reduction New Missed Lines %
contrib/babelfishpg_tds/src/backend/tds/tdslogin.c 1 76.61%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 11160101100: 0.003%
Covered Lines: 45125
Relevant Lines: 60589

💛 - Coveralls
sftim commented 5 hours ago

I see this is failing tests. Would you be willing to rebase it against the appropriate target branch @staticlibs ?

staticlibs commented 4 hours ago

@sftim

I've rebased the PR to the latest BABEL_4_X_DEV and the tests are passing now (previous failure was unrelated to this change).