feat: Use cryptography package and verify certificate signatures#5
feat: Use cryptography package and verify certificate signatures#5sylvainpelissier wants to merge 1 commit intonipo:masterfrom
Conversation
nipo
left a comment
There was a problem hiding this comment.
Thanks for this work. I think some parts need rework:
- Loading data from the network without user consent or configuration is edgy.
- Saving data to resources cannot work in a general case.
- Hitting the network every time we need to validate a document does not scale.
Using proper cryptography libraries rather than my silly cert parsing is a nice improvement though. I would consider it if submitted alone.
For real installation contexts, we'll have to cache certs properly.
We may have a keychain directory passed as keychain initialization parameter, we may provide a tool to refresh certs from public chains to that directory (add a tdd.main with some CLI entry points to do this on demand), but we cannot do it at runtime.
| s = int.from_bytes(self.signature[32:], "big") | ||
| signature = encode_dss_signature(r, s) | ||
| cert.public_key().verify(signature, self.signed_data, ec.ECDSA(hashes.SHA256())) | ||
| return True |
There was a problem hiding this comment.
This change shifts the semantics of the function.
Before, we had "signature_is_valid" that did as it advertised: tell whether signature is valid by returning a boolean. If it could not decide (because of missing key), it would raise an exception.
Now it asserts for signature being valid, excepts whatever the reason (cannot decide or invalid). We should either rename the function to something like assert_signature_valid() or catch InvalidSignature to return False.
| certs = blob.split(boundary) | ||
|
|
||
| for i, c in enumerate(certs): | ||
| for _, c in enumerate(certs): |
There was a problem hiding this comment.
We can remove enumerate() at all there.
| chains = files('tdd.chains') | ||
| name = issuer_cn + subject_cn | ||
| chain_file = chains.joinpath(name + ".der") | ||
| with chain_file.open("wb") as f: |
There was a problem hiding this comment.
Trying to write to resources will not work in many installation contexts.
If writing cert fails, it will basically fail the whole call stack.
We cannot do this that way.
cryptographypackage to handle certificates and signature verification