search

sigmavirus24 / github3.py

Hi, I'm a library for interacting with GItHub's REST API in a convenient and ergonomic way. I work on Python 3.6+.
https://github3.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.21k stars 404 forks source link

Flexible generation of JWT tokens #1088

Closed pettermk closed 2 years ago

pettermk commented 2 years ago

This makes it possible to inject a function for signing a JWT token with a private key.

The rationale is that libraries like azure keyvault will take care of storing the private key, but will never actually deliver the private key but rather does the signing of the token as part of the SDK. This is more secure as the app never actually sees the private key then.

Would like input on how to extend the tests for this function

Fixes #1087

sigmavirus24 commented 2 years ago

I dislike the new API greatly. One now has to pass None as the private key to use this. That's confusing and unpleasant. There should be a different way to use this functionality

pettermk commented 2 years ago

I agree that the new API is somewhat clunky, at least ideally it would require a re-ordering of the arguments to allow for not passing None explicitly. But I guess that would consitute a breaking change. Do you think it is better to make a new function signature, or other approaches?

sigmavirus24 commented 2 years ago

So we have a few options that don't introduce the weird ergonomics here:

  1. Add a new method (name unknown) and deprecate login_as_app_installation that has the signature that might make sense where the parameters are re-ordered/kw-only and have that support custom token generators (I'm still not a fan)
  2. Don't change private_key_pem to t.Optional but also have the default function callable (once correctly annotated) be a proxy to jwt.encode
  3. Refactor things such that github3.github.GitHub takes configuration which handles this. I imagine this would be something that defines a sort of interface (abc.ABCMeta) in github3.apps and has at least one concrete implementation - the one that is backwards compatible with what we do today. That could be TokenCreator which has an abstract method of generate which itself accepts app_id and expire_in. Then the concrete class we have that becomes the default is TokenCreatorFromPrivateKey (better names welcome) which does what we hvae today, and one could create a class they use instead of this in GitHub that calls arbitrary functions or specific other APIs. I wouldn't turn my nose at a concrete class that adds this specific functionality. Then, login_as_app_installation would call TokenCreatorFromPrivateKey.generate by default unless some other class instance has been provided.

I'm fairly biased towards the last one as it reduces overall maintenance going forward. No need for us as a library to support every possible configuration someone wants when they can plug and play easily.

pettermk commented 2 years ago

I'm happy to have a crack at option 3, but I don't immediately see how it fixes the problem. login_as_app_installation still needs to accept the pem key to remain API compatible, and we'd still have to pass None in the case of using a custom token generator? I like the idea of using an ABC. I don't fully understand suggestion 2, I feel like if I could understand that then I might understand how 3 fixes the problem. I guess the point is that using a custom token generator means that we don't want to have the pem key to login_as_app_installation, which puts the functionality I'm suggesting fundamentally at odds with the current API.

pettermk commented 2 years ago

Maybe I could simply add login_as_app_installation_with_token instead of getting mired down in this complexity? It would 100% support my use case.

I have a working version of that locally. It is similar to login_as_app_installation except it deduces the expiry time from the token before it sets session.AppBearerTokenAuth Please let me know if you would be more inclined to that solution, I will update the PR

pettermk commented 2 years ago

I ended up using the REST API directly for this one. Thanks for consideration and feedback.