sicpa-dlab / didcomm-python Goto Github PK

I'm not too familiar with how the DIDPeer DIDs are supposed to work, but according to this comment, it appears that there may be some issues with Peer DID Numalgo 2 generation across libraries between https://github.com/sicpa-dlab/didcomm-rust/ and here. The output is different enough that it has caused a conversion layer to be built to convert the DID between libraries in order to maintain inter-operability. Daniel has submitted a PR to the Spec to make clarifications decentralized-identity/peer-did-method-spec#62 but as of this moment it has not been merged.

Irregardless of the merge status, did:peer:2 DIDs generated by didcomm-rust and didcomm-python are incompatible with each-other when a service endpoint is defined. didcomm-python is expecting a flat servicesEndpoint structure, where-as didcomm-rust is expecting an object.

cc: @dbluhm

The ResolversConfig object groups the SecretsResolver and DIDResolver objects into one. This is then passed around as a method parameter for the pack and unpack operations.

My argument is that the ResolversConfig consumes more thought-space than is won by having one fewer method parameters on the pack and unpack operations. As a user of the library, it merely causes confusion (ResolversConfig is not an intuitive name to indicate what it contains nor how it is to be used). I believe it would be better to just individually pass the SecretsResolver and DIDResolver objects to these methods.

https://github.com/sicpa-dlab/didcomm-python/blob/main/didcomm/message.py#L167-L168

The message class should accept a list of strings for the ack parameter but the Message._validate method is enforcing a single string value.

PyDID is a library specifically designed for validating and representing DID Documents. Rather than redefining these structures, should we be reusing structures from PyDID?

The peerdid python library was recently updated to use PyDID for it's document representations. Moving this library to also use PyDID would make it easier to integrate the two libraries.

cc @chumbert @yvgny

The current version is pinned to beta release 1.0.0b1. This should be updated to a recent official release.

GenericMessage class expects custom headers in a custom-headers field as a List[Header] from the plain json/dict, instead of a header in the root msg. Example of a custom header Is return_route.

It would seem that in python 3.11, named tuples aren't behaving exactly the same way as before. I am getting the following error when attempting to pack a message:

alg = <AuthCryptAlg.A256CBC_HS512_ECDH_1PU_A256KW: ('ECDH-1PU+A256KW', 'A256CBC-HS512')>

    def _build_header(to: List[Key], frm: Key, alg: AuthCryptAlg):
        skid = frm.kid
        kids = [to_key.kid for to_key in to]

        apu = to_unicode(urlsafe_b64encode(to_bytes(skid)))
        apv = calculate_apv(kids)
        protected = {
            "typ": DIDCommMessageTypes.ENCRYPTED.value,
>           "alg": alg.value.alg,
            "enc": alg.value.enc,
            "apu": apu,
            "apv": apv,
            "skid": skid,
        }
E       AttributeError: 'tuple' object has no attribute 'alg'

.venv/lib/python3.11/site-packages/didcomm/core/authcrypt.py:129: AttributeError

See https://github.com/sicpa-dlab/didcomm-python/blob/main/didcomm/core/utils.py#L386-L402

The are_keys_compatible helper method seems to be inappropriately restrictive when used to determine if a sender key can encrypt to a recipient key. Presently, the check is requiring that sender and recipient have expressed their keys as the same type AND format. If verification method types were discrete, this check would hold. However, I would expect that a verification method with type X25519KeyAgreementKey2019 and a verification method with type JsonWebKey2020 would be "compatible" if the JWK represents an x25519 key.

Other pairings likewise seem like they should be "compatible:"

• X25519KeyAgreementKey2019 and X25519KeyAgreementKey2020
• Ed25519VerificationKey2018 and Ed25519VerificationKey2020
• Ed25519VerificationKey2018/Ed25519VerificationKey2020 and JsonWebKey2020 when using ed25519 curve

The pack and unpack examples in the readme are out of date. Snippet from the README:

# ALICE
message = Message(body={"aaa": 1, "bbb": 2},
                  id="1234567890", type="my-protocol/1.0",
                  frm=ALICE_DID, to=[BOB_DID])
pack_result = await pack_encrypted(message=message, frm=ALICE_DID, to=BOB_DID)
packed_msg = pack_result.packed_msg
print(f"Sending ${packed_msg} to ${pack_result.service_metadata.service_endpoint}")

# BOB
unpack_result = await unpack(packed_msg)
print(f"Got ${unpack_result.message} message")

Note that the pack and unpack methods do not take as parameters the resolver objects mentioned as being required in the previous section of the readme.

The verification check for the python 3.11 is absent.

Add the verification path for this version.

Messages are validated on from_dict but the error message is too general to be useful for debugging. We should improve this. Example error output:

didcomm.errors.DIDCommValueError: Plaintext message structure is invalid: Message(id=‘213d81ce-bb89-4932-b3d9-88b21e0db597’, type=‘https://didcomm.org/report-problem/2.0/problem-report’, body={‘code’: ‘e.p.msg’, ‘comment’: ‘decoding to str: need a bytes-like object, list found’}, frm=‘did:peer:2.Ez6LStGW8tJKk3vJQNQczW5HQtJDJMoGQmMV2YfPaxs6r44wH.Vz6MkgtowzANZkqXKccDiJXa6hu2ErEXsmoYvUnci6GTbZBgo.SeyJ0IjoiZG0iLCJzIjoiZGlkY29tbTovL3F1ZXVlIiwiciI6W10sImEiOlsiZGlkY29tbS92MiJdfQ’, to=[‘did:peer:2.Ez6LStGW8tJKk3vJQNQczW5HQtJDJMoGQmMV2YfPaxs6r44wH.Vz6MkgtowzANZkqXKccDiJXa6hu2ErEXsmoYvUnci6GTbZBgo.SeyJ0IjoiZG0iLCJzIjoiZGlkY29tbTovL3F1ZXVlIiwiciI6W10sImEiOlsiZGlkY29tbS92MiJdfQ’], created_time=None, expires_time=None, from_prior=None, please_ack=None, ack=[‘yWd8wfYzhmuXX3hmLNaV5bVbAjbWaU’], thid=None, pthid=‘yWd8wfYzhmuXX3hmLNaV5bVbAjbWaU’, attachments=None, custom_headers=None)

The is_did_url function found in didcomm.core.utils checks only for a fragment. Other valid DID URLs, such as did:example:bob/some/path?some=query would fail the check.

In the context of DID Doc processing, checking that a value is a DID URL with only a fragment is valid; however, we should more precisely name the function to clearly mark this intent.

It seems authlib isn't supporting key validation for EC keys...

But in the case of ECDH-1PU, where we do a C(1e,2s) key agreement, key validation is essential (especially in the multi-recipient case) since a malicious static key could otherwise easily cause invalid curve attacks or significantly reduce the entropy of the KEM.

Could we add a key validation function?

Would it be possible to make a new release soon?

• #94
• #96

It's blocking some agents from being interoperable with other agents that we are building.

Apologies for the nitpick but I think Secret.verification_material is poorly named. From the docstring:

verification_material (VerificationMaterial): A verification material representing a private key.

I would consider "verification material" to be the key material used to verify a signed message i.e. the public key, not the private key. Would there be any objections to be me changing this value to private_key or something similarly clear?

I would like to recommend that we restructure the API of this library to accept standard objects (i.e. dictionary or perhaps dataclass?) wherever it is expecting a Message object. didcomm-python should then perform any necessary validation for the given operation but otherwise assume the data is well structured as received from the caller.

I make this recommendation for a few reasons:

• People (including me) are opinionated about their models. Letting the caller bring their own models (or not; I think it's perfectly acceptable for people to just wing it with raw dictionary values) averts problems for our library users in the long run. This will enable an ACA-Py developer to continue to use their Marshmallow models, others to use Pydantic, still others to use attrs, and the rest to YOLO it.
• Frankly speaking, the current Message model definitions are pretty rough right now. They're defined with a mixture of attrs and dataclasses but are not actually using attrs to perform any validation; instead, the validation is done by hand.
• Most operations using Message almost immediately convert it to a dictionary and operate on that anyways.

This raises questions about what the return object of the unpack operation should be. I think there are plenty of examples out there where the caller specifies a class or a callable that can be used to deserialize the object.

I am not suggesting that the Message object should be removed altogether. If the user wants more structure than a dictionary but doesn't want to define it themselves, then Message would be available. Additionally, there's likely value in continuing to use it internally for the Forward message.

Open to thoughts.

cc @TelegramSam @chumbert @burdettadam

It'd be great if the library automatically assigned message IDs if not provided. This takes the need to do it away from users.

Additionally, it'd be great if thid defaulted to the message id if no other value present.

What format should be used? uuids?

I can write this if a format is agreed upon.

The didcomm-python library should "export" the key components of the library from it's main package __init__.py.

This will make it easier to consume and direct people to the types and objects they're most likely interested in. For example, rather than:

from didcomm.pack_encrypted import pack_encrypted
from didcomm.unpack import unpack
from didcomm.message import Message, Attachment
# ... etc

We should make these importable as:

from didcomm import pack_encrypted, unpack, Message, Attachment # etc. etc.

This will create a more cleanly defined "public interface" for the package and help people discover key entry points to the library.