MSSQLSPATIAL: Get UID and PWD from environment variable #6797

ninsbl · 2022-11-25T14:58:08Z

MS SQL supports only two authentication modes: TrustedConnection or username and password given as UID and PWD in the connection string.
If TrustedConnection is not avaialable, login credentials can be exposed to other users when command line tools (ogrinfo or ogr2ogr) are used.

A somewhat more secure way to handle login information, is to store it in environment variables.

What does this PR do?

This PR allows the MSSQLSPATIAL driver to fetch UID and PWD from environment variables MSSQLSPATIAL_UID and MSSQLSPATIAL_PWD, if no other authentication information is provided. That way UID and PWD do not have to be provided on the command line.

Tasklist

Environment

OS: Linux

rouault · 2022-11-25T15:43:36Z

ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialdatasource.cpp

@@ -690,6 +690,9 @@ int OGRMSSQLSpatialDataSource::Open( const char * pszNewName, bool bUpdate,
    char* pszGeometryFormat = nullptr;
    char* pszConnectionName = CPLStrdup(pszNewName + 6);
    char* pszDriver = nullptr;
+    char* pszUID = nullptr;


those variables should also be free'd towards the end of the method

See: 2db0d53

rouault · 2022-11-25T15:43:50Z

ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialdatasource.cpp

+            nCurrent, nNext, nTerm, FALSE))
+            continue;
+
+        if (ParseValue(&pszTrustedConnection, pszConnectionName, "trustedconnection=",


pszTrustedConnection not used currently

Originally, I planned not to query UID and PWD if TrustedConnection is specified in the connection string. But then I thought, that if env variables are set, that has been deliberate too. So, if UID and PWD are defined in the environment variables, it may be safe to add it anyway.
Alternatively, the block where UID and PWD are injected in the connection string can be only executed only if the connection string dies not contain TrustedConnection... Let me know what you prefer...

Let me know what you prefer...

no idea. I'm not familiar at all with MSSQLSpatial authentication. But I don't really understand your comment: I don't see anything in the code (latest version) that tests for "trustedconnection".

Sorry for not being clear. yes, with the latest commit I removed the check trusted_connection. In the comment I just wanted to clarify, why I added it at first...

I am no MS SQL expert either. A second look at the authentication documentation tells me, that trusted_connection can also be stored in the odbc.ini as part of the DSN. So it will not be part of the connection string.

Therefore, I think the current implementation that does not check for that keyword is appropriate. If a user creates the MSSQLSPATIAL_UID and MSSQLSPATIAL_PWD environment variables, they are used and get priority over trusted_connection.

A bit more clarification in the docs can be useful. Will have a look at that...

ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialdatasource.cpp

rouault · 2022-11-25T16:34:46Z

CC @szekerest

mloskot · 2022-11-25T17:18:04Z

doc/source/drivers/vector/mssqlspatial.rst

+through username (**UID**) and password (**PWD**). As providing username
+and password on the commandline can be a security issue, login credentials
+can also be more securely stored in user defined environment variables
+*MSSQLSPATIAL_UID* and *MSSQLSPATIAL_PWD*.


I'd suggest to use GDAL-specific names GDAL_MSSQLSPATIAL_UID and GDAL_MSSQLSPATIAL_PWD

Well, existing config options https://gdal.org/drivers/vector/mssqlspatial.html#configuration-options all starts with MSSQLSPATIAL_, so better keep with that scheme

The docs call these two environment variables, but I guess they are not distinguished from config options

they are queried with CPLGetConfigOption(), so can be set through env. variable or config options

I` ll go with whatever name for the variables you prefer...

Co-authored-by: Even Rouault <even.rouault@spatialys.com>

rouault · 2022-11-27T23:42:20Z

doc/source/drivers/vector/mssqlspatial.rst

+
+      export MSSQLSPATIAL_UID=user
+      export MSSQLSPATIAL_PWD=pwd
+      ogrinfo -al   MSSQL:server=.\MSSQLSERVER2008;database=geodb;trusted_connection=no


is is trusted_connection or trustedconnection ? There's a mix of boths in your PR

Thanks for pointing that out. It should be trusted_connection. Will double check and fix accordingly.

ninsbl · 2022-11-28T09:35:01Z

I just tested the code in this PR with an internal MS SQL Server and can confirm that it works as expected.

The authentication documentation for the driver could be improved. The challenge however is, that authentication is handled differently for different OS (MS Windows vs. unix like systems), and different drivers.

On MS Windows, neither driver nor authentication method needs to be provided, internal ones are used by default. And no odbc.ini is needed at all.

On Linux, the FreeTDS driver for example uses the ServerName keyword, if no DSN is used that is defined in a odbc.ini, while the ODBC driver from Microsoft uses the server keyword.

If you consider such detailed documentation useful, I can add some sentences to the docs. Otherwise, I would limit documentation changes to the added functionality...

doc/source/drivers/vector/mssqlspatial.rst

rouault · 2022-11-28T12:46:16Z

If you consider such detailed documentation useful, I can add some sentences to the docs. Otherwise, I would limit documentation changes to the added functionality...

I'd say just finalize this PR with its current scope, which should be just a matter of integrating the proposed change I made regarding TrustedConnection -> trusted_connection.
Doc enhancements regarding authentication are welcome, but could be part of another PR dedicated to them, to make things manageable

Co-authored-by: Even Rouault <even.rouault@spatialys.com>

ninsbl · 2022-11-28T19:41:02Z

If you consider such detailed documentation useful, I can add some sentences to the docs. Otherwise, I would limit documentation changes to the added functionality...

I'd say just finalize this PR with its current scope, which should be just a matter of integrating the proposed change I made regarding TrustedConnection -> trusted_connection. Doc enhancements regarding authentication are welcome, but could be part of another PR dedicated to them, to make things manageable

Fine with me. I just committed the proposed change.
OK for me to merge.

Do you see any chance to backport this? It is minor, yet security related change...

ninsbl added 3 commits November 25, 2022 08:49

MSSQL get UID/PWD from env

ab93634

MSSQL get UID/PWD from env

056f556

remove tabs

957faab

ninsbl changed the title ~~MSSQLSPATIAL: Get UID and PWD from environment variable~~ [WIP] MSSQLSPATIAL: Get UID and PWD from environment variable Nov 25, 2022

rouault reviewed Nov 25, 2022

View reviewed changes

mloskot reviewed Nov 25, 2022

View reviewed changes

ninsbl and others added 3 commits November 27, 2022 22:01

Update ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialdatasource.cpp

09fb086

Co-authored-by: Even Rouault <even.rouault@spatialys.com>

Update ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialdatasource.cpp

46973a7

Co-authored-by: Even Rouault <even.rouault@spatialys.com>

address review

2db0d53

rouault reviewed Nov 27, 2022

View reviewed changes

rouault reviewed Nov 28, 2022

View reviewed changes

doc/source/drivers/vector/mssqlspatial.rst Outdated Show resolved Hide resolved

Update doc/source/drivers/vector/mssqlspatial.rst

510d4e3

Co-authored-by: Even Rouault <even.rouault@spatialys.com>

ninsbl changed the title ~~[WIP] MSSQLSPATIAL: Get UID and PWD from environment variable~~ MSSQLSPATIAL: Get UID and PWD from environment variable Nov 28, 2022

rouault added the backport release/3.6 label Nov 28, 2022

rouault merged commit 47ff50d into OSGeo:master Nov 29, 2022

rouault mentioned this pull request Nov 29, 2022

[Backport release/3.6] MSSQLSPATIAL: Get UID and PWD from environment variable #6818

Merged

geographika mentioned this pull request Feb 4, 2023

MSSQL Connection Strings no longer work with User Id and Password #7179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSSQLSPATIAL: Get UID and PWD from environment variable #6797

MSSQLSPATIAL: Get UID and PWD from environment variable #6797

ninsbl commented Nov 25, 2022 •

edited

rouault Nov 25, 2022

ninsbl Nov 27, 2022

rouault Nov 25, 2022

ninsbl Nov 27, 2022

rouault Nov 27, 2022

ninsbl Nov 28, 2022

rouault commented Nov 25, 2022

mloskot Nov 25, 2022

rouault Nov 25, 2022 •

edited

mloskot Nov 25, 2022

rouault Nov 25, 2022

ninsbl Nov 27, 2022

rouault Nov 27, 2022

ninsbl Nov 28, 2022

ninsbl commented Nov 28, 2022

rouault commented Nov 28, 2022

ninsbl commented Nov 28, 2022

MSSQLSPATIAL: Get UID and PWD from environment variable #6797

MSSQLSPATIAL: Get UID and PWD from environment variable #6797

Conversation

ninsbl commented Nov 25, 2022 • edited

What does this PR do?

Tasklist

Environment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault commented Nov 25, 2022

Choose a reason for hiding this comment

rouault Nov 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninsbl commented Nov 28, 2022

rouault commented Nov 28, 2022

ninsbl commented Nov 28, 2022

ninsbl commented Nov 25, 2022 •

edited

rouault Nov 25, 2022 •

edited