SqlConnection: allow configurable encryption via SQL_COPT_SS_ENCRYPT - #578
Open
Yaraslaut wants to merge 5 commits into
Open
SqlConnection: allow configurable encryption via SQL_COPT_SS_ENCRYPT#578Yaraslaut wants to merge 5 commits into
Yaraslaut wants to merge 5 commits into
Conversation
Connections had no way to request (or refuse) TLS encryption other than spelling out a driver keyword in a raw connection string. On the DSN-based connect path (`SQLConnect`) there is no connection string to put that keyword in, so encryption was simply not configurable there at all. Add `SqlEncryptionMode` (`DriverDefault` / `Disabled` / `Enabled`) and a matching `SqlConnectionDataSource::encryption` field, following the existing `SqlIsolationMode::DriverDefault` precedent: the default leaves the attribute untouched, so a caller that does not opt in behaves exactly as before. When the caller does opt in, `SQL_COPT_SS_ENCRYPT` is applied to the connection handle before `SQLConnectW`. That attribute is pre-connect, so the server type is not known yet and cannot be branched on -- which is why only an explicit opt-in touches it. A driver that rejects the attribute fails the connection rather than establishing it: silently downgrading a requested encrypted connection to plaintext would be the wrong failure mode for a security setting. The setting also survives the flattening into a connection string: `ToConnectionString()` emits `Encrypt=yes|no` (and nothing at all for `DriverDefault`), `FromConnectionString()` parses it back, and `SetDefaultDataSource()` now delegates to `ToConnectionString()` instead of re-formatting a subset of the fields, so the knob is not dropped on that path. `SQL_COPT_SS_ENCRYPT` and its `SQL_EN_*` values live in Microsoft's `msodbcsql.h`, which unixODBC does not ship; the values are mirrored locally rather than taking a dependency on that header. Closes #14 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…-trip more spellings Applying /code-review findings on this branch. The one DB-level encryption test asserted `SELECT 42` returns 42, which is equally true over a plaintext connection -- it passed whether or not the keyword was honoured. Assert sys.dm_exec_connections.encrypt_option == 'TRUE' instead; verified against the live SQL Server 2022 container. The same test wrote its overrides as "Encrypt"/"TrustServerCertificate" while ParseConnectionString upper-cases keys, so the entries were appended next to the existing TRUSTSERVERCERTIFICATE rather than replacing it, and a connection string carrying both spellings has no defined winner. Use the upper-cased keys. ParseEncryptionMode mapped every unrecognized value to DriverDefault and ToConnectionString then omits the keyword entirely, so a FromConnectionString -> ToConnectionString round-trip silently dropped the setting. Add Driver 18's `mandatory` and `optional` to the spelling table and document that `strict` has no SQL_COPT_SS_ENCRYPT representation and is dropped on re-emit. Connect() reuses the DBC handle across reconnects, so an attribute set by an earlier Enabled connect survives into a later DriverDefault one. ODBC offers no "restore driver default", so this is documented inline rather than fixed -- an actual fix means re-allocating the handle. Rewrite both spelling lookups as range-based loops over the descriptor table: readability-qualified-auto (enabled via readability-*) fires on the std::array iterator, and its suggested `auto const* const` would not compile under MSVC, whose std::array iterator is a class type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The "Windows Tests (MS SQL Server (LocalDB))" leg failed with an escaping exception: 08001 (20) "[ODBC Driver 17 for SQL Server]Encryption not supported on SQL Server". SQL Server Express LocalDB serves no TLS endpoint, so it refuses the `Encrypt=yes` handshake outright. Every other SQL Server leg - Docker 2017 / 2019 / 2022 and the ODBC 17 / 18 Windows legs - connects fine. `UNSUPPORTED_DATABASE` keys on `ServerType`, and LocalDB is `MICROSOFT_SQL` like the rest, so it cannot express "this instance, not this DBMS". Connect through the non-throwing `Connect()` overload instead and skip only when the driver reports that specific refusal; any other connect failure now fails the test with its SQLSTATE, so a genuine regression in the `Encrypt=` plumbing is still caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The clang-tidy job (previously cancelled by the matrix fail-fast, so this only surfaced once the LocalDB failure was out of the way) rejected two aggregate initializers with clang-diagnostic-missing-designated-field-initializers: `username` and `password` carry no default member initializer, so a designated list that names only `datasource` and `encryption` leaves them uninitialized by the list. Spell both fields out, as the round-trip test directly above already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The enumeration has a fixed underlying type, so a value outside the named enumerators is representable rather than undefined behaviour — it reaches Lightweight from an ABI mismatch against a differently-versioned build, or from a plain cast. The keyword table then finds no match and the function must return an empty spelling, which BuildConnectionString omits, rather than putting a garbage `Encrypt=` value in front of the driver. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14
What
Adds
SqlEncryptionMode(DriverDefault/Disabled/Enabled) and a matchingSqlConnectionDataSource::encryptionfield, so an application can explicitly request — or refuse —a TLS-encrypted connection.
Why this shape
Previously the only way to configure encryption was to spell out a driver keyword in a raw
connection string. On the DSN-based connect path (
SQLConnect) there is no connection string to putthat keyword in, so encryption was not configurable there at all — which is exactly the gap
SQL_COPT_SS_ENCRYPTexists to close.Three judgment calls worth reviewing, since the issue specifies the mechanism but not the API:
DriverDefaultsentinel, following the existingSqlIsolationMode::DriverDefaultprecedent (
SqlTransaction.cpp:19). The default leaves the attribute untouched, so anything thatdoes not opt in behaves bit-for-bit as before.
SQL_COPT_SS_ENCRYPTis a pre-connect attribute, so the server type is not knownyet and cannot be branched on — which is why only an explicit opt-in touches it at all. If the
caller did opt in and the driver rejects the attribute, the connection fails instead of being
established. Silently downgrading a requested encrypted connection to plaintext seemed like the
wrong failure mode for a security setting; happy to flip this if you disagree.
ToConnectionString()emitsEncrypt=yes|no(nothing at all forDriverDefault),FromConnectionString()parses it back, andSetDefaultDataSource()now delegates toToConnectionString()rather than re-formatting a subsetof the fields — otherwise that path would silently drop the knob. (
std::formatter<SqlConnectInfo>was duplicating the same format string and is now delegated too.)
SQL_COPT_SS_ENCRYPTand itsSQL_EN_*values live in Microsoft'smsodbcsql.h, which unixODBC doesnot ship, so the values are mirrored locally rather than adding a dependency on that header.
Tests
SqlConnectInfoEdgeTests.cpp: keyword parsing (all documented spellings,case-insensitive, whitespace-tolerant, unknown →
DriverDefault), rendering, round-tripping,comparison, and a regression guard that a non-opted-in data source renders byte-for-byte as before.
SqlConnectionDbTests.cpp: opens an explicitly encrypted connection against SQLServer and round-trips a query over it.
UNSUPPORTED_DATABASE-gated for the other backends, whichconfigure TLS through their own keywords.
Coverage gap, stated explicitly: the
SQLSetConnectAttr(SQL_COPT_SS_ENCRYPT)call itself is on theDSN connect path, which needs a registered DSN and so is not reachable from the CI harness (all test
envs use connection strings). The end-to-end test therefore exercises the equivalent
connection-string path. The attribute application itself is covered only by construction and review.
Databases tested
sqlite3mssql2022(Docker,mcr.microsoft.com/mssql/server:2022-latest)postgres(Docker 16.4)The new encrypted-connection case was confirmed to actually run (not skip) and pass on
mssql2022.Compilers tested
clang-debug(ASan + UBSan + pedantic-Werror) — all three databases above. This is the one thatran the suite.
AGENT.mdstep 3. Thegcc-releasepreset is Linux-gated(
Cannot use disabled configure preset) and this is macOS. I configured GCC 15 manually withLIGHTWEIGHT_BUILD_MODULES=ONinstead;SqlConnection.cppandSqlConnectInfo.cppboth compiledclean under it, but the build cannot complete on macOS for reasons unrelated to this change (see
below). The GCC and modules legs need CI to be the judge.
Pre-existing issues found while validating (not fixed here)
Building
gcc-release -D LIGHTWEIGHT_BUILD_MODULES=ONwith GCC 15 on macOS fails on untouched code.Flagging in case they bite on a compiler bump — CI currently pins GCC 14:
SqlLogger.cpp:291:'std::stacktrace' has not been declared(Homebrew libstdc++ lacks it; theLIGHTWEIGHT_HAVE_STDCXXEXPprobe fails and the#ifguard then leaves the call unguarded).SqlConnectInfo.hpp'sPrefetchDepthDefault(namespace-scopeconstexpr, needsinline constexpr),detail::kDefaultRowArrayFetchDepth, andReflection::MaxReflectionMemerCount(in the vendoredreflection-cppdep). None involve this PR's new symbols.Performance impact
None. One extra
SQLSetConnectAttrper connection, and only when the caller opts in; the default pathadds a single predictable enum comparison. No allocation added on any hot path.
Risk assessment
Low. The entire feature is inert unless a caller sets
encryptionto something other thanDriverDefault. The one behaviour change that reaches non-opted-in code isSetDefaultDataSource()delegating to
ToConnectionString()— verified to produce a byte-identical string in that case, with aregression test pinning it.
ABI:
SqlConnectionDataSourcegrows a member, so this is a breaking ABI change for that struct (sourcecompatible; the defaulted
operator<=>now also compares the new field).