-
Notifications
You must be signed in to change notification settings - Fork 479
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
JWT Detector Plugin #239
JWT Detector Plugin #239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! 🎈 ⚡️
Just one regex change I think
|
||
@staticmethod | ||
def is_formally_valid(token): | ||
parts = token.split('.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting for posterity the spec at https://tools.ietf.org/id/draft-jones-json-web-token-04.html#rfc.appendix.Appendix%20B aligns with this code
(Specifically)
If the length mod 4 is 0, no padding is added;
if the length mod 4 is 2, two '=' padding characters are added;
if the length mod 4 is 3, one '=' padding character is added;
if the length mod 4 is 1, the input is malformed.
detect_secrets/plugins/jwt.py
Outdated
class JwtTokenDetector(RegexBasedDetector): | ||
secret_type = 'JSON Web Token' | ||
denylist = [ | ||
re.compile(r'eyJ[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a ?
after the *
? I believe this will make it non-greedy i.e. more efficient.
In other words
re.compile(r'eyJ[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.?[A-Za-z0-9-_.+/=]*?'),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinHock Absolutely, and thanks for pointing it out.
08d0ba2
to
c54dcd6
Compare
* fix: auto fix ibm_db import issue Supports https://github.ibm.com/git-defenders/detect-secrets-discuss/issues/306 * fix: support python 2 * fix: cov
* fix: auto fix ibm_db import issue Supports https://github.ibm.com/git-defenders/detect-secrets-discuss/issues/306 * fix: support python 2 * fix: cov
Remove will_be_mocked fix: auto fix ibm_db import issue (Yelp#239) Supports https://github.ibm.com/git-defenders/detect-secrets-discuss/issues/306 * fix: support python 2 * fix: cov
New plugin checking for JWTs
Motivation
The current high entropy base64 string checker would not find JWTs because they contain dots too. JWTs are more and more widespread - one very good example is Kubernetes API server tokens. The following very typical way of using detect-secrets for example would not detect a kubeconfig file committed in the codebase:
detect-secrets scan --use-all-plugins --no-keyword-scan
Of course, Kubernetes is not the only use case and because JWTs have an easy-to-validate format, I thought it would be worth checking for them as well - I think it's reasonable to expect low false positive ratio from this plugin.
How does it work?
I have implemented a simple regex based plugin that first filters for potential tokens just by looking at the character set and format (see https://tools.ietf.org/html/rfc7519 for details). Then it refines the search by validating the JWT format and encoding (base64url+json). I choose not to include a full JWT library just for this as I do not need to support signature validation or expiry checking here - I only intend to check that the JWT is formally correct. It shouldn't want to be too smart anyways. It is generally not possible to know where that JWT is accepted just by looking at it, so verification by network call is not supported by this plugin.
Test cases are documented in the source code for easier review.