SNOW-715524: Add SSO token cache #921

sfc-gh-ext-simba-lf · 2024-04-18T17:01:00Z

Description

Add SSO token cache

Checklist

Code compiles correctly
Code is formatted according to Coding Conventions
Created tests which fail without the change (if possible)
All tests passing (dotnet test)
Extended the README / documentation, if necessary
Provide JIRA issue id (if possible) or GitHub issue id in PR name

codecov · 2024-04-18T18:31:34Z

Codecov Report

Attention: Patch coverage is 92.53333% with 28 lines in your changes missing coverage. Please review.

Project coverage is 87.23%. Comparing base (c3cb344) to head (126e337).

Files with missing lines	Patch %	Lines
Snowflake.Data/Core/Tools/BrowserOperations.cs	5.55%	17 Missing ⚠️
...astructure/SFCredentialManagerWindowsNativeImpl.cs	92.15%	2 Missing and 2 partials ⚠️
...Core/Authenticator/ExternalBrowserAuthenticator.cs	96.96%	3 Missing ⚠️
...ager/Infrastructure/SFCredentialManagerFileImpl.cs	98.09%	0 Missing and 2 partials ⚠️
...re/CredentialManager/SFCredentialManagerFactory.cs	95.83%	0 Missing and 1 partial ⚠️
Snowflake.Data/Core/Session/SFSession.cs	96.15%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #921      +/-   ##
==========================================
+ Coverage   85.70%   87.23%   +1.53%     
==========================================
  Files         117      122       +5     
  Lines       11331    11614     +283     
  Branches     1130     1158      +28     
==========================================
+ Hits         9711    10132     +421     
+ Misses       1342     1196     -146     
- Partials      278      286       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…nector-net into SNOW-715524-SSO-Token-Cache

Snowflake.Data/Snowflake.Data.csproj

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerFactory.cs

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerAdysTechImpl.cs

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerFileImpl.cs

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerInMemoryImpl.cs

Snowflake.Data/Core/Session/SFSession.cs

rafael-sotelo · 2024-05-16T15:14:49Z

Any updates on this PR ? Can I help ? I been waiting for this SSO Token Cache for long time

sfc-gh-ext-simba-lf · 2024-05-16T17:02:32Z

Any updates on this PR ? Can I help ? I been waiting for this SSO Token Cache for long time

Currently we're trying out different libraries/packages for the credential manager. Then it'll be reviewed if it meets the security requirements

Snowflake.Data/Client/SnowflakeCredentialManager/ISnowflakeCredentialManager.cs

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerFactory.cs

sfc-gh-knozderko · 2024-06-05T14:55:58Z

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerInMemoryImpl.cs

+        public void SaveCredentials(string key, string token)
+        {
+            s_logger.Debug($"Saving credentials into memory for key: {key}");
+            s_credentials[key] = SnowflakeCredentialEncryption.EncryptToken(token);


Why do we encrypt tokens when storing them in memory whereas in file implementation we do not do it?
I think encryption here it is not necessary... Maybe only we should make sure that ToString() operation on this class does not reveal any credentials.

I think I saw JDBC do some encryption so I tried to follow it

The encryption is now removed. I also checked ToString() and it only shows the class name so no sensitive info. The dictionary itself is private so it shouldn't be possible to call ToString() on the dictionary itself

maybe, you are right some kind of obfuscating the string would be good. Maybe let's use SecureString to store the tokens. You could use Snowflake.Data.Core.Tools.SecureStringHelper

Sure, the tokens are now stored with SecureStringHelper

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerInMemoryImpl.cs

sfc-gh-knozderko · 2024-06-05T15:13:16Z

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerFactory.cs

+
+namespace Snowflake.Data.Client
+{
+    public class SnowflakeCredentialManagerFactory


this class should not be public since it is not meant to be used by the user. You should move it into another package because in Snowflake.Data.Client should be only classes we want to share with the user.

Understood, the class is no longer public and has been moved

But you moved all the classes from Client to Core - I meant something else...

The Idea is that all classes which could be used by the client should be publicly accessible and located in Client package so the client has all his classes in one place. Classes in Client package start with Snowflake - so it would be good to follow this convention.

In other than Client package we have our internal classes. These should not be public. These classes do not prefix with Snowflake. Sometimes they prefix with SF, but I don't think we should prefix every our internal class with SF, because we do not gain anything from this. So for our internal classes we are free to use any name we want.

The factory class is internal so it is good that you moved it to Core. But for instance ISFCredentialManager is public so it would better fit to Client package. And its previous name ISnowflakeCredentialManager was better fitting our conventions. However the class TokenType which is defined in the same file as ISFCredentialManager is internal so it should be extracted to separate file and placed in Core. Sometimes if we would not like to put too many implementation details into the Client classes we can use a pattern of creating a wrapper class in API. Such a pattern was used here: https://github.com/snowflakedb/snowflake-connector-net/blob/master/Snowflake.Data/Client/SnowflakeDbSessionPool.cs is a public class in Client package which is wrapping the internal class in Core package: https://github.com/snowflakedb/snowflake-connector-net/blob/master/Snowflake.Data/Core/Session/SessionPool.cs
So following this pattern we could have internal implementations in Core.CredentialManager package but then we would need a publicly accessible wrappers in Client package with names starting from Snowflake. Or you can don't create such wrappers but move all public classes to Client package - it is up to you.

My mistake, I thought I was supposed to move the other classes as well

I've reverted some of the changes and did some refactoring:

Rename ISFCredentialManager back to ISnowflakeCredentialManager

Move ISnowflakeCredentialManager back to Client

Extract TokenType from ISnowflakeCredentialManager

Does that look alright to you?

sfc-gh-knozderko · 2024-06-05T15:16:07Z

Snowflake.Data/Client/SnowflakeCredentialManager/SnowflakeCredentialManagerNativeImpl.cs

+
+namespace Snowflake.Data.Client
+{
+    public class SnowflakeCredentialManagerNativeImpl : ISnowflakeCredentialManager


You could move all the implementations into subpackage Infrastructure

sfc-gh-knozderko · 2024-06-05T15:32:12Z

Snowflake.Data/Core/Authenticator/ExternalBrowserAuthenticator.cs

@@ -252,6 +258,8 @@ private SFRestRequest BuildAuthenticatorRestRequest(int port)
                AccountName = session.properties[SFSessionProperty.ACCOUNT],
                Authenticator = AUTH_NAME,
                BrowserModeRedirectPort = port.ToString(),
+                DriverName = SFEnvironment.DriverName,


this improvement is not related to the feature itself?

It's not directly related, but I saw that other connectors like JDBC include the driver name and version in the request so I thought to include it as well in .NET for parity

…nector-net into SNOW-715524-SSO-Token-Cache # Conflicts: # Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs

…nector-net into SNOW-715524-SSO-Token-Cache

…nector-net into SNOW-715524-SSO-Token-Cache # Conflicts: # Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs

…nector-net into SNOW-715524-SSO-Token-Cache # Conflicts: # Snowflake.Data/Core/SFError.cs

sfc-gh-ext-simba-lf and others added 5 commits April 18, 2024 09:54

SNOW-715524: Add SSO token cache

7de3ea6

Add tests

4457077

Remove unused namespace

15a58be

Merge branch 'master' into SNOW-715524-SSO-Token-Cache

632a6b0

Fix unit test

7f28fa8

sfc-gh-ext-simba-lf added 10 commits April 18, 2024 14:09

Refactor constructor and tests

a317158

Refactor test

625e04b

Refactor test and file impl

383fe5e

Refactor file name and fix test

8ce9d10

Merge branch 'master' of https://github.com/snowflakedb/snowflake-con…

a460e4b

…nector-net into SNOW-715524-SSO-Token-Cache

Revert removed lines

e0b65d0

Refactor code and remove unnecessary check

6606823

Add session test

4e6869d

Fix test

3788474

Refactor test and rename file

c597c64

sfc-gh-ext-simba-lf marked this pull request as ready for review April 20, 2024 00:42

sfc-gh-ext-simba-lf requested a review from a team as a code owner April 20, 2024 00:42

sfc-gh-ext-simba-lf requested a review from sfc-gh-knozderko April 20, 2024 00:42

Merge branch 'master' of https://github.com/snowflakedb/snowflake-con…

39b90b2

…nector-net into SNOW-715524-SSO-Token-Cache

sfc-gh-pbulawa requested changes Apr 30, 2024

View reviewed changes

sfc-gh-ext-simba-lf added 4 commits May 1, 2024 13:05

Add more logging

b4ab4ed

Change log from error to info

13ba839

Add file path to logs

b334fb3

Add Meziantou package for credential manager implementation

623ce6a

sfc-gh-ext-simba-lf added 2 commits May 17, 2024 09:50

Add native implementation of credential cache

976bac2

Add impl for ReleaseHandle

feab579

sfc-gh-knozderko requested changes Jun 5, 2024

View reviewed changes

sfc-gh-ext-simba-lf and others added 30 commits July 8, 2024 15:44

Remove catching HttpListenerException

e49ed64

Catch HttpListenerException

c434184

Add check if result is completed and add wait for result

67a8fbf

Add detail for listener exception

f8fcf2b

Specify parameter as AsyncCallback

a46f1ad

Refactor saml request functions

52bfd3e

Increase browser response timeout for test

9edfd79

Remove catching HttpListenerException

f6d8a0f

Remove unnecessary NonParallelizable attribute

77a69b3

Revert remove catching HttpListenerException

9038ba1

Refactor if check

d800e6f

Add temp console lines

58b74d8

Remove console lines

8f919e7

Stop http listener before throwing browser exceptions

fbbac23

Close HttpListener before throwing exception

d53dd8f

Close HttpListener before throwing exception

98df45e

Modify browser timeout test

359cfa3

Abort HttpListener before throwing exception

85d04ed

Check if browser timeout already reached before getting context

2d39171

Use SecureString for tokens

f7be152

Add security checks when reading the credential cache json

2f20edd

Add error logs when reading the json cache file

ac1d659

Merge branch 'master' of https://github.com/snowflakedb/snowflake-con…

8c65f4d

…nector-net into SNOW-715524-SSO-Token-Cache # Conflicts: # Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs

Merge branch 'master' of https://github.com/snowflakedb/snowflake-con…

330fca9

…nector-net into SNOW-715524-SSO-Token-Cache

Merge branch 'master' of https://github.com/snowflakedb/snowflake-con…

fc928fc

…nector-net into SNOW-715524-SSO-Token-Cache

Merge branch 'master' into SNOW-715524-SSO-Token-Cache

d53955a

Merge branch 'master' of https://github.com/snowflakedb/snowflake-con…

d6ff05a

…nector-net into SNOW-715524-SSO-Token-Cache # Conflicts: # Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs

Merge branch 'master' into SNOW-715524-SSO-Token-Cache

a1a6a31

Merge branch 'master' of https://github.com/snowflakedb/snowflake-con…

74963c7

…nector-net into SNOW-715524-SSO-Token-Cache # Conflicts: # Snowflake.Data/Core/SFError.cs

Merge branch 'master' into SNOW-715524-SSO-Token-Cache

126e337

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-715524: Add SSO token cache #921

SNOW-715524: Add SSO token cache #921

sfc-gh-ext-simba-lf commented Apr 18, 2024

codecov bot commented Apr 18, 2024 •

edited

Loading

rafael-sotelo commented May 16, 2024

sfc-gh-ext-simba-lf commented May 16, 2024

sfc-gh-knozderko Jun 5, 2024

sfc-gh-ext-simba-lf Jun 6, 2024

sfc-gh-knozderko Jun 20, 2024

sfc-gh-ext-simba-lf Jul 9, 2024

sfc-gh-knozderko Jun 5, 2024

sfc-gh-ext-simba-lf Jun 6, 2024

sfc-gh-knozderko Jun 12, 2024

sfc-gh-ext-simba-lf Jun 17, 2024

sfc-gh-knozderko Jun 5, 2024

sfc-gh-ext-simba-lf Jun 6, 2024

sfc-gh-knozderko Jun 5, 2024

sfc-gh-ext-simba-lf Jun 6, 2024

SNOW-715524: Add SSO token cache #921

Are you sure you want to change the base?

SNOW-715524: Add SSO token cache #921

Conversation

sfc-gh-ext-simba-lf commented Apr 18, 2024

Description

Checklist

codecov bot commented Apr 18, 2024 • edited Loading

Codecov Report

rafael-sotelo commented May 16, 2024

sfc-gh-ext-simba-lf commented May 16, 2024

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

codecov bot commented Apr 18, 2024 •

edited

Loading