Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update login method as Snowflake changed the OAuth response #46

Merged

Conversation

joshuataylor
Copy link
Contributor

  1. Allow redirects from GET requests
  2. Get cookies from all responses
  3. Workaround for the issue that the API now returns the client_id in the URL, and the apiGET function doesn't return the URL. So we need to return the final redirected URL as the result for
    start-oauth/snowflake.

Tested username+password and SSO.

Fixes #45

1. Allow redirects from GET requests
2. Get cookies from all responses
3. Workaround for the issue that the API now returns the client_id in
the URL, and the apiGET function doesn't return the URL. So we need to
return the final redirected URL as the result for
`start-oauth/snowflake`.

Tested username+password and SSO.
Copy link

@lziegler lziegler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate & in URL looks like a typo, see inline comment.
I don't have the relevant dev tools so can't test these changes myself.
Thanks for the effort!


Tuple<string, List<string>, HttpStatusCode> result = apiGET(
appServerUrl,
String.Format("start-oauth/snowflake?accountUrl={0}&state={1}", HttpUtility.UrlEncode(accountUrl), HttpUtility.UrlEncode(stateParam)),
String.Format("start-oauth/snowflake?accountUrl={0}&&state={1}", HttpUtility.UrlEncode(accountUrl), HttpUtility.UrlEncode(stateParam)),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may want to remove duplicate & from URL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what Snowflake sends back, we should make a note it's not a typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this with some more contextual documentation around &&state - great pickup, as this IS confusing! 😕

When a user logs in via the Snowsight UI at https://app.snowflake.com, Snowflake redirects them to the start-oauth/snowflake endpoint:

https://apps-api.c1.us-east-999.aws.app.snowflake.com/start-oauth/snowflake?accountUrl=https%3A%2F%2Faccount12345us-east-999.snowflakecomputing.com&&state=%7B%22csrf%22%3A%22abcdefab%22%2C%22url%22%3A%22https%3A%2F%2Faccount12345.us-east-999.snowflakecomputing.com%22%2C%22windowId%22%3A%2200000000-0000-0000-0000-000000000000%22%2C%22browserUrl%22%3A%22https%3A%2F%2Fapp.snowflake.com%2F%22%7D

Note that the &&state is not a typo, it is what Snowflake sends, so we send the same.

This string is URL-encoded; its decoded form appears as follows:

https://apps-api.c1.us-east-999.aws.app.snowflake.com/start-oauth/snowflake?accountUrl=https://account12345us-east-999.snowflakecomputing.com&&state={"csrf":"abcdefab","url":"https://account12345.us-east-999.snowflakecomputing.com","windowId":"00000000-0000-0000-0000-000000000000","browserUrl":"https://app.snowflake.com/"}

Snowflake expects the following keys in the state object:

  1. csrf - The csrf token from the earlier step
  2. url - The URL of the user's Snowflake instance (https://account12345.us-east-999.snowflakecomputing.com)
  3. windowId - This parameter is not needed, this parameter is the unique window ID of the user's web browser session, used to mitigate forgery risks.
  4. browserUrl - https://app.snowflake.com

Copy link

@lziegler lziegler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look plausible.
Compiled and tested against my Snowflake account.
This change fixed the problem for me.

@sfc-gh-mybarra
Copy link
Collaborator

sfc-gh-mybarra commented Jan 19, 2024

@lziegler @joshuataylor I can accept this pull request, but I need to ask you both to acknowledge the Snowflake Labs CLA. Please review the documentation at the following link and add your acceptance in a comment. An acceptable response would be _I accept Snowflake Open Source Contribution terms and conditions.

https://github.com/snowflakedb/CLA

@joshuataylor
Copy link
Contributor Author

I, Josh Taylor, accept Snowflake Open Source Contribution terms and conditions, signed Sunday, 21st January 2024 (2024-01-21).

@lziegler
Copy link

I accept Snowflake Open Source Contribution terms and conditions.

@danielodievich
Copy link

I have access to 3 snowflake accounts, 2 behind MFA, and one just username/password.

I seem to be getting authentication issues on the SnowflakeDriver.GetMasterTokenAndSessionTokenFromCredentials with the MFA protected one with the call just timing out. ok scratch that I just didn't see the MFA popup, once I acknowledged it worked just great

The username/password one works fine, although I had to use the actual account name and not the account locator before it let me proceed.

Thank you for fixing this! Much appreciated.

@sfc-gh-mybarra
Copy link
Collaborator

@joshuataylor I compiled your code and it doesn't work for me.
Screenshot 2024-01-23 at 2 16 20 PM

@danielodievich
Copy link

Try hitting ora74440 instead of spfcogs-mybarra_aws_db, I'd be curious if it works.

@danielodievich
Copy link

And are you SURE you are running the code compiled from changes? Your path those 2023.2.8.0 version with binaries?

@sfc-gh-mybarra
Copy link
Collaborator

@danielodievich I initially used the account locator and received the same error. I cloned Joshua's repo and compiled the code without updating the version in the .csproj file. That is why 2023.2.8.0 is showing.
Screenshot 2024-01-23 at 3 46 27 PM

@sfc-gh-mybarra
Copy link
Collaborator

I tried authenticating to another account in Azure and received the same error.

@joshuataylor
Copy link
Contributor Author

I'll sign up for some trial accounts for Azure and Google Cloud, as it seems there might be a different cookie or something needed (they might not have rolled out the feature across all clouds perhaps yet?).

@sfc-gh-mybarra
Copy link
Collaborator

The screenshots that I shared were for an AWS account in the us west 2 region. That is the first region that Snowflake deployed their first accounts. I should have been able to authenticate. I'll keep trying.

@joshuataylor
Copy link
Contributor Author

Could you email me your sanitised logs, the app does verbose logging (doesn't store passwords, but does store cookies, so remove those etc). joshuataylorx at gmail dot com

@danielodievich
Copy link

danielodievich commented Jan 24, 2024

May I suggest that @sfc-gh-mybarra give @joshuataylor an account in his spfcogs-mybarra_aws_db tenant. I recognize is as a a personal test tenant and it should be possible to get a PUBLIC role that can do nothing but login and create worksheets.

@sfc-gh-mybarra
Copy link
Collaborator

I upgraded PowerShell to the latest version and ran the ZipReleases module. I'm am still receiving the same error with the newly generated binaries. I'm not sure what granting access to my account would accomplish. If all users can't compile and run, I can't approve the pull request yet.

@danielodievich
Copy link

I think if Joshua had access to your account, he can check if his logic works in your account and debug it from there.

@sfc-gh-mybarra
Copy link
Collaborator

That could be a solution, but how would my Enterprise Edition account with username/password differ from any other Enterprise Edition account with the same configuration? Debugging my account seems more like a bandaid measure. Additionally, would this be the solution to debug issues in other accounts?

@joshuataylor
Copy link
Contributor Author

I think we need to take a step back and answer "When something goes wrong, how do we figure out what is going on?".

E.g:

  1. A user cannot authenticate (most common problem) using an authentication method (Username & Password, SSO, Private Key, MFA, etc).
  2. A user can authenticate, but gets an unexpected error on one the internal endpoints (worksheets, etc).

We should focus on #1 first, as this will be the most common issue - either due to user error, or Snowflake changing something on their end, adding a new authentication method etc. Authentication and other processes rarely change, especially at SF, but given this is more of an internal endpoint there will be changes.

When Snowflake changes something (rare)

If there is a change that Snowflake has made that breaks authentications we'll all know pretty quickly :-). I have recently setup a scheduled job which logs in using Username+Password and Private Key Auth twice a day to ensure everything is still valid for these endpoints.

Login issues on the client side

I can't login will be such a common and such an ambigious issue, and currently requires a lot of back<>forth to figure out.

I'm going to bet that most of the time it comes down to a user typing in their credentials wrong, HTTP Proxy issues (I bet Snowflake tends to be used a lot on enterprise/work computers :-)) and other transient issues that are hard to track down. We can become better at telling the user about this when it occurs, instead of a generic womp womp something went wrong message (which is fine for now btw).

Login Error Codes

Snowflake has done some great work to improve issues around the login process by adding better error codes for Private Keys, and then I think also MFA, OAuth and SSO. So if it is actually a user issue, then we should be able to give them a (hopefully relevant!) error code.

Diagnosing Issues

What would be good is to have a way for a user to share detailed enough diagnosis information in a format that is both easily understandable by a non-technical user but also be able to figure out what is going on.

This file should be terse enough so the user can understand that they are not going to be sharing any secrets (passwords, cookies, etc), company information, endpoint info, etc.

This way we can tell if there are multiple I can't login issues if they are the same thing or something different.

If the user can't login, we could print the line along the lines of To diagnose this issue, see file logs/login-log-YYYMMDD.log and create an issue <here> to the user.

Short term solution

Let's come up with a short term solution, with the intention of having it another draft PR which can be used to help those who currently can't login with this PR's change.

This is a quick idea dump (least amount of effort, with the intention to not merge it and come up with something better later):

Scenario 1: Someone who just wants to share a quick log:

  1. Have a new logger called LoginDebug (for lack of a better name)
  2. Log to a file with as little information we can for each step in the authentication process. Sanitise account information, login credentials etc. We just want to see what requests were sent (sanitised URL + body of the request) and what the response URL+relevant headers+sanitised body was. Each endpoint would have custom logging to only document as little as possible about the response (eg the code, if something was returned).
  3. User provides this file output in the issue.

Scenario 2: More technical user who wants to provide a debug log

Sometimes we're going to need more information, so having a way to get the sanitised request and response URL/Headers/Body will be good. More on this to come.

@joshuataylor
Copy link
Contributor Author

Just to be clear, I'll create a follow up PR in the next day or so, it's a long weekend here and I finally have a chance to finish this.

@sfc-gh-mybarra
Copy link
Collaborator

That sounds great @joshuataylor. I appreciate you stepping in to help outside of your regular day to day. I'll look for the upcoming PR.

@joshuataylor
Copy link
Contributor Author

Okay, so it looks like with release 8.4 there were some major changes made around authentication, which explains why we saw the authentication workflow break, and perhaps work for some but not others during the rollout.

I'm going to guess that those who couldn't log in were either:

a/ Still on the old version
b/ On the new version

I know Snowflake does gradual rollouts, and some users can also request to be on the new version etc. This should hopefully be a change that isn't made often, but should be something we can hopefully adapt to if needed.

I'm seeing a new authentication flow for SSO now on both my AWS US instance (8.4.1, us-east-1) and my AWS Sydney (ap-southeast-2) instances. Could I get those that couldn't login to confirm their version & region?

select concat('Snowflake Version: ', coalesce(current_version(), '-'), ' | Region: ', coalesce(current_region(), '-'));

I'll get the code updated for this change, username/password seems to work fine still.

I also have a new branch I'll push out with what I'm calling Diagnostic Test, which should help diagnose these sorts of issues.

The log file output looks like this:

This file contains a Diagnostic Test, with sanitised information for API Endpoints accessed by SFSnowsightExtensions.
This should not contain any sensitive information about your account, but please double check before posting.
Please also include the output of the following SQL command, which will output the Snowflake version and Region, as this allows us to see if you're running on a newer or older version of Snowflake, and the region.
This should output a string similar to "Snowflake Version: 8.4.1 | Region: AWS_US_EAST_1".

"""
select concat('Snowflake Version: ', coalesce(current_version(), '-'), ' | Region: ', coalesce(current_region(), '-'));
"""

SFSnowsightExtensions Diagnostic Test - v2024.1.0.0 - Date: 2024-02-05
Environment: Darwin 23.3.0 Darwin Kernel Version 23.3.0: Wed Dec 20 21:31:00 PST 2023; root:xnu-10002.81.5~7/RELEASE_ARM64_T6020 - Arm64 | Dotnet Version: .NET 8.0.1 | PowerShell Version: 7.4.1 | HTTP_PROXY set: True | HTTPS_PROXY set: True
Passed Parameters: Account: True | UserName: True | Password: True | Credential: False | SSO: False | MainAppURL: default (https://app.snowflake.com)
--------------------
GET /v0/validate-snowflake-url
Response: 200 (OK)
Response Body: valid: VALID | account: EXISTS | appServerUrl: EXISTS | region: EXISTS | url: EXISTS
Request Cookies: N/A
Response Cookies: snowflake_deployment: EXISTS
--------------------
GET /start-oauth/snowflake
Response: 200 (OK)
Redirected to oauth/authorize: True
Request Cookies: N/A
Response Cookies: S8_SESSION: EXISTS | oauth-nonce-XXX: EXISTS
--------------------
POST /session/v1/login-request
Response: 200 (OK)
Response Body: success: TRUE | code: N/A | serverVersion: EXISTS | masterToken: EXISTS | token: EXISTS | sessionId: EXISTS | displayUserName: EXISTS | schemaName: EXISTS | warehouseName: EXISTS | roleName: EXISTS | databaseName: EXISTS
Request Cookies: N/A
Response Cookies: N/A
--------------------
POST /session/authenticate-request
Response: 200 (OK)
Response Body: success: TRUE | code: N/A | masterToken: EXISTS
Request Cookies: N/A
Response Cookies: N/A
--------------------
POST /oauth/authorization-request
Response: 200 (OK)
Response Body: success: FALSE | code: EXISTS | redirectUrl: EXISTS | nextAction: EXISTS | inFlightCtx: EXISTS
Request Cookies: N/A
Response Cookies: N/A
--------------------
GET /complete-oauth/snowflake
Response: 200 (OK)
Response Body: text/html, params var: True
Request Cookies: oauth-nonce-XXX: EXISTS
Response Cookies: user-XXX: EXISTS | oauth-nonce-XXX: EXISTS
--------------------
GET /bootstrap
Response: 200 (OK)
Response Body: BuildVersion: EXISTS | User.id: EXISTS
Request Cookies: user-XXX: EXISTS
Response Cookies: csrf-XXX: EXISTS | user-XXX: EXISTS
--------------------
POST /v0/organizations/XXX/entities/list
Response: 200 (OK)
Response Body: defaultOrgId: EXISTS
Request Cookies: user-XXX: EXISTS
Response Cookies: N/A

There is also an "Extensive Diagnostic Test" which includes Unknown Cookie Values from the request/response, as well as additional information from the response such as additional keys.

@joshuataylor
Copy link
Contributor Author

Super annoying, I had to readd my security integration to get SSO to work with both this and the Snowflake Connector Python. I do not have multiple security integrations.

If you have the same issue, and can reproduce it with Snowflake Connector Python, submit a support ticket. 🤷

Now 8.4 has been out for a week, I believe all accounts should be on the newest release and everything should hopefully work now. Double check with select concat('Snowflake Version: ', coalesce(current_version(), '-'), ' | Region: ', coalesce(current_region(), '-'));.

If you still have issues, please try this PR and post the DiagnosticTest log file, as it will help figure out what's going on.

@sfc-gh-sredding
Copy link
Collaborator

I was testing your fix branch and I did get errors when trying to use the "regionless" url format ( org_name-account_name ) as the -Account . It did look like it was able to figure out the URL when using this format but I kept getting

ConnectAppCommand threw Unable to authenticate user sredding@sfpscogs-redding_p because of Bad request; operation not supported. (390400) (SnowflakePS)

when I switched to just using the account locator URL format it worked just fine. I was also able to use SSO login to an account as well.

@joshuataylor
Copy link
Contributor Author

joshuataylor commented Feb 12, 2024

Interesting!

Can you login normally using that format on app.snowflake.com? Is this a new login URL format?

Does it pass valid on https://app.snowflake.com/v0/validate-snowflake-url?url=XXX&isSecondaryAccount=false?

edit:

Ah https://docs.snowflake.com/en/user-guide/organizations-connect explains it!

The legacy account locator format is currently supported, but its use is discouraged.

So testing for this would be good. Looks like clicking "copy" here adds a . instead of a -, that's slightly confusing!

SCR-20240212-topn-2

IMHO, using the values from here is what you would want to do in future.

And this would be interesting to test with Snowsight

@joshuataylor
Copy link
Contributor Author

This might also be related to https://stackoverflow.com/questions/60177963/how-can-you-connect-snowflake-to-an-ide-using-okta-with-mfa , could I ask what SSO provider you're testing with, so far I've been testing with Auth0 and it's been pretty flawless, but this is a pretty simple implementation and others might have more complex setups.

I tested using <ORG_NAME>-<ACCOUNT_IDENTIFIER> for AWS & Azure, and they both seemed to work. The url and other information is gathered from the validate-url endpoint.

SFSnowsightExtensions Diagnostic Test - v2024.1.0.0 - Date: 2024-02-13
Environment: Darwin 23.3.0 Darwin Kernel Version 23.3.0: Wed Dec 20 21:31:00 PST 2023; root:xnu-10002.81.5~7/RELEASE_ARM64_T6020 - Arm64 | Dotnet Version: .NET 8.0.1 | PowerShell Version: 7.4.1 | HTTP_PROXY set: True | HTTPS_PROXY set: True
Passed Parameters: Account: True | UserName: True | Password: False | Credential: False | SSO: True | MainAppURL: default (https://app.snowflake.com)
--------------------
GET /v0/validate-snowflake-url
Response: 200 (OK) in 2705ms
Response Body: valid: VALID | account: EXISTS | appServerUrl: EXISTS | region: EXISTS | url: EXISTS
Request Cookies: N/A
Response Cookies: snowflake_deployment: EXISTS
--------------------
GET /start-oauth/snowflake
Response: 200 (OK) in 657ms
Redirected to oauth/authorize: True
Request Cookies: N/A
Response Cookies: S8_SESSION: EXISTS | oauth-nonce-XXX: EXISTS
--------------------
POST /session/authenticator-request
Response: 200 (OK) in 232ms
Response Body: success: TRUE | code: N/A | message: N/A | tokenUrl: N/A | ssoUrl: EXISTS | proofKey: EXISTS
Request Cookies: N/A
Response Cookies: N/A
--------------------
POST /session/v1/login-request
Response: 200 (OK) in 413ms
Response Body: success: TRUE | code: N/A | serverVersion: EXISTS | masterToken: EXISTS | token: EXISTS | sessionId: EXISTS | displayUserName: EXISTS | schemaName: N/A | warehouseName: EXISTS | roleName: EXISTS | databaseName: N/A
Request Cookies: N/A
Response Cookies: N/A
--------------------
POST /oauth/authorization-request
Response: 200 (OK) in 248ms
Response Body: success: FALSE | code: EXISTS | redirectUrl: EXISTS | nextAction: EXISTS | inFlightCtx: EXISTS
Request Cookies: N/A
Response Cookies: N/A
--------------------
GET /complete-oauth/snowflake
Response: 200 (OK) in 906ms
Response Body: text/html, params var: True
Request Cookies: oauth-nonce-XXX: EXISTS
Response Cookies: user-XXX: EXISTS | oauth-nonce-XXX: EXISTS
--------------------
GET /bootstrap
Response: 200 (OK) in 311ms
Response Body: BuildVersion: EXISTS | User.id: EXISTS
Request Cookies: user-XXX: EXISTS
Response Cookies: csrf-XXX: EXISTS | user-XXX: EXISTS
--------------------
POST /v0/organizations/XXX/entities/list
Response: 200 (OK) in 253ms
Response Body: defaultOrgId: EXISTS
Request Cookies: user-XXX: EXISTS
Response Cookies: N/A

@sfc-gh-sredding
Copy link
Collaborator

The SSO I used is OKTA .

Copy link
Collaborator

@sfc-gh-sredding sfc-gh-sredding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to use the hotfix branch and create the binaries and use them to successfully authenticate using id/pass and SSO (Okta) .

@sfc-gh-mybarra
Copy link
Collaborator

Ok @joshuataylor. @sfc-gh-sredding helped me get past the issues that I was having, and will merge your pull request. Thank you for your hard work.

@sfc-gh-mybarra sfc-gh-mybarra merged commit da34e2d into sfc-gh-klo:main Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connect-SFApp - Unable to get account client ID for account
5 participants