Merge develop for release 1.2 #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In order to drop off most of subprocess call, a method to generates self signed certificates has been added
Certificate was generate calling openssl commands using subprocess. In order to avoid using subprocess (except for specific cases) certificate generation has moved to cryptography, producing more scalable x509 objects. The method was already been implemented with the last merge, with this commit I'm changing validation and x5u functions according to it. Validation now has just to look for self.path_to_key, as attribute that give a key file to read from. x5u functions, need only to grab self.x5c (that's already been converted to x5c format), without open and process any file. During certificate generation now no file will be produced anymore, that's the most beneficial things
Generating public key from x5c, was using a non imported method. This has been fixed
Since certificates are now generated using cryptography (see two commits behind), looking for remove the 'testing.crt' file if exists, is now useless, so has been removed
Following the last commits intention (dropping off subprocess as far as possible), the method get_key_from_ssl_certs, now use the ssl library, instead of calling the s_client via subprocess. This removed other two file to handle with, and lot of line of code. Exception for non existing domain, or missing connection has to be checked up.
Two exception of the new auto_try feature (see previous commit), has been handled. This require to import the socket library. If exception to handle will get more, maybe a singl 'except Exception' statement should be used, even if it's going to be not pep8 compliant (there's so many things here that are not).
Added support for null signature. New option --null-signature, provide a token with 'HEADER.PAYLOAD.' format, instead of 'HEADER.PAYLOAD.SIGNATURE'. This does not depend on algorithm (setting alg as None generates no signature), user can provide every supported algorithm without this being overidden. Of course, setting this options will make the tool skipping al key validation; since signature is going to be a null character, it would be uselessly resource-consuming
Since now certificates and public keys retrieved with get_key_from_ssl_cert are stored as cryptography objects, we need no more to look for such filenames. So those states has been removed. Useless white lines used in docstrings has been removed
In order to stop using subprocess (for make the script more windows compatible, even if it's not a priority), now jwks files are downloaded with urllib.request instead of calling wget with subprocess. The method download_jwks has been added and implemented in jku/x5u functions. Subprocess can now be dropped off from imports. Anyway this functionality has to be considered experimental, since exceptions handling is not covering all possible errors yet (we have to ran tests). We won't use a generic except statement, since is a bad way to handle errors
Since urllib import are controversial (if you import urllib, interpreter won't access parse, error and request files, or if you import urllib.request, it implicit import also error and parse), the import has been moved to the most secure (secure only for this excepional case) way. 'from urllib import request, error, parse', works fine, without having implicits imports or double imports. Even if this is not good, especially for 'error', that's a common word, is not going to generate warnings for now'
Found two issues: 1) JWKS files not properly generated with jku and x5u functions. 2) (minor) --find-key-from-jwks, was returning an error if no key in the jwks file can verify the token This issues are being closed with this commit
dropped off pyOpenSSL dependency
This was
linked to
issues
Feb 18, 2021
DontPanicO
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge develop, for release 1.2
Closes (#7, #8)
FIXED
IMPLEMENTED
ENANCHEMENT