Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: only verify self-signatures over user ids and attributes #448

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hko-s
Copy link
Contributor

@hko-s hko-s commented Dec 20, 2024

This should fix deltachat/deltachat-core-rust#6350 (which I now believe is caused by the existence of third-party User ID certifications)

@dignifiedquire
Copy link
Member

can you add an explicit test please?

@hko-s
Copy link
Contributor Author

hko-s commented Dec 20, 2024

can you add an explicit test please?

good point. done.

@dignifiedquire dignifiedquire changed the title fix: only verify self-signatures over user ids and attributes fix!: only verify self-signatures over user ids and attributes Dec 23, 2024
@dignifiedquire
Copy link
Member

So while I understand the priniciple fix, I am not 100% convinced this should be merged as is. My current concern is , this means ending up with a bunch of signatures that are invalid/not verified on these calls, without any information about it.

@hko-s
Copy link
Contributor Author

hko-s commented Dec 27, 2024

After thinking for a while, I'm unsure that we can do any better than this.

Third party signatures can - by definition - not be verified without having signer's key material at hand (in the context of this call, we definitely don't).

We could drop third-party signatures during verification, but that would be surprising for users who would like to keep them.

Or we could make the TPK data structure more elaborate, and store third-party signatures in a separate place. However, that would be a relatively big deviation from what the OpenPGP RFCs outline, and move rPGP further towards "high level behavior" (away from more or less straightforwardly doing just the things the RFC explicitly outlines).

So my current sense is that this PR is the best way forward, and that your concern is just another way of saying: "using OpenPGP through a low level API is scary, because OpenPGP is weird" (which I fully agree with 😅)

@hko-s
Copy link
Contributor Author

hko-s commented Dec 27, 2024

All of that said, if we do merge this change, we should make sure that Delta is able to handle TPKs that contain third-party signatures, even in the most unfortunate packet orderings.

I'm not sure if Delta looks into signature packets at all, though?

@dignifiedquire
Copy link
Member

So, to make this better I would suggest this:

  • change the signature of the verify function to also pass a list of other keys, that will be used to to verify signatures
    • if a signature is invalid, return an "invalid signature" error
    • if a signature is missing a key, return a "missing key for signature" error, this should only be returned if all other signatures are actual valid

This would allow callers like deltachat to accept missing keys, if they want to, without hiding the fact that there are potentially invalid signatures

@hko-s hko-s mentioned this pull request Jan 4, 2025
4 tasks
@hko-s
Copy link
Contributor Author

hko-s commented Jan 4, 2025

That sounds like a reasonable direction, although it is a bit unfortunate that the "missing key for signature" case would use a Rust Error as a sort of warning.

I still wonder if it wouldn't be better to filter out elements that we don't "trust" (such as signatures that we can't verify, because we don't have the public key for them at hand). That would probably be a bigger change to the API, but I feel like it would be a smaller change in terms of the intended API semantics between rPGP and Delta Chat?

@dignifiedquire
Copy link
Member

I still wonder if it wouldn't be better to filter out elements that we don't "trust"

I think that is very bad, and problematic behaviour, maaaybe at a very high level, but not at this low level, you want to make sure everything is accounted for by default

@hko-s
Copy link
Contributor Author

hko-s commented Jan 5, 2025

After some more discussion, I increasingly think that this type of API is hard to provide in a way that is general, defensive, and reasonable for callers.

It might be better to deprecate/remove this "medium level" verify API, or at least not to try and extend it beyond its current state.

Instead rPGP could document/suggest approaches how applications can cover their specific requirements with the existing low level primitives (e.g. something like key.bindings().map_res(|binding| binding.verify())).

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.

Failed to import secret key
2 participants