decentralized-identity / didcomm-messaging

https://identity.foundation/didcomm-messaging/spec/
Apache License 2.0
167 stars 57 forks source link

Should implementation check consistency of 'from', 'to', 'skid' and similar headers for authcrypt, anoncrypt, sign envelopes and plaintext? #237

Closed vimmerru closed 2 years ago

vimmerru commented 3 years ago

I suggest imagine multiple situations to think and answer:

  1. We have Signed message from Alice to Bob, but from field in plaintext is empty or different.
  1. We have message in anoncrypt(authcrypt(plaintext)) envelopes, but set of to dids/keys in anoncrypt, authcrypt and maybe plaintext is different.
  1. We have authcrypt(plaintext) message encrypted for multiple keys. Bob owns multiple of this case on this device.
  1. We have anoncrypt(authcrypt(plaintext)) message to Bob. anoncrypt and authcrypt parts have different to, but Bob still can decrypt it as he owns keys for both parts.

It's just examples of such cases i can imagine significantly more similar corner cases. Ideally specification should define process of unwrapping every envelope and checks it should perform after it.

ashcherbakov commented 3 years ago

Another example: authcrypt(signed(plaintest)). Should we check that skid from authcrypt and kid from JWS belong to the same sender DID, or encryption and signature can be done by different DIDs (for example, pairwise DID is used for encryption and a public DID (rooted on blockchain for example) is used for signing).

mirceanis commented 3 years ago

Is it just me or does anyone else feel disturbed that multiple layers need to be considered simultaneously? What would be the purpose of both authcrypt and signing in one go? I suppose, some operation pairs can be grouped for the sake of saving some bytes of overhead, but otherwise why not think of these as separate operations?

vimmerru commented 3 years ago

but otherwise why not think of these as separate operations?

If applications will need to analyse different layers and make it's own trust decisions based on non-defined logic than i don't see any value in DIDComm. We just recommend to use Jose, not define protocol.

ashcherbakov commented 3 years ago

What would be the purpose of both authcrypt and signing in one go?

I agree that this is rather an edge case, but the current spec says nothing about it explicitly. The spec should either say that such construction is invalid (and implementations will raise an error), or define how implementations should handle this situation.

As for potential example of authcrypting a signed message: Alice has two DIDs: pairwise DID for Alice-to-Bob communication (DID_AB) and a public DID (DID_A) rooted on a blockchain for example. Alice and Bob want to authenticate themselves in the scope of established pairwise connection (so authcrypt is done via DID_AB), but at the same time non-repudiation (for a 3d party) is needed fort a particular message. So, the message is also signed by Alice's public DID (DID_A). I'm not sure that this example is valid for DID Comm. So, it would be great to have the spec explicitly clarify situations like this.

mirceanis commented 3 years ago

@vimmerru What I mean is that each layer serves a specific purpose. If all layers need to be analyzed simultaneously, then what do you do with even more complex combos like anoncrypt(authcrypt(signed(anoncrypt(signed(....)))))? It doesn't make much sense to think of this situation, right?

It makes a lot of sense to compare from/to fields from payloads with skid and kid from headers per layer of encryption/signing and raise errors when they don't match, but not across layers.

@ashcherbakov in your example, I would argue that the authcrypt layer should not care that payload it [de]encrypts is signed or not, its only purpose should be to provide authenticated encryption, not judge the payload.

vimmerru commented 3 years ago

It makes a lot of sense to compare from/to fields from payloads with skid and kid from headers per layer of encryption/signing and raise errors when they don't match, but not across layers.

I think what can be useful to avoid such questions:

otherwise API that application developers expect:

(from, to, plaintext) = decrypt(msg)

may mean quite different things in different implementations.

ashcherbakov commented 3 years ago

We don't insist on any option/answer here. We just want the spec to provide explicit and clear description of such cases, so that it says what's acceptable and expected, what needs to be verified by implementations, and what's an error or unexpected format, where implementations can raise an exception/error.

dhh1128 commented 3 years ago

First of all, signing is likely to be unusual. Not weird, just unusual. That's because the major use case for signing in non-DIDComm contexts -- knowing with 100% certainty who sent a message -- is NOT addressed with signing in DIDComm. To know the sender of a message, you use authenticated encryption. It's only if you need non-repudiation (proving to the rest of the world, not just the recipient, who is behind a message) that you add signing. And I believe non-repudiation to happen in a tiny minority of cases.

Secondly, I believe anoncrypt and authcrypt are usually going to be alternatives (pick one or the other), not things that are combined.

By far the most common scenario, when Bob is looking at a message, will be that it was authcrypted by Alice. Just authcrypted -- not authcrypted and signed, not anoncrypted then authcrypted, not anon+auth+sign. This is the 95% case, at least. In the remaining cases, Bob might see anoncrypt(authcrypt(message)). But this will be unusual.

On the other hand, by far the most common scenario, when Bob's agency/mediator is looking at a message, is that it will be anoncrypted. Just anoncrypted -- not anoncrypted and anything else. This is the 95% case. It's also possible (not illegal) to send the agency/mediator an authcrypted message. But I don't know why you would want to do this. If Alice DOES send an authcrypted message to Bob's mediator, there doesn't have to be any relationship between the A.did@A:Mediator DID that Alice uses to authcrypt her forward message, and the A.did@A:Bob DID that Alice uses to authcrypt the inner payload that Bob eventually sees.

No other combinations make sense. Ignoring the rewrapping that happens when you are preparing a complex route, authcrypt(authcrypt(payload)) is wrong. anoncrypt(anoncrypt(payload)) is wrong. More layers of encryption are wrong.

My suggestion is that implementation code should unpack/decrypt exactly and only one layer at a time, NOT try to pull apart two layers as if they were one. In the 9x% case where Bob gets an authcrypted message from Alice, this single unpack operation will turn the message into plaintext. In the rare case where Bob gets an authcrypt(sign(payload)) message, unwrapping once will produce sign(payload) as output. Bob can then be properly surprised that he got something signed, and call unpack again on the signed stuff to get the original payload. This is more or less what would happen with human mail if you got an envelope and opened it. Usually you'd expect to see a letter inside. But if, instead, you saw another envelope with a special wax seal inside, you'd know that you're dealing with something carrying a special signature, and you'd open the second envelope. Performing a second unpacking operation is not an inconvenience. And in the other rare case, where the message for Bob is in the form anoncrypt(authcrypt(something)) -- and "something" could be either signed or plaintext -- Bob should call the unpack function once and observe an inner authcrypted envelope, then again to find out what's inside that envelope. Libraries that try to understand what's going on with more than one layer at a time are doing too much.

Separate from the implementation advice I proposed in the previous paragraph, I think we should update the spec with an explanation of each of these combinations -- why they exist, what they mean, when they should be used.

If everybody on this thread agrees with my reasoning, I will open a PR to this effect.

vimmerru commented 3 years ago

@dhh1128

My suggestion is that implementation code should unpack/decrypt exactly and only one layer at a time, NOT try to pull apart two layers as if they were one. In the 9x% case where Bob gets an authcrypted message from Alice, this single unpack operation will turn the message into plaintext. In the rare case where Bob gets an authcrypt(sign(payload)) message, unwrapping once will produce sign(payload) as output. Bob can then be properly surprised that he got something signed, and call unpack again on the signed stuff to get the original payload. This is more or less what would happen with human mail if you got an envelope and opened it. Usually you'd expect to see a letter inside.

I have feeling that if we go this way our developers MUST be crypto experts to use this API and specification and getting every message will be uggly loop in a program with a lot of conditions inside.

vimmerru commented 3 years ago

@dhh1128 Could you formally answer to questions we asked? Most of them are related to only one layer, not to layer combination.

dhh1128 commented 3 years ago

I have feeling that if we go this way our developers MUST be crypto experts to use this API and specification and getting every message will be uggly loop in a program with a lot of conditions inside.

I disagree with the word "every". Getting unusual messages will require two calls. Getting normal messages will require one.

Where I feel we are not aligned is whether we should try to hide unusual situations. I say "no", because I want the code to look different (follow a different observable codepath) when the semantics are different. Receiving a certified letter in the mail is different from receiving an ordinary letter, because you have to sign for it. But it hardly ever happens -- and when it does, the extra work you are doing teaches you something about the extra guarantees that come with it.

I feel like you are preferring to make all codepaths as similar as possible, from the perspective of the application developer, because you feel that would be easiest for them. Is this an accurate summary of your preference?

dhh1128 commented 3 years ago

The formal answers to many of your questions depends on a concept called message trust contexts (MTC), that is described here: https://github.com/hyperledger/aries-rfcs/blob/master/concepts/0029-message-trust-contexts/README.md. We don't have to implement them exactly as that RFC describes -- but we need some construct like them. See below.

We have Signed message from Alice to Bob, but from field in plaintext is empty or different. Should implementation check it and return error for this case?

Is it illegal for Alice to mail to Bob a message signed by Carol? I don't think so. But I agree that it's an anomaly. What I would like here is a warning. But I don't see a way to raise warnings. What we should do is return an MTC that describes the anomaly and recommends different trust (trust in Carol, not Alice) as a result.

If not should implementation not only unwrap sign envelope and return plaintext, but return additional metadata with some information about trust received from sign envelope?

"additional metadata" = MTC

We have message in anoncrypt(authcrypt(plaintext)) envelopes, but set of to dids/keys in anoncrypt, authcrypt and maybe plaintext is different. Should implementation check it and return error for this case?

anoncrypt and authcrypt keys will be different by design; if they aren't different, anoncrypt makes no sense. So I don't understand the question.

We have authcrypt(plaintext) message encrypted for multiple keys. Bob owns multiple of this case on this device. Should implementation check auth for all keys owned by Bob?

No. It is wrong for a sender not to encrypt for all of Bob's keys. But Bob may have chosen not to tell the sender about all of the keys that he has. So it not an error to encrypt a message only to some of Bob's keys.

What it should do if one key is correctly wrapped, but second not?

Keys at the same level in a single authcrypted envelope -- or keys used by different authcrypted envelopes?

Should implementation return some metadata about what keys where checked?

Yes. In MTC.

We have anoncrypt(authcrypt(plaintext)) message to Bob. anoncrypt and authcrypt parts have different to, but Bob still can decrypt it as he owns keys for both parts. Should implementation check it and return error for this case?

No. I don't think this is an error. I also don't think a single call should look at both of those layers, anyway.

If not should it return additional metadata about keys used for decrypt?

Yes. In MTC.

vimmerru commented 3 years ago

I disagree with the word "every". Getting unusual messages will require two calls. Getting normal messages will require one.

You don't now is it 'usual' or 'not'. As a result you need 3 calls and condition always. Call #3 is to check that message is usual. Also it should be inside cycle as there can be multiple unusual envelopes.

Yes. In MTC.

What is MTC?

dhh1128 commented 3 years ago

What is MTC?

Message Trust Context. See https://github.com/hyperledger/aries-rfcs/blob/master/concepts/0029-message-trust-contexts/README.md.

vimmerru commented 3 years ago

@dhh1128 I like this idea, but what we miss is an instruction how to build MTC based on specific message and it's layers.

dhh1128 commented 3 years ago

Some example pseudocode for receiving any DIDComm message:

# Starting point is an encrypted envelope that just came in over the wire.
inside, mtc = unpack(envelope, MTC())
if inside.is_encrypted:
    if not envelope.anoncrypted or not inside.anoncrypted:
        raise Exception("The only reason to double-encrypt is if the outer envelope provides anonymity, and the inner one provides the main encryption. Sender is behaving strangely. Message rejected.")
    payload, mtc = unpack(inside, mtc)
else:
    payload = inside
if payload.is_signed:
    plaintext, mtc = unpack(payload, mtc)
else:
    plaintext = payload
# Now examine mtc, which has accumulated state over repeated calls. Does the trust match your
# requirements? That will depend on what type of message it is. If the plaintext message is to make
# the next move in a chess game, then you don't care much about the identity of the sender, as long
# as it's a sender you know. But if the plaintext message is to agree to a mortgage, you probably need
# to know that the message was signed. Etc. Here, filter for all error conditions that the application
# cares about.
if plaintext.type == something_needing_nonrepudiable and not mtc.nonrepudiable:
    raise Exception("I can't prove to other parties that the {plaintext.type} message came from {mtc.sender_did} because it wasn't signed.")
if plaintext.type ==  something_needing_authcrypt and not mtc.authcrypted:
    raise Exception("I need to know who sent the the {plaintext.type} message, but sender is anonymous."

Observations:

  1. This is application-level code. The library provides the impl of unpack() and of message trust contexts, but the app decides what to do about extra layers of encryption. That is not the library's job.

  2. The first error could probably be detected by the library to make application-level code simpler. But then the app has to be prepared to handle a library error... But in the later parts of the code, whether the extra layers of encryption meet the library's needs and expectations is context-dependent (where "context" could be other messages previously processed, the application-level protocol that's underway, what type of app it is, etc). I don't believe there is any way to avoid writing logic like these latter conditionals. They are analogous to the conditionals you have to write in a web app to decide what you will do if an endpoint is called with improper authorisation. It's an app-level concern.

ashcherbakov commented 3 years ago

@dhh1128 There is an alternative API proposal here: https://hackmd.io/aD4TzKRhT1ma2ERl6EKU_g?view#Example-API.

// A. decrypt and verify signature
(payload, signed_message) = unpack(transferred_bytes)

// B. decrypt and verify signature later
(payload, signed_message) = unpack(transferred_bytes, verify_signature=false)
if signed_message != null:
    verify_signature(signed_message)

Regardless of combination, just one common call can be called by receiver: unpack.

ashcherbakov commented 3 years ago

We should probably address the following items in the Spec: Proposed-Consistency-checks This is also closely related to #229: Error handling and edge cases.

ashcherbakov commented 3 years ago

@kdenhartog @dhh1128 Should we mark the issue as PR Needed?

kdenhartog commented 3 years ago

I suggest imagine multiple situations to think and answer:

1. We have Signed message from Alice to Bob, but `from` field in plaintext is empty or different.

* Should implementation check it and return error for this case?

* If not should implementation not only unwrap sign envelope and return plaintext, but return additional metadata with some information about trust received from sign envelope?

I'd say that's an implementation consideration. A conservative approach would be to always fail and error. A more "fail gracefully" approach would be to return the metadata about what passed and what failed and let the caller decide what to do with it. In the case of which one you should choose I think it depends on how you're architecting your system. For example, if you choose to implement an agent "all in one" I'd think you'd be fine just erroring on the first one. If you're going more in line with a microservice architecture where DIDComm envelope unpacking is a separate service then I'd think the second option is likely a better approach.

1. We have message in `anoncrypt(authcrypt(plaintext))` envelopes, but set of `to` dids/keys in anoncrypt, authcrypt and maybe plaintext is different.

* Should implementation check it and return error for this case?

I don't think this should ever error anymore. The spec may need to be updated to say this though. it's my opinion that to is a nice to have header at the JWM layer.

1. We have `authcrypt(plaintext)` message encrypted for multiple keys. Bob owns multiple of this case on this device.

* Should implementation check auth for all keys owned by Bob?

I don't think that's necessary. Our implementation just uses the first recipient kid recognized and fails if it doesn't fully authenticate for that first one. A more robust approach would be to try the first and if it fails try the next, but that seemed overly complex to implement so we opted against it.

* What it should do if one key is correctly wrapped, but second not?

We don't bother checking the second one. I'd say this is more of an implementation consideration. Some people could require more, some might say one is fine. I don't see a reason to check them all in this case. Do you have any that I might be missing?

* Should implementation return some metadata about what keys where checked?

Again, I think it depends on your architecture here, but in general we do make the assumption that this information is returned. Even when we don't use it, it's been useful to include it because it may provide useful information for generating a response. I think in the original indy-sdk implementation I went with returning this information as we, but you could just as easily manage this without returning the key information and instead use a message management engine which associates a thread with a recipient DID and the response is generated based on all keyAgreement keys in the DID Document of the associated recipient DID.

1. We have `anoncrypt(authcrypt(plaintext))` message to Bob. anoncrypt and authcrypt parts have different `to`, but Bob still can decrypt it as he owns keys for both parts.

* Should implementation check it and return error for this case?

Hmm, not sure how to would impact this here. Did you mean from instead?

* If not should it return additional metadata about keys used for decrypt?

I think this falls back on the architectual decision aspects.

It's just examples of such cases i can imagine significantly more similar corner cases. Ideally specification should define process of unwrapping every envelope and checks it should perform after it.

My opinion has been that much of this could fall back on the implementation details because many of these questions are more specific to how to process errors. I think it's more so that we just need to say "if this case happens throw this error back to the sender". I may have just misunderstood what you were trying to convey though.

kdenhartog commented 3 years ago

Another example: authcrypt(signed(plaintest)). Should we check that skid from authcrypt and kid from JWS belong to the same sender DID, or encryption and signature can be done by different DIDs (for example, pairwise DID is used for encryption and a public DID (rooted on blockchain for example) is used for signing).

My opinion was that originally this would be a "yes" this should match. This thread has convinced me there's valid use cases where decoupling these would be useful though. I'm now less certain about this then I was when I wrote the nesting code to process this. Let me think about it.

kdenhartog commented 3 years ago

Is it just me or does anyone else feel disturbed that multiple layers need to be considered simultaneously? What would be the purpose of both authcrypt and signing in one go? I suppose, some operation pairs can be grouped for the sake of saving some bytes of overhead, but otherwise why not think of these as separate operations?

Yeah... you're probably right that this may be an architectual conflation that I made. That would explain why our implementation always seemed more complex then it needed to be.

TelegramSam commented 3 years ago

I think:

from should always match on layered messages (not attached messages). from is optional, when not included, it is filled in from the skid from the encryption layer. Then previous rule applies.

I'm a fan of sending an error up when this check fails when decrypting a message.

AnomalRoil commented 3 years ago

Notice this is changing since:

Most notably: when receiving a signed message it is really important to verify we are included in the to field since otherwise it could be a surreptitious forwarding attack...

TelegramSam commented 2 years ago

Proposal: We add language that indicates the DIDs present in the encryption and signature layers must match the DIDs in the plaintext layer.

TelegramSam commented 2 years ago

Discussed WG 20220509. Check must be performed, and inconsistency errors are reported. Recipient MAY discard the message. After mime table - after no other envelope combinations allowed. Add small section, refer to it from the from and to plaintext headers.