ODBC SSL Compliance Fix #75

forestmvey · 2022-06-17T21:35:37Z

Description

Fixed ODBC Driver Issues:

Driver ignores the UseSSL flag
SQL plugin query fails if opensearch instance has SSL enabled, and host url doesn't use https:// prefix
UI allows for mismatch between UseSSL flag, and specified protocol in host url
Driver defaults to OpenDistro sql endpoint in event of error with SQL plugin query

Issues Resolved

Github: 280

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…oint bug when using SSL without protocol specified in url Signed-off-by: forestmvey <forestv@bitquilltech.com>

…he url Signed-off-by: forestmvey <forestv@bitquilltech.com>

…tribution node Signed-off-by: forestmvey <forestv@bitquilltech.com>

… url and/or incompatible with server url Signed-off-by: forestmvey <forestv@bitquilltech.com>

Signed-off-by: forestmvey <forestv@bitquilltech.com>

codecov · 2022-06-17T21:48:14Z

Codecov Report

Merging #75 (4d88664) into integ-odbc_ssl_compliance_fix (26058b8) will not change coverage.
The diff coverage is n/a.

@@                       Coverage Diff                        @@
##             integ-odbc_ssl_compliance_fix      opensearch-project/sql#75   +/-   ##
================================================================
  Coverage                            97.66%   97.66%           
  Complexity                            2743     2743           
================================================================
  Files                                  268      268           
  Lines                                 6772     6772           
  Branches                               435      435           
================================================================
  Hits                                  6614     6614           
  Misses                                 157      157           
  Partials                                 1        1

Flag	Coverage Δ
sql-engine	`97.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26058b8...4d88664. Read the comment docs.

sql-odbc/src/IntegrationTests/ITODBCConnection/test_odbc_connection.cpp

sql-odbc/src/sqlodbc/opensearch_connection.cpp

MaxKsyunz · 2022-06-17T22:02:07Z

sql-odbc/src/sqlodbc/opensearch_connection.cpp

+        if(rt_opts.crypt.use_ssl) {
+            rt_opts.conn.server = std::string("https://") + std::string(self->connInfo.server);
+        } else {
+            rt_opts.conn.server = std::string("http://") + std::string(self->connInfo.server);


rt_opts.conn.server should follow the same assignment patter as other properties in this method -- rt_opts.conn.server.assign(...)

sql-odbc/src/sqlodbc/opensearch_connection.cpp

sql-odbc/src/sqlodbc/opensearch_communication.cpp

MaxKsyunz · 2022-06-17T22:17:10Z

sql-odbc/src/sqlodbc/opensearch_communication.cpp

@@ -1005,7 +1009,9 @@ std::string OpenSearchCommunication::GetClusterName() {

 void OpenSearchCommunication::SetSqlEndpoint() {
    std::string distribution = GetServerDistribution();
-    if (distribution.compare("opensearch") == 0) {
+    if (distribution.empty()) {
+        sql_endpoint = "Error";


If this is an error state, is there a better way to propagate the error to the user?

Is it possible for distribution to be empty?

Distribution is empty in the event of GetServerDistribution() having an exception caught. I figured having Error for the endpoint is better than the previous /_opendistro/_sql when unable to query sql plugin. I am open to something else.

I see.

So, with this change, if GetServerDistribution returns "", then SetSqlEndpoint sets sql_endpoint to "Error", then EstablishConnection proceeds to try to connect to host Error, probably fails and returns false?

If that's correct, then it's better if EstablishConnection returns false after SetSqlEndpoint.

…ut server url generation into own function. Minor logic revisions. Signed-off-by: forestmvey <forestv@bitquilltech.com>

…ausing an unusable UI state for windows users. Signed-off-by: forestmvey <forestv@bitquilltech.com>

…failure to establish connection with url Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto · 2022-06-20T21:35:57Z

sql-odbc/src/sqlodbc/opensearch_communication.cpp

        SetErrorDetails("Connection error", m_error_message,
                        ConnErrorType::CONN_ERROR_COMM_LINK_FAILURE);
        LogMsg(OPENSEARCH_ERROR, m_error_message.c_str());
+        m_error_message_to_user = m_error_message;
        return "";


why are we returning "" in all cases? Does it make sense to reconsider the response type of this function? Change to return void, or change the returns so that they return actual content ....?

Note: this is the problem with not have a javadoc.
The API contract for this function isn't described, which means I have to read the code and look up all the usages of this function to understand WHAT string is expected from this function.
Please add a javadoc header comment to the function you know what the parameters/returns are supposed to be. Then determine if a string return type is appropriate.

MaxKsyunz · 2022-06-20T23:11:29Z

sql-odbc/src/sqlodbc/opensearch_connection.cpp

@@ -73,6 +75,26 @@ char CC_connect(ConnectionClass *self) {
    return 1;
 }

+std::string generateValidServerUrl(ConnectionClass *self) {
+    std::string valid_server_url = "";
+    if(!strlen(self->connInfo.server)) {


I think, you'll simplify this function if you wrap self->connInfo.server into a string_view.

MaxKsyunz · 2022-06-20T23:42:56Z

sql-odbc/src/sqlodbc/opensearch_connection.cpp

+        return valid_server_url;
+    }
+
+    bool url_len_valid = (strlen(self->connInfo.server) > strlen(HTTPS_PREFIX));


Why is url_len_valid necessary?

std::string::find: "If s does not point to an array long enough, it causes undefined behavior".
https://cplusplus.com/reference/string/string/find/

…g valid server URL's with string_view usage. Signed-off-by: forestmvey <forestv@bitquilltech.com>

1. Remove unnecessary throws exception. 2. Use java style array declaration. 3. Remove unused variables. 4. Replace lambda with method references 5. Replace statement with lambda 6. Use valid random string Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

forestmvey added 6 commits June 8, 2022 16:25

Defaulting to https protocol for ODBC connection, and fixing SQL endp…

8ea36ee

…oint bug when using SSL without protocol specified in url Signed-off-by: forestmvey <forestv@bitquilltech.com>

Add Integration Tests for edge cases of specified host protocols in t…

fd6f791

…he url Signed-off-by: forestmvey <forestv@bitquilltech.com>

Improved diagnostics in event of failure to query sql plugin with dis…

97718cf

…tribution node Signed-off-by: forestmvey <forestv@bitquilltech.com>

Disabling SSL protocol relevant checkboxes when protocol specified in…

9180b88

… url and/or incompatible with server url Signed-off-by: forestmvey <forestv@bitquilltech.com>

Fix setting dialog information prior to advanced dialog selection

065b29f

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Adding relevant SSL enabled test

629fe21

Signed-off-by: forestmvey <forestv@bitquilltech.com>

forestmvey requested a review from a team June 17, 2022 21:38

MitchellGale approved these changes Jun 17, 2022

View reviewed changes

acarbonetto requested changes Jun 17, 2022

View reviewed changes

MaxKsyunz requested changes Jun 17, 2022

View reviewed changes

acarbonetto approved these changes Jun 20, 2022

View reviewed changes

Add error case when specified protocol mismatched with UseSSL flag. P…

af7c34b

…ut server url generation into own function. Minor logic revisions. Signed-off-by: forestmvey <forestv@bitquilltech.com>

forestmvey force-pushed the odbc_ssl_compliance_fix branch from 630d961 to af7c34b Compare June 20, 2022 20:32

forestmvey added 2 commits June 20, 2022 13:50

Removed state to check for mismatch in server protocols due to this c…

864ee58

…ausing an unusable UI state for windows users. Signed-off-by: forestmvey <forestv@bitquilltech.com>

Updating error message to user due to updates in logical ordering in …

3a72cd7

…failure to establish connection with url Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto reviewed Jun 20, 2022

View reviewed changes

MaxKsyunz reviewed Jun 20, 2022

View reviewed changes

Adding comments to altered functions. Simplifying logic for generatin…

4d88664

…g valid server URL's with string_view usage. Signed-off-by: forestmvey <forestv@bitquilltech.com>

Yury-Fridlyand approved these changes Jun 22, 2022

View reviewed changes

MaxKsyunz approved these changes Jun 22, 2022

View reviewed changes

forestmvey merged commit 61e9a7b into integ-odbc_ssl_compliance_fix Jun 22, 2022

Yury-Fridlyand deleted the odbc_ssl_compliance_fix branch January 12, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ODBC SSL Compliance Fix #75

ODBC SSL Compliance Fix #75

forestmvey commented Jun 17, 2022

codecov bot commented Jun 17, 2022 •

edited

Loading

MaxKsyunz Jun 17, 2022

MaxKsyunz Jun 17, 2022

forestmvey Jun 17, 2022 •

edited

Loading

MaxKsyunz Jun 17, 2022 •

edited

Loading

acarbonetto Jun 20, 2022

acarbonetto Jun 20, 2022

MaxKsyunz Jun 20, 2022 •

edited

Loading

MaxKsyunz Jun 20, 2022

forestmvey Jun 22, 2022

ODBC SSL Compliance Fix #75

ODBC SSL Compliance Fix #75

Conversation

forestmvey commented Jun 17, 2022

Description

Issues Resolved

Check List

codecov bot commented Jun 17, 2022 • edited Loading

Codecov Report

MaxKsyunz Jun 17, 2022

Choose a reason for hiding this comment

MaxKsyunz Jun 17, 2022

Choose a reason for hiding this comment

forestmvey Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

MaxKsyunz Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

acarbonetto Jun 20, 2022

Choose a reason for hiding this comment

acarbonetto Jun 20, 2022

Choose a reason for hiding this comment

MaxKsyunz Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

MaxKsyunz Jun 20, 2022

Choose a reason for hiding this comment

forestmvey Jun 22, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 17, 2022 •

edited

Loading

forestmvey Jun 17, 2022 •

edited

Loading

MaxKsyunz Jun 17, 2022 •

edited

Loading

MaxKsyunz Jun 20, 2022 •

edited

Loading