Replace pyopenssl with cryptography module and add parsing of SAN extension #81
Conversation
…tensions (subjectAltName in particular)
| @@ -1,5 +1,8 @@ | |||
| Changelog | |||
| ========= | |||
| * next release | |||
There was a problem hiding this comment.
When you're ready to do a release, I think that semver would dictate that this should be at least 1.2 if not 2.0.
There was a problem hiding this comment.
i think we can get away with 1.2, if people do install -U they will get new lib and their stuff will not have to change.
They will just be more accurate, since classes attributes stayed the same from what i see.
| @@ -75,8 +75,7 @@ Troubleshooting | |||
|
|
|||
| Some setups (like MacOS) have trouble with building the dependencies required | |||
| for reading SSL certificates. If you don't care about SSL stuff and only want | |||
| to use sagan to say, parse traceroute or DNS results, then you can tell the | |||
| installer to skip building ``pyOpenSSL`` by doing the following:: | |||
| to use sagan to say, parse traceroute or DNS results, then you can do the following: | |||
There was a problem hiding this comment.
This may require additional explanation or none at all and probably needs a little experimentation on different platforms to be sure. PyOpenSSL was a pain in the ass to install on a Mac, but I'm not sure if that's also the case with cryptography. I know that in Linux environments, cryptography requires certain system-level libraries to be installed, and I'm sure that Mac & Windows each have their own quirks.
The good news however is that this is all beautifully documented by the cryptography team here so you might want to adjust this paragraph to just give the user a heads-up and point them to that URL if they have trouble.
setup.py
Outdated
| @@ -6,15 +6,15 @@ | |||
|
|
|||
| name = "ripe.atlas.sagan" | |||
| install_requires = [ | |||
| "IPy", | |||
There was a problem hiding this comment.
This isn't part of this issue is it? If not, it should probably be in a separate commit, if only to keep future developers from conflating IPy with the addition of cryptography. The same goes for the documentation references to IPy above.
ripe/atlas/sagan/ssl.py
Outdated
| self._process_validation_times(x509) | ||
| def _add_extensions(self, cert): | ||
| for ext in cert.extensions: | ||
| if ext.oid._name == 'subjectAltName': |
There was a problem hiding this comment.
Nit: the rest of the module is using double quotes for strings. It'd be nice to stay consistent.
ripe/atlas/sagan/ssl.py
Outdated
| o = None | ||
| c = None | ||
| for attr in name: | ||
| if attr.oid.dotted_string == '2.5.4.6': # country |
There was a problem hiding this comment.
Arbitrary strings like "2.5.4.6" don't make a lot of sense out of context like this, so it might be nicer to place them in a class constant like OLD_STYLE_COUNTRY="2.5.4.6" (or a more appropriate name). I don't really get what's going on here, but maybe that's just my ignorance of arcane SSL rules. If it's particularly weird though, a docstring on the function might also be nice.
|
Honestly, I expected a lot more headaches in the conversion, but it looks like it was a lot easier than I thought! |
|
Seems okay to me, I don't have a lot of experience with both libs, so my input can only be minimal. |
No description provided.