feat(api): long lived api keys #658
Conversation
gphorvath
added this to the v0.9.0 - RAG Improvements | Model Directory | Odds and Ends milestone
✅ Deploy Preview for leapfrogai-docs canceled.
|
… has been consolidated into supabase
…y to read whole table
…leapfrogai into 561-long-lived-api-keys
…exposed beyond just debugging purposes now that we have API keys
|
|
||
| def _validate_checksum(unique_key: str, checksum: str): | ||
| """Validate the checksum of an API key.""" | ||
| return hashlib.sha256(unique_key.encode()).hexdigest()[:CHECKSUM_LENGTH] == checksum |
There was a problem hiding this comment.
I recommend changing this to secrets.compare_digest(calculated_checksum, checksum)
https://pycharm-security.readthedocs.io/en/latest/fixes/comparedigestfixer.html
| """ | ||
| Validate and encode an API key. | ||
|
|
||
| Should be in the form: `{prefix}_{unique_key}_{checksum}` |
There was a problem hiding this comment.
We should also add some sort of versioning prefix here to add backwards compatibility in case we change the API key algorithm, etc.
Example:
read_once_token = f"{KEY_PREFIX}_{VERSION}.{unique_key}.{checksum}"There was a problem hiding this comment.
If we do this on the database side (see pgcrypto comment) do you think the created_at date in the table is sufficient for this?
There was a problem hiding this comment.
Yeah, that would definitely work! In the future, we can just use the created_on date to determine key version in case we need to be backwards compatible.
| returns: | ||
| api_key: str # in the format {prefix}_{one_way_hash}_{checksum} | ||
| """ | ||
| one_way_hash = hashlib.sha256(unique_key.encode()).hexdigest() |
There was a problem hiding this comment.
We should probably use a different method here, like bcrypt or pbkdf2_hmac. Both are more secure, if not a little more computationally intensive. For example, 1Password uses a version of pbkdf2_hmac, I believe.
https://security.stackexchange.com/questions/211/how-to-securely-hash-passwords
Example:
key = hashlib.pbkdf2_hmac('sha256', unique_key.encode(), salt.encode(), 100000, dklen=64)
one_way_hash = key.hex()
There was a problem hiding this comment.
If this is for security purposes, I would also toss pgcrypto in the running there as well.
Though if its not for security purposes then I think its unnecessary to change the algorithm any encoding will do if it gets the job done.
There was a problem hiding this comment.
Yeah, it was mainly for security best practice. It just depends on how secure we want these API keys to be!
You are right though - if we are only looking for functionality, everything here works.
There was a problem hiding this comment.
Yeah, definitely need security here. I'll look at these options
There was a problem hiding this comment.
Think I'm going with pgcrypto here. The main advantage is server side hashing. I will update when I finish the commit.
| def test_create_api_key(create_api_key): | ||
| """Test creating an API key. Requires a running Supabase instance.""" | ||
| assert create_api_key.status_code is status.HTTP_200_OK | ||
| assert "api_key" in create_api_key.json(), "Create should return an API key." |
There was a problem hiding this comment.
May be worthwhile to also test if the API key is in the correct format (prefixes).
| @@ -26,6 +26,7 @@ | |||
| from leapfrogai_api.routers.supabase_session import Session | |||
|
|
|||
| router = APIRouter(prefix="/openai/v1/vector_stores", tags=["openai/vector_stores"]) | |||
| security = HTTPBearer() | |||
There was a problem hiding this comment.
Perhaps use constants for the magic numbers in this file or add more explanations in the comments are to why these (32, 4) numbers were chosen.
| returns: | ||
| api_key: str # in the format {prefix}_{one_way_hash}_{checksum} | ||
| """ | ||
| one_way_hash = hashlib.sha256(unique_key.encode()).hexdigest() |
There was a problem hiding this comment.
If this is for security purposes, I would also toss pgcrypto in the running there as well.
Though if its not for security purposes then I think its unnecessary to change the algorithm any encoding will do if it gets the job done.
Addresses #561