search

Indicio-tech / pydid

Python library for validating, constructing, and representing DIDs and DID Documents
Apache License 2.0
11 stars 12 forks source link

add = to valid characters in regex #62

Closed Jsyro closed 1 year ago

Jsyro commented 1 year ago

b64 encoded entries may be padded with ='s.

Proposed Change to #61

swcurran commented 1 year ago

To add some additional colour to this PR. In Aries, we recommend code that encodes base64 data SHOULD NOT adding padding, but code that decodes base64 data MUST accomodate the presence of padding ((example)). This is a pain, but it is pretty common for people to use libraries that add padding and so as nice as it is to require it insist the data not having padding, one has to expect it will happen from time to time…

dbluhm commented 1 year ago

So I double checked the DID Core spec rules here: https://www.w3.org/TR/did-core/#did-syntax

It defines the idchar as:

idchar             = ALPHA / DIGIT / "." / "-" / "_" / pct-encoded

This does not include =. I'm sympathetic to the need we're trying to address but I try to keep these rules as close to the DID spec as I can. I do also try to accommodate common exceptions where it makes sense (i.e. publicKey vs verificationMethod and other things that have changed over time with the DID spec). What are your thoughts on these findings? I know we are generally permissive of padding vs no padding elsewhere in DIDComm land but perhaps it is best to enforce no padding in this case? @swcurran @Jsyro

dbluhm commented 1 year ago

It kinda seems like the real problem is that the did:peer spec isn't technically in alignment with the DID core spec.

Jsyro commented 1 year ago

It kinda seems like the real problem is that the did:peer spec isn't technically in alignment with the DID core spec.

That appears to be the root issue. The Peer DID REGEX/SPEC here is not compliant with the DID REGEX/SPEC here... I'll let @swcurran weigh in.

swcurran commented 1 year ago

@dhh128 — what do you think of this?

It looks like we need to explicitly add in the did:peer spec that when doing the base64 encoding the padding characters MUST be stripped to be compliant with the DID Spec, since it is not a valid character in a DID. And we need to correct the example (it’s got the padding, correct?).

Of course, we could get super weird and require that the padding character be converted to pct-encoded, but that just feels like poking ourselves in the eye.

I think updating the did:spec (as above) is the right thing to do. And make sure we get corrections into the all of the did:peer:2 creator libraries that we know of to fix this.

dbluhm commented 1 year ago

Given https://github.com/decentralized-identity/peer-did-method-spec/pull/52 has been merged, are we good to close this PR and the related issue #61?

swcurran commented 1 year ago

Yup