change certificate processing code to use python-cryptography #217
Conversation
There was a problem hiding this comment.
Wow, awesome work! 👍
You are using the fully dotted name in several places. In Python attribute lookup comes with a cost. It might be better to do from cryptography.x509.oid import NameOID.
Side note: cryptography disabled my SELinux execmem fix for Python 3 because they run into segfaults on some platforms. We have to keep it in mind when we move ipaserver to Python 3.
| cryptography.x509.oid.NameOID.BUSINESS_CATEGORY: 'businessCategory', | ||
| cryptography.x509.ObjectIdentifier('2.5.4.9'): 'STREET', | ||
| cryptography.x509.ObjectIdentifier('0.9.2342.19200300.100.1.1'): 'UID', | ||
| } |
There was a problem hiding this comment.
I see you got most EV cert fields covered. postal code http://oid-info.com/get/2.5.4.17 is missing, though.
There was a problem hiding this comment.
Yes, well spotted. Updated PR coming.
| if not subject_base: | ||
| return None | ||
|
|
||
| nickname_by_subject_dn = { |
There was a problem hiding this comment.
I wonder if it makes sense to move this mapping to a publicly available module, e.g. ipalib.api. It might be useful to others.
There was a problem hiding this comment.
Not really, this is only used to construct DN of the cert renewal LDAP entry, which is touched only from this script.
There was a problem hiding this comment.
Furthermore, I'd like to (eventually) get certmonger to put the nickname into the helper's environment, and then remove the mapping.
There was a problem hiding this comment.
Don't, it's actually better not to derive the DN from the local nickname, because the nickname may potentially change, but the DN must stay the same.
There was a problem hiding this comment.
@jcholast right, but if we get certmonger to put the cert location (which for NSSDB, includes nickname) into the helper environment, the subject does not need to be referenced at all, to find the nickname.
Recall that we used to read nickname directly from PKCS #10 "friendlyName" attribute, but that was removed to avoid doing that parsing work ourselves.
There was a problem hiding this comment.
By DN I did not mean the subject DN of the certificate, but the DN of the LDAP entry - it must remain static, otherwise renewal wouldn't work correctly, and given that the nicknames in this mapping are only ever used to construct the DN of the LDAP entry, the mapping must remain static as well.
There was a problem hiding this comment.
OK, thanks for the clarification. I will not worry about filing that certmonger ticket then :)
| cert.tbs_certificate_bytes, | ||
| asn1Spec=rfc2459.TBSCertificate() | ||
| )[0] | ||
| OID_SAN = univ.ObjectIdentifier('2.5.29.17') |
There was a problem hiding this comment.
cryptography.x509.oid.ExtensionOID.SUBJECT_ALTERNATIVE_NAME
There was a problem hiding this comment.
Here we must construct the actual pyasn1 OID value for later comparison.
Do you propose to replace the string literal with cryptography.x509.oid.ExtensionOID.SUBJECT_ALTERNATIVE_NAME.dotted_string ?
I think we can tolerate the string literal here :)
frasertweedale
force-pushed
the
refactor/6398-cert-processing
branch
from
396149a
to
149fe1f
Compare
|
The travis-ci failure is due to two minor pep8 violations, which I intend :) |
| return nickname_by_subject_dn[DN(subject)] | ||
| except KeyError: | ||
| if not api.Backend.ldap2.isconnected(): | ||
| api.Backend.ldap2.connect() |
There was a problem hiding this comment.
This should be removed now that #216 was merged.
There was a problem hiding this comment.
Thanks, I updated the PR.
The upcoming change to using python-cryptography for certificate process will require a way to convert ``cryptography.x509.name.Name`` values to ``ipapython.dn.DN``. Update the ``DN`` constructor to accept a ``Name``. Part of: https://fedorahosted.org/freeipa/ticket/6398
Update ``ipalib.pkcs10`` module to use python-cryptography for CSR processing instead of NSS. Part of: https://fedorahosted.org/freeipa/ticket/6398
In the dogtag-ipa-ca-renew-agent-submit certmonger renewal helper, we currently use our hand-rolled PKCS freeipa#10 pyasn1 specification to parse the friendlyName out of CSRs generated by certmonger (it contains the NSSDB nickname of the cert). Use other information from the renewal helper process environment to determine the nickname and remove our PKCS freeipa#10 pyasn1 spec. Part of: https://fedorahosted.org/freeipa/ticket/6398
Avoid use of the nss.data_to_hex function for formatting certificate fingerprints. Add our own helper functions to format the fingerprints as hex (with colons). Part of: https://fedorahosted.org/freeipa/ticket/6398
Remove our hand-rolled pyasn1 specifications for X.509 in favour of those provided by the pyasn1-modules library. This also avoids a bug in our _Extension spec wherein parsing fails if the 'critical' field is absent. Part of: https://fedorahosted.org/freeipa/ticket/6398
Update x509.load_certificate and related functions to return python-cryptography ``Certificate`` objects. Update the call sites accordingly, including removal of NSS initialisation code. Also update GeneralName parsing code to return python-cryptography GeneralName values, for consistency with other code that processes GeneralNames. The new function, `get_san_general_names`, and associated helper functions, can be removed when python-cryptography provides a way to deal with unrecognised critical extensions. Part of: https://fedorahosted.org/freeipa/ticket/6398
This code was presumably once used for testing, but has been subsumed by the actual test suite. Part of: https://fedorahosted.org/freeipa/ticket/6398
frasertweedale
force-pushed
the
refactor/6398-cert-processing
branch
from
149fe1f
to
2a4e29c
Compare
ghost
added
the
This commit changes certificate processing code to use python-cryptography
instead of NSS.
Part of the refactoring effort, certificates sub-effort.
Reviewed at dkupka/freeipa:pull/1