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

DidCommV2 doesn't specify 'typ' #93

Closed
wants to merge 1 commit into from

Conversation

FabioPinheiro
Copy link

DidCommV2 doesn't specify 'typ'
This makes it incompatible with other implementations

@FabioPinheiro
Copy link
Author

I didn't test anything so someone needs to figure out how to make this according to the specification.

@yvgny
Copy link
Contributor

yvgny commented Mar 9, 2023

@FabioPinheiro Thanks for your contribution!

From what I read in the specs, DIDComm may use the typ field (see https://identity.foundation/didcomm-messaging/spec/#iana-media-types). So maybe we should make this optional instead of removing completely?

@FabioPinheiro
Copy link
Author

yes and no

I'm basically only removing the check. The line 105 is still there: https://github.com/sicpa-dlab/didcomm-python/pull/93/files#diff-cb2b8c37b8a59e04819d6634da7909e96a364413c241a5a17a33c5e7685a4c1eR105

However, JOSE specs recommends to use typ on the outermost envelope to make JOSE structure formats self-describing.

We are adding to the content of the encrypted bytes...

On the outermost envelope, we can have "typ":"application/didcomm-encrypted+json"

{
"ciphertext":"...",
"protected":"eyJlcGsiOnsia3R5IjoiT0tQIiwiY3J2IjoiWDI1NTE5IiwieCI6IndtSU90Sm03MVUwTGI3NDJqYThudFo1NjlGWDlqRjlBVVJ0cm1WYTluU28ifSwiYXB2IjoiLWNOQ3l0eFVrSHpSRE5SckV2Vm05S0VmZzhZcUtQVnVVcVg1a0VLbU9yMCIsInR5cCI6ImFwcGxpY2F0aW9uL2RpZGNvbW0tZW5jcnlwdGVkK2pzb24iLCJlbmMiOiJBMjU2Q0JDLUhTNTEyIiwiYWxnIjoiRUNESC1FUytBMjU2S1cifQ",
"recipients":[{...}],
"tag":"...",
"iv":"..."
}

protected -> is decoded to {"epk":{"kty":"OKP","crv":"X25519","x":"wmIOtJm71U0Lb742ja8ntZ569FX9jF9AURtrmVa9nSo"},"apv":"-cNCytxUkHzRDNRrEvVm9KEfg8YqKPVuUqX5kEKmOr0","typ":"application/didcomm-encrypted+json","enc":"A256CBC-HS512","alg":"ECDH-ES+A256KW"}

But in this implementation "typ":"application/didcomm-plain+json" its required to be hard coded on the content (from ciphertext).

If you go to all the 'headers' https://identity.foundation/didcomm-messaging/spec/#message-headers and all the additional headers from the DID Comm extensions typ does not exist.

So I assume this implementation is unable to decrypt any of the test vectors/examples of the DID Comm v2 Sepc: https://identity.foundation/didcomm-messaging/spec/#appendix-c-test-vectors

@FabioPinheiro
Copy link
Author

In sicpa-dlab/didcomm-jvm you still have this field... but at least doesn't seem to be a requirement.

https://github.com/sicpa-dlab/didcomm-jvm/blob/993265c75c12924f7ded917e3c54282c327ab315/lib/src/main/kotlin/org/didcommx/didcomm/message/Message.kt#L58

@@ -102,7 +102,7 @@ def as_dict(self) -> dict:
d["from"] = d["frm"]
del d["frm"]

d["typ"] = DIDCommMessageTypes.PLAINTEXT.value
d["typ"] = DIDCommMessageTypes.PLAINTEXT.value # this is not part of the official specification
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is optional, I would rather leave it here. Not require it, but always put.

if f not in d:
raise MalformedMessageError(MalformedMessageCode.INVALID_PLAINTEXT)

if d["typ"] != DIDCommMessageTypes.PLAINTEXT.value:
Copy link
Contributor

Choose a reason for hiding this comment

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

It is important to delete 'typ' if present, not to make problems later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if present, I think we should check its value.

Copy link
Contributor

@dkulic dkulic left a comment

Choose a reason for hiding this comment

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

I am pro these changes, but we need some improvements in this PR. Can we add some tests?

@dkulic
Copy link
Contributor

dkulic commented Mar 30, 2023

Closing, because of lack of activity, and it is reimplemented here: #96

@dkulic dkulic closed this Mar 30, 2023
@FabioPinheiro
Copy link
Author

I only do this in my free time and I had very little time on the last three weekends. But important thing is that it's fixed.

But you asked for tests and I was trying to create a test set that everyone could use. Would help us to achieve interoperability at the DID Comm libraries level.

If you could contribute with your test set would be nice decentralized-identity/didcomm.org#77

@dkulic
Copy link
Contributor

dkulic commented Mar 31, 2023

No problem Fabio, like you said, it is important that it's fixed now 👍

@FabioPinheiro
Copy link
Author

When will there be a release with the fix?

@yvgny
Copy link
Contributor

yvgny commented Apr 12, 2023

@FabioPinheiro This will be released as soon as #98 is merged

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.

3 participants