SNOW-715551: Support authentication with OKTA #750

sfc-gh-ext-simba-jy · 2024-10-08T17:43:25Z

Implemented the okta authencation
Added the disable saml URL check option

cpp/lib/Authenticator.cpp

…ategy

sfc-gh-ext-simba-hx · 2024-10-11T21:40:24Z

CMakeLists.txt

@@ -155,6 +155,8 @@ set (SOURCE_FILES_PUT_GET
        cpp/StorageClientFactory.hpp
        cpp/StorageClientFactory.cpp
        cpp/RemoteStorageRequestOutcome.hpp
+        cpp/entities.cpp
+        cpp/entities.hpp


Since this is copied from ODBC I think better to make ODBC to reuse this later. Can we move entities.hpp to include/snowflake or move the function declaration into an existing header file there?

sfc-gh-ext-simba-hx · 2024-10-11T21:54:51Z

lib/http_perform.c

-        if ((retry) && (renew_timeout > 0) &&
-            ((time(NULL) - elapsedRetryTime) >= renew_timeout)) {
+        if (((retry) && (renew_timeout > 0) &&
+            ((time(NULL) - elapsedRetryTime) >= renew_timeout)) || renew_timeout < 0) {


This is somehow misleading, please put renew_timeout < 0 in a new line, retry in a new line, and others in the same line. I think the condition here is incorrectly, renew should not be triggered if retry is false.

This if expression is too long. How about?

SF_BOOL renew_timeout_reached = (renew_timeout > 0) && ((time(NULL) - elapsedRetryTime) >= renew_timeout) SF_BOOL renew_timeout_disabled = renew_timeout < 0 if ((retry && renew_timeout_reached) || renew_timeout_disabled)

I applied it @sfc-gh-jszczerbinski. Thank you for your suggestion.

sfc-gh-jszczerbinski · 2024-10-14T08:49:54Z

lib/http_perform.c

-        if ((retry) && (renew_timeout > 0) &&
-            ((time(NULL) - elapsedRetryTime) >= renew_timeout)) {
+        if (((retry) && (renew_timeout > 0) &&
+            ((time(NULL) - elapsedRetryTime) >= renew_timeout)) || renew_timeout < 0) {


This if expression is too long. How about?

SF_BOOL renew_timeout_reached = (renew_timeout > 0) && ((time(NULL) - elapsedRetryTime) >= renew_timeout) SF_BOOL renew_timeout_disabled = renew_timeout < 0 if ((retry && renew_timeout_reached) || renew_timeout_disabled)

lib/connection.c

cpp/lib/Authenticator.hpp

sfc-gh-ext-simba-hx · 2024-11-25T19:19:49Z

cpp/lib/Authenticator.cpp

@@ -73,6 +80,11 @@ extern "C" {
        conn->auth_object = static_cast<Snowflake::Client::IAuthenticator*>(
                              new Snowflake::Client::AuthenticatorJWT(conn));
      }
+      if (AUTH_OKTA == auth_type)


switch or else if?

sfc-gh-ext-simba-hx · 2024-11-25T19:40:00Z

cpp/lib/Authenticator.cpp

+          snowflake_cJSON_AddItemToObject(body, "data", data);
+    }
+
+    snowflake_cJSON_DeleteItemFromObject(data, "AUTHENTICATOR");


auth_update_json_body is a C wrapper for updateDataMap in C++. AUTH_OAUTH is an exception since it's quite simple so we don't have authenticator implementation for it in C++. For others we should fully rely on updateDataMap in case any common logic here might not apply.

I moved this to connection.c

sfc-gh-ext-simba-hx · 2024-11-25T19:43:42Z

cpp/lib/Authenticator.cpp

@@ -194,6 +212,10 @@ extern "C" {
      {


I think better to use
if (conn->auth_object)
{
delete static_castSnowflake::Client::IAuthenticator*(conn->auth_object);
}

sfc-gh-jszczerbinski

This PR is moving in the right direction. We need to improve some things before the merge:

Please rethink the inheritance structure. I don't think we need a complex one for this change
Please avoid using exceptions (https://google.github.io/styleguide/cppguide.html#Exceptions) and goto, it complicates the flow and makes it hard to reason about resource lifetimes.
Consider moving the code which handles SAML auth to separate file. Currently it dominates files that are containing implementations of other auth methods.
In C++ we use RAII to managed lifetimes of resources. When using resource from C API consider using unique_ptr with custom deleter (https://stackoverflow.com/questions/19053351/how-do-i-use-a-custom-deleter-with-a-stdunique-ptr-member)

sfc-gh-jszczerbinski · 2024-11-28T15:40:53Z

include/snowflake/IAuth.hpp

+    };
+
+
+    class IIDPAuthenticator


Is this class necessary? It is named like it should be an interface, but it has member variables.

Actually, this class is more close to an abstract class. I will summarize the reason of this in the ticket.

sfc-gh-jszczerbinski · 2024-11-28T15:41:27Z

include/snowflake/IAuth.hpp

+        std::string m_protocol;
+    };
+
+    class IAuthenticatorOKTA : public IIDPAuthenticator, public IAuthenticator


Multiple inheritance looks like an issue with underlying design. Can you get rid of it?

IIDPAuthenticator is for the HTTP call to get IDP info for Okta and ExternalBrowser in the future. IAuthenticator is the original Interface class for all authenticators in the libsnowflakeclient.

It makes sense, but also makes the code hard to read. Generally in C++ multiple inheritance is not advised and composition is preferred over implementation inheritance (https://google.github.io/styleguide/cppguide.html#Inheritance). Can we make IDPAuthenticator (perhaps could be a better name) a member of AuthenticatorOKTA instead?

I will try to redesign this based on your feedback.

sfc-gh-jszczerbinski · 2024-11-28T15:43:24Z

include/snowflake/SFURL.hpp

+   * 
+   * @return true if same prefix otherwise false
+   */
+  static bool urlHasSamePrefix(std::string url1, std::string url2);


We want to compare urls here? Maybe we should implement function like this instead.

bool hasSamePrefix(SFURL other);

This function is from ODBC and I try to use the same function without any changes. Actually, this is not a good place for this function. I will try to move this function elsewhere.

I moved this function to SNOWFLAKE_UTIL

sfc-gh-jszczerbinski · 2024-11-28T15:48:35Z

cpp/lib/IAuth.cpp

+                    "The specified authenticator is not supported, "
+                    "authenticator=%s, token url=%s, sso url=%s",
+                    m_authenticator.c_str(), tokenURLStr.c_str(), ssoURLStr.c_str());
+                AUTH_THROW("SFAuthenticatorVerificationFailed: the token URL does not have the same prefix with the authenticator");


Why do we need a macro for throwing an exception?

Also please avoid using exceptions. Based on our internal discussions we should try to follow google c++ style guide: https://google.github.io/styleguide/cppguide.html#Exceptions

Actually, this error throwing is to escape from the authentication block and go to auth_update_json_body or auth_renew_json_body. We have used this in JWT authentication as well.

sfc-gh-jszczerbinski · 2024-11-28T15:53:38Z

cpp/lib/Authenticator.cpp

+              snowflake_cJSON_Print(snowflake_cJSON_GetObjectItem(resp_data, "data")));
+          SET_SNOWFLAKE_ERROR(err, SF_STATUS_ERROR_GENERAL, "SFConnectionFailed", SF_SQLSTATE_GENERAL_ERROR);
+          AUTH_THROW(err);
+          goto cleanup;


This is dead code, we never reach clean up

I refactored this with try .. catch

sfc-gh-jszczerbinski · 2024-11-28T15:57:33Z

cpp/lib/Authenticator.cpp

+      std::string destination = url.toString();
+      void* curl_desc;
+      CURL* curl;
+      curl_desc = get_curl_desc_from_pool(destination.c_str(), m_connection->proxy, m_connection->no_proxy);


We are leaking curl descriptors right now. Also you are using C API, but C++ API for curl descriptors are also available

sfc-gh-jszczerbinski · 2024-11-28T16:06:27Z

cpp/lib/Authenticator.cpp

+      m_retryTimeout -= elapsedTime;
+      sf_header_destroy(httpExtraHeaders);
+      free_curl_desc(curl_desc);
+      SF_FREE(raw_resp);


We allocate with new and deallocate with SF_FREE

I fixed it as using SF_MALLOC instead of new when the NON_JSON_RESP pointer is created.

sfc-gh-jszczerbinski · 2024-11-28T16:07:37Z

cpp/lib/Authenticator.cpp

+  cleanup:
+      m_retryTimeout -= elapsedTime;
+      sf_header_destroy(httpExtraHeaders);
+      free_curl_desc(curl_desc);


Why do we free curl_desc here?

Sorry, it is unclear to me. I use free_curl_desc here because this is the end of the function. Don't you think that this is reasonable?

sfc-gh-jszczerbinski · 2024-11-28T16:12:53Z

cpp/lib/Authenticator.cpp

+              m_retryTimeout, elapsedTime);
+
+          SET_SNOWFLAKE_ERROR(err, SF_STATUS_ERROR_REQUEST_TIMEOUT, "OktaConnectionFailed: timeout reached", SF_SQLSTATE_GENERAL_ERROR);
+          goto cleanup;


Please don't use goto in C++ code. Use RAII and if you need to manage resource from C API use unique_ptr (https://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr constructors (3) and (4))

I use the exception instead of goto. In my opinion, using unique_ptr is inappropriate for this because I should move the pointer to call the function and get the pointer back from the HTTP perform.

sfc-gh-ext-simba-hx · 2024-12-02T19:45:28Z

Please avoid using exceptions (https://google.github.io/styleguide/cppguide.html#Exceptions) and goto, it complicates the flow and makes it hard to reason about resource lifetimes.

The c++ implementation in libsnowflakeclient are mainly for reusing in ODBC, which is using exception for error handling. That's from the SimbaEngineSDK framework and unlikely that could be changed. Keep using exception would make it easier to reuse it in ODBC, especially when the implementation is originally from ODBC.
Or maybe we could consider not to reuse it in ODBC? That would make the logic (including the inheritance structure) a lot simpler.

…lakeclient into SNOW-715551

sfc-gh-ext-simba-jy · 2024-12-05T21:07:53Z

As we decide to follow Google c++ style, I will create a new PR instead of refactoring the code on the current PR. I will close this PR after I create a new one.

sfc-gh-ext-simba-jy · 2024-12-17T00:44:47Z

Closed this PR. I made a new one. #799

sfc-gh-ext-simba-jy added 12 commits September 13, 2024 12:12

save

22cd9c4

save

ed99744

implemented the okta authentication

37bca3e

comment out the connection manual testing

b76617c

Merge branch 'master' into SNOW-715551

91bcf7e

added comments and updated codes

6204854

add entities files

73468ec

fix

555cbfb

fix

4be9c7f

fix

bf2ca7d

fix testing cases

fd5caa0

fixed the error on mac

4271b2c

sfc-gh-ext-simba-jy requested a review from sfc-gh-ext-simba-hx October 9, 2024 03:52

removed unnecessary changes

fd57683

sfc-gh-ext-simba-hx reviewed Oct 9, 2024

View reviewed changes

cpp/lib/Authenticator.cpp Outdated Show resolved Hide resolved

sfc-gh-ext-simba-hx reviewed Oct 9, 2024

View reviewed changes

cpp/lib/Authenticator.cpp Outdated Show resolved Hide resolved

sfc-gh-ext-simba-jy added 7 commits October 9, 2024 15:54

added retry parameters in the curl_get_call and applied the retry str…

d8680d0

…ategy

Merge branch 'master' into SNOW-715551

127cd76

add one more condition for the retry strategy

48aae9f

removed the error in catch

9245aae

removed unnecessary changes

40077ca

fix

2c56894

comment out manual test

6e58935

sfc-gh-ext-simba-jy requested a review from sfc-gh-ext-simba-hx October 10, 2024 17:27

sfc-gh-ext-simba-jy added 2 commits October 10, 2024 11:09

add retried_count increment on authentication level

ef7d51a

Merge branch 'master' into SNOW-715551

d7d9967

sfc-gh-ext-simba-hx reviewed Oct 11, 2024

View reviewed changes

sfc-gh-jszczerbinski requested changes Oct 14, 2024

View reviewed changes

Merge branch 'master' into SNOW-715551

5b84cd2

sfc-gh-ext-simba-jy added 3 commits November 15, 2024 10:45

lint fix

df7d539

refactoring IAuth

bf71bb3

add IAuth files

1f37bf8

sfc-gh-ext-simba-hx reviewed Nov 25, 2024

View reviewed changes

sfc-gh-ext-simba-jy and others added 8 commits November 25, 2024 18:18

fix minior errors

cd3cf01

Merge branch 'master' into SNOW-715551

44bdf3a

fix

8eb336a

merge

d1312a8

testing

24af0a6

testing

c5be612

fix

b613860

refactor

3bcbb91

sfc-gh-ext-simba-jy marked this pull request as ready for review November 26, 2024 20:52

sfc-gh-ext-simba-jy requested a review from a team as a code owner November 26, 2024 20:52

sfc-gh-ext-simba-jy requested review from sfc-gh-jszczerbinski and sfc-gh-dprzybysz November 27, 2024 00:22

Merge branch 'master' into SNOW-715551

f15b3a3

sfc-gh-jszczerbinski requested changes Nov 28, 2024

View reviewed changes

sfc-gh-ext-simba-jy added 4 commits December 2, 2024 14:53

update

77278da

Merge branch 'SNOW-715551' of https://github.com/snowflakedb/libsnowf…

2488367

…lakeclient into SNOW-715551

move urlSamePrefix function to snowflake util

38f0619

remove testing from unit_sfurl

1f178ec

sfc-gh-ext-simba-jy requested a review from sfc-gh-jszczerbinski December 3, 2024 23:01

sfc-gh-ext-simba-jy added 2 commits December 4, 2024 09:36

Merge branch 'master' into SNOW-715551

6da2082

Merge branch 'master' into SNOW-715551

c40efaa

SNOW-715551: Support authentication with OKTA #750

Are you sure you want to change the base?

SNOW-715551: Support authentication with OKTA #750

Conversation

sfc-gh-ext-simba-jy commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

sfc-gh-ext-simba-hx Oct 11, 2024 • edited Loading

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

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

sfc-gh-jszczerbinski left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-jy Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

sfc-gh-jszczerbinski Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-jy Dec 2, 2024 • edited Loading

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

sfc-gh-ext-simba-hx commented Dec 2, 2024

sfc-gh-ext-simba-jy commented Dec 5, 2024

sfc-gh-ext-simba-jy commented Dec 17, 2024

sfc-gh-ext-simba-jy commented Oct 8, 2024 •

edited

Loading

sfc-gh-ext-simba-hx Oct 11, 2024 •

edited

Loading

sfc-gh-jszczerbinski left a comment •

edited

Loading

sfc-gh-ext-simba-jy Nov 29, 2024 •

edited

Loading

sfc-gh-jszczerbinski Nov 28, 2024 •

edited

Loading

sfc-gh-ext-simba-jy Dec 2, 2024 •

edited

Loading