WIP - OIDC keycloak implementation alpha cut#93

Open

ntai-arxiv wants to merge 46 commits intodevelopfrom

ntai/keycloak-take-2

ntai-arxiv commented Oct 11, 2024

No description provided.

ntai-arxiv added 30 commits

August 27, 2024 10:34


          keycloak auth implementation - fresh branch

466c85b


          Log out is getting better.

65e5547


          Add refresh endpoint.

70c408b


          Factor out the envvars.

9471af6


          Add CORS from env var.

45d125c


          Fix up the logging, and add more logging. Update arxiv-base

7bef9da


          switching to use localhost.arxiv.org

f28411e


          Fix up some nits

a60dc90


          Get the real IP from the header. Fix up the tapir cookie.

5da05a4


          add extra logging cookie

3adff0c


          Added DOMAIN, make sure classic cookie can be had.

bf4437b

I believe the missing ingredients was the CLASSIC_SESSION_HASH and thus the cookie between this and other services did not agree.


          refactor to cookie handling - nothing is working atm :(

7e000ee


          Switch (back) to use localhost. (works!) Update arxiv-base

862d2fd

Safe-guard not having jwt secret (it's a disaster without it.)


          nit pick of Dockerfile

a775e3f


          Finally things working on dev3

fd41175


          Still working on token refresh.

c70aa96


          Still working on token refresh.

a78d7dd


          Logout with POST. Refresh token with POST. I think it can refresh the…

39c1970

… tapir cookie with refresh too.


          DB access works again.

52678b0


          DB access works again.

093ad6a


          Fix the Dockerfile (it doesn't build without readme. I think I know a…

d4d0466

… switch to turn this off.)

Username needs casefolging.

Add "login" to Makefile.


          Deal with tapir session creation error.

2b97c40

Add logout-callback endpoint. It's NOOP and until we need it, we may set it to none on keycloak. It is "Backchannel logout URL"


          Implement token refresh for real.

d8f88dc


          For refresh, redirect by default seems harmful.

311bebd


          Fix a bug

3663fe2


          /refresh's next - None does not work appaently.

476f479

Ues the updated arxiv-base.


          Change to use refresh token instead of user claims for token refresh.

cd56834


          Change to use refresh token instead of user claims for token refresh.

960de88


          Update arxiv-base ref to use the "no access token"

45d3aa5


          Refine the refresh token response type.

3686db9

legacy_auth_provider.py to include the tracking cookie.

ntai-arxiv added 9 commits

September 27, 2024 15:48


          Fill in the email array with existing email. (This should not matter …

c2e7995

…but hey.)


          Make cookie to stick very long.

187ac4a

Make the tapir cookie optional for refresh. Some day, we may ditch it.


          Introduce cookie max age!

7aeab89


          sigh

ee43eda


          sigh

38b3555


          1. Keycloak sends out the audit events to GCP pubsub.

570f9d8

2. Add kc-to-tapir bridge prototype code


          Update the event draining

4f1bed0


          arxiv-base update


          Implement the connection retry for oauth2 authenticator.

49138f4

github-advanced-security bot found potential problems

View reviewed changes

legacy_auth_provider/tools/mass_reset_password.py

+                      # Update the password for each fetched user_id
+                      email: str
+                      for user_id, email in user_ids:
+                          if "cornell.edu" in email or "arxiv.org" in email:

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization

The string [cornell.edu](1) may be at an arbitrary position in the sanitized URL.

Copilot Autofix

AI over 1 year ago

To fix the problem, we need to ensure that the email addresses are checked more robustly. Instead of using a substring match, we should parse the email addresses and check the domain part explicitly. This can be done using the email.utils module to parse the email address and then checking the domain part.

Parse the email address to extract the domain part.
Check if the domain part matches "cornell.edu" or "arxiv.org".
Update the code in the hack_creds function to implement this change.

Suggested changeset 1

legacy_auth_provider/tools/mass_reset_password.py

@@ -42,3 +42,4 @@
                     for user_id, email in user_ids:
-                        if "cornell.edu" in email or "arxiv.org" in email:
+                        domain = email.split('@')[-1]
+                        if domain == "cornell.edu" or domain == "arxiv.org":
                             continue

Copilot is powered by AI and may make mistakes. Always verify output.

legacy_auth_provider/tools/mass_reset_password.py

+                      # Update the password for each fetched user_id
+                      email: str
+                      for user_id, email in user_ids:
+                          if "cornell.edu" in email or "arxiv.org" in email:

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization

The string [arxiv.org](1) may be at an arbitrary position in the sanitized URL.

Copilot Autofix

AI over 1 year ago

To fix the problem, we need to ensure that the email addresses are properly validated to belong to the allowed domains (cornell.edu and arxiv.org). Instead of using a simple substring check, we should parse the email addresses and verify the domain part explicitly.

We will use the email.utils module to parse the email addresses and extract the domain part.
We will then check if the domain part matches the allowed domains (cornell.edu and arxiv.org).

Suggested changeset 1

legacy_auth_provider/tools/mass_reset_password.py

@@ -42,3 +42,5 @@
                     for user_id, email in user_ids:
-                        if "cornell.edu" in email or "arxiv.org" in email:
+                        from email.utils import parseaddr
+                        domain = parseaddr(email)[1].split('@')[-1]
+                        if domain == "cornell.edu" or domain == "arxiv.org":
                             continue

Copilot is powered by AI and may make mistakes. Always verify output.

legacy_auth_provider/tools/mass_reset_password.py

+                          if "cornell.edu" in email or "arxiv.org" in email:
+                              continue
+                          new_password = generate_random_password()
+                          print(f"{email},{new_password}", file=creds_file)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

This expression logs [sensitive data (password)](1) as clear text.

Copilot Autofix

AI over 1 year ago

To fix the problem, we should avoid logging sensitive information such as passwords in clear text. Instead, we can log a placeholder or mask the sensitive data. In this case, we can log the email and a masked version of the password (e.g., only showing the first and last character of the password with the rest replaced by asterisks).

Modify the code to mask the password before logging it.
Ensure that the functionality of the code remains unchanged, i.e., the passwords are still updated in the database correctly.

Suggested changeset 1

legacy_auth_provider/tools/mass_reset_password.py

@@ -45,3 +45,4 @@
                         new_password = generate_random_password()
-                        print(f"{email},{new_password}", file=creds_file)
+                        masked_password = new_password[0] + '*' * (len(new_password) - 2) + new_password[-1]
+                        print(f"{email},{masked_password}", file=creds_file)
                         cursor.execute(

Copilot is powered by AI and may make mistakes. Always verify output.

oauth2-authenticator/arxiv_oauth2/authentication.py

Comment on lines +257 to +258

		response.set_cookie(session_cookie_key, token, max_age=cookie_max_age,
		domain=domain, path="/", secure=secure, samesite=samesite)

Check warning

Code scanning / CodeQL

Failure to use secure cookies

Cookie is added without the HttpOnly attribute properly set.

oauth2-authenticator/arxiv_oauth2/authentication.py

Comment on lines +260 to +261

		response.set_cookie(session_cookie_key, "", max_age=0,
		domain=domain, path="/", secure=secure, samesite=samesite)

Check warning

Code scanning / CodeQL

Failure to use secure cookies

Cookie is added without the HttpOnly attribute properly set.

oauth2-authenticator/arxiv_oauth2/authentication.py

Comment on lines +265 to +266

		response.set_cookie(classic_cookie_key, tapir_cookie, max_age=cookie_max_age,
		domain=domain, path="/", secure=secure, samesite=samesite)

Check warning

Code scanning / CodeQL

Failure to use secure cookies

Cookie is added without the HttpOnly attribute properly set.

oauth2-authenticator/arxiv_oauth2/authentication.py

Comment on lines +269 to +270

		response.set_cookie(classic_cookie_key, '', max_age=0,
		domain=domain, path="/", secure=secure, samesite=samesite)

Check warning

Code scanning / CodeQL

Failure to use secure cookies

Cookie is added without the HttpOnly attribute properly set.

ntai-arxiv added 5 commits

October 12, 2024 18:05


          Fix up github hook tests

755e14f


          the local tests working

bad9ce8


          Fix the mysql retry middleware.

845e93f


          Let email to be purely email address

2ce2ca9


          Improve the refresh.

f8d4f46

github-advanced-security bot found potential problems

View reviewed changes

oauth2-authenticator/arxiv_oauth2/authentication.py

+                  if tapir_cookie:
+                      logger.debug('%s=%s',classic_cookie_key, tapir_cookie)
+                      response.set_cookie(classic_cookie_key, tapir_cookie, max_age=cookie_max_age,

Check warning

Code scanning / CodeQL

Construction of a cookie using user-supplied input

Cookie is constructed from a [user-supplied input](1).

ntai-arxiv added 2 commits

December 18, 2024 13:18


          Allow the token refresh with just the modern cookie.

a680dba

Include the access token in the URL so the web app can see it.


          Update arxiv-base tag

fd3f8e6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet