Skip to content

feat: Use cryptography package and verify certificate signatures#5

Open
sylvainpelissier wants to merge 1 commit intonipo:masterfrom
sylvainpelissier:cryptography
Open

feat: Use cryptography package and verify certificate signatures#5
sylvainpelissier wants to merge 1 commit intonipo:masterfrom
sylvainpelissier:cryptography

Conversation

@sylvainpelissier
Copy link

  • Use the cryptography package to handle certificates and signature verification
  • Lookup certificate when needed only
  • Verify the certificate signature from the CA when loaded
  • Update the TLS-Trust-service Status List from the link given on official 2D-DOC page
  • Remove unused dependency and code

Copy link
Owner

@nipo nipo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments