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

Expand access for other jwt algorithms #3378

Merged
merged 2 commits into from
May 10, 2024
Merged

Conversation

duckboy81
Copy link
Contributor

@duckboy81 duckboy81 commented May 1, 2024

Changes: Commented out the algorithm requirement in the jwt decoding function

More:
RS256 is unintentionally hardcoded into the jwt decoding function. Keycloak allows for many other signing algorithms and the Financial-grade API even suggests to use PS256 instead.

Copy link

vercel bot commented May 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2024 8:09pm

@krrishdholakia
Copy link
Contributor

can you confirm this change passes testing -

import sys, os, asyncio, time, random

@duckboy81

@duckboy81
Copy link
Contributor Author

@krrishdholakia I could probably do that.

Is there a doc on setting up the test environment for this repo? (I'll admit, I don't typically dev in python.)

@krrishdholakia
Copy link
Contributor

  1. Clone the repo
git clone https://github.com/BerriAI/litellm.git
  1. Navigate into the project, and install dependencies:
cd litellm
poetry install
  1. Test your change:
cd litellm/tests 
pytest test_jwt.py -x -vv

@krrishdholakia
Copy link
Contributor

hey @duckboy81 bump on this?

@duckboy81
Copy link
Contributor Author

Sorry, tried your instructions in a codespaces VM and it didn't like. Haven't had the time at home, maybe tomorrow 🤞🏼

@duckboy81
Copy link
Contributor Author

@krrishdholakia

Curious, does the test pass for you in the repo's current state? I cloned and I'm getting a couple of errors:

====================================== test session starts =======================================
platform linux -- Python 3.10.12, pytest-8.2.0, pluggy-1.5.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: [removed]/litellm
configfile: pyproject.toml
plugins: anyio-4.2.0
collected 5 items

test_jwt.py::test_load_config_with_custom_role_names PASSED                                [ 20%]
test_jwt.py::test_team_token_output ERROR                                                  [ 40%]
test_jwt.py::test_token_single_public_key SKIPPED (async def function and no async plugin
installed (see warnings))                                                                  [ 60%]
test_jwt.py::test_user_token_output ERROR                                                  [ 80%]
test_jwt.py::test_valid_invalid_token SKIPPED (async def function and no async plugin
installed (see warnings))                                                                  [100%]


==================================== short test summary info =====================================
ERROR test_jwt.py::test_team_token_output - TypeError: Cannot mix str and non-str arguments
ERROR test_jwt.py::test_user_token_output - TypeError: Cannot mix str and non-str arguments
====================== 1 passed, 2 skipped, 41 warnings, 2 errors in 3.34s =======================

@duckboy81
Copy link
Contributor Author

@krrishdholakia Alright, getting that all setup was a doozy. Turns out I was wrong, so I've instead expanded the existing array of options to include the other supported RS and PS algos. See Digital Signature Algorithms.

See latest commit in PR. Should be ready to approve.

pass

I'm not smart enough on the other algos to give those greenlights, so my omission should not be misconstrued to mean they shouldn't be used.

PS: I'm not sure the last time you've setup the dev env from scratch, but I think it might've changed a bit. For future readers who are stuck trying to set everything up, here's what I did. (Essentially copied circleci's pipeline)

Setting up dev test environment (configured on an Ubuntu VM)

Note: You need to run your own postgres db. You can grab it from docker.

git clone https://github.com/BerriAI/litellm.git
cd litellm

sudo apt-install python3-pip

python -m venv ~/venv
source ~/venv/bin/activate

pip install -r .circleci/requirements.txt

# Who knows if all of these are necessary.
pip install "pytest==7.3.1"
pip install "pytest-asyncio==0.21.1"
pip install mypy
pip install "google-generativeai==0.3.2"
pip install "google-cloud-aiplatform==1.43.0"
pip install pyarrow
pip install "boto3==1.34.34"
pip install "aioboto3==12.3.0"
pip install langchain
pip install lunary==0.2.5
pip install "langfuse==2.27.1"
pip install numpydoc
pip install traceloop-sdk==0.0.69
pip install openai
pip install prisma            
pip install "httpx==0.24.1"
pip install fastapi
pip install "gunicorn==21.2.0"
pip install "anyio==3.7.1"
pip install "aiodynamo==23.10.1"
pip install "asyncio==3.4.3"
pip install "apscheduler==3.10.4"
pip install "PyGithub==1.59.1"
pip install argon2-cffi
pip install "pytest-mock==3.12.0"
pip install python-multipart
pip install google-cloud-aiplatform
pip install prometheus-client==0.20.0

export DATABASE_URL=postgresql://<USERNAME>:<PASSWORD>@<HOST>:<PORT>/<DATABASE_NAME>
export JWT_PUBLIC_KEY_URL=this_doesnt_matter # ...because the script makes its own pub/priv keys on the fly

prisma db push

python -m pytest -vv litellm/tests/test_jwt.py

# Or to run all tests (command not tested myself)
# python -m pytest -vv litellm/tests/ -x --junitxml=test-results/junit.xml --durations=5 

@duckboy81 duckboy81 changed the title Removed jwt algo restriction Expand access for other jwt algorithms May 7, 2024
@duckboy81 duckboy81 closed this May 7, 2024
@duckboy81
Copy link
Contributor Author

duckboy81 commented May 7, 2024

@krrishdholakia Ready for approval (resolved merge issue). Tests pass still.

@krrishdholakia krrishdholakia merged commit 8a35354 into BerriAI:main May 10, 2024
2 checks passed
@duckboy81 duckboy81 deleted the patch-1 branch May 15, 2024 20:04
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.

None yet

2 participants