Skip to content

SNOW-1825621 OAuth code flow PKCE support #2137

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.

Sign up for GitHub

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

Jump to bottom
Merged
sfc-gh-mmishchenko merged 81 commits into from
Mar 4, 2025
Merged

SNOW-1825621 OAuth code flow PKCE support #2137

sfc-gh-mmishchenko merged 81 commits into from
Mar 4, 2025

Conversation

@sfc-gh-mkeller
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller commented Jan 16, 2025 •
edited
Loading

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1825621

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency

  3. Please describe how your code solves the related issue.

This PR builds on top of #2135 and it adds PKCE support on top of OAuth code flow.
This change has been tested manually, as it's fairly complicated to setup and we don't do unit tests for the different authentication methods.

  1. (Optional) PR for stored-proc connector:

@sfc-gh-mkeller sfc-gh-mkeller changed the title SNOW-1825621 OAuth PKCE support SNOW-1825621 OAuth code flow PKCE support Jan 16, 2025
@sfc-gh-mkeller sfc-gh-mkeller force-pushed the mkeller/SNOW-1825621/pkce-support branch from dc687f4 to 3684460 Compare January 16, 2025 19:24
@sfc-gh-mkeller sfc-gh-mkeller self-assigned this Jan 16, 2025
sfc-gh-eworoshow
sfc-gh-eworoshow reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@sfc-gh-eworoshow sfc-gh-eworoshow left a comment

Choose a reason for hiding this comment

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

I tried to take a quick look at only the PKCE changes...

src/snowflake/connector/auth/oauth_code.py Outdated
hashlib.sha256(self._verifier.encode("utf-8")).digest()
)
.decode("utf-8")
.replace("=", "")
Copy link
Contributor

@sfc-gh-eworoshow sfc-gh-eworoshow Jan 24, 2025

Choose a reason for hiding this comment

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

if it's URL-safe it shouldn't have any padding?

Copy link
Collaborator Author

@sfc-gh-mkeller sfc-gh-mkeller Jan 30, 2025

Choose a reason for hiding this comment

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

from https://docs.python.org/3/library/base64.html#base64.urlsafe_b64encode

The result can still contain =.

Unfortunately this remains necessary

@sfc-gh-mkeller sfc-gh-mkeller force-pushed the mkeller/SNOW-1825621/pkce-support branch from 4591ea1 to 6ae67ca Compare January 30, 2025 18:27
@sfc-gh-mkeller sfc-gh-mkeller force-pushed the mkeller/SNOW-1825621/pkce-support branch from 6ae67ca to 81627af Compare January 30, 2025 18:31
sfc-gh-eworoshow
sfc-gh-eworoshow reviewed Jan 30, 2025
View reviewed changes
Copy link
Contributor

@sfc-gh-eworoshow sfc-gh-eworoshow left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me but I'd prefer to stamp after we see a "clean" diff with the preceding work merged first.

@sfc-gh-mkeller sfc-gh-mkeller mentioned this pull request Jan 30, 2025
Closed
7 tasks
@sfc-gh-mmishchenko sfc-gh-mmishchenko force-pushed the mkeller/SNOW-1825621/oauth-code-flow-support branch from a850492 to 3fbb1ed Compare February 19, 2025 10:48
sfc-gh-mmishchenko and others added 21 commits February 19, 2025 15:19
@sfc-gh-mmishchenko
Merge branch 'mkeller/SNOW-1825621/oauth-code-flow-support' into mkel…
8bbbc55 
…ler/SNOW-1825621/pkce-support
@sfc-gh-mkeller @sfc-gh-mmishchenko
adding new AuthHttpServer class
6e76857
@sfc-gh-mkeller @sfc-gh-mmishchenko
unrelated simplifications
c6c17d1
@sfc-gh-mkeller @sfc-gh-mmishchenko
adding new oauth code flow implementation
550e54c
@sfc-gh-mkeller @sfc-gh-mmishchenko
adding new TODO
f9ffa01
@sfc-gh-mkeller @sfc-gh-mmishchenko
adding changelog entry
ff44946
@sfc-gh-mkeller @sfc-gh-mmishchenko
making oauth state random
a1d9b54
@sfc-gh-mkeller @sfc-gh-mmishchenko
make client_secret optional
fec1dca
@sfc-gh-mkeller @sfc-gh-mmishchenko
make state check failure fail auth
ff041a2
@sfc-gh-mkeller @sfc-gh-mmishchenko
switch to cryptographically-secure source
3e60038
@sfc-gh-mkeller @sfc-gh-mmishchenko
redo connection parameters based on feedback
a85abac
@sfc-gh-mkeller @sfc-gh-mmishchenko
some string updates
e4549ff
@sfc-gh-mkeller @sfc-gh-mmishchenko
add Authorization header to token request
1305df7
@sfc-gh-mkeller @sfc-gh-mmishchenko
fix logging bug
fc6a403
@sfc-gh-mkeller @sfc-gh-mmishchenko
review feedback
638f741
@sfc-gh-mkeller @sfc-gh-mmishchenko
rewording parameters based on discussion
b94eeb7
@sfc-gh-mkeller @sfc-gh-mmishchenko
reworking the defaut oauth scope
e7e664e
@sfc-gh-mkeller @sfc-gh-mmishchenko
adding port support into oauth code flow auth
725b3d0
@sfc-gh-mkeller @sfc-gh-mmishchenko
do not log state
6b9b48c
@sfc-gh-mkeller @sfc-gh-mhofman @sfc-gh-mmishchenko
Update src/snowflake/connector/connection.py
acfe7e7 
Co-authored-by: Michał Hofman <[email protected]>
@sfc-gh-pbulawa @sfc-gh-mkeller
@sfc-gh-mhofman @sfc-gh-mmishchenko
SNOW-1917265: Add OAUTH_TYPE for authorization flow (#2174)
6bad2e1 
Co-authored-by: Mark Keller <[email protected]>
Co-authored-by: Michał Hofman <[email protected]>
@sfc-gh-mmishchenko sfc-gh-mmishchenko force-pushed the mkeller/SNOW-1825621/oauth-code-flow-support branch from 8210acf to 6bad2e1 Compare February 25, 2025 13:42
@sfc-gh-mmishchenko sfc-gh-mmishchenko force-pushed the mkeller/SNOW-1825621/oauth-code-flow-support branch from 6bad2e1 to b85a824 Compare March 4, 2025 14:17
@sfc-gh-mmishchenko sfc-gh-mmishchenko merged commit 493efad into mkeller/SNOW-1825621/oauth-code-flow-support Mar 4, 2025
2 of 4 checks passed
@sfc-gh-mmishchenko sfc-gh-mmishchenko deleted the mkeller/SNOW-1825621/pkce-support branch March 4, 2025 15:04
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sfc-gh-eworoshow sfc-gh-eworoshow sfc-gh-eworoshow left review comments

@sfc-gh-dyadav sfc-gh-dyadav Awaiting requested review from sfc-gh-dyadav

Assignees

@sfc-gh-mmishchenko sfc-gh-mmishchenko

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sfc-gh-mkeller @sfc-gh-eworoshow @sfc-gh-mmishchenko @sfc-gh-pbulawa