-
Notifications
You must be signed in to change notification settings - Fork 83
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] Multibase public key decoding incorrectly #1279
Conversation
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
fixed up some numalgo 2 tests. IMO the previous test behaviour highlights how it was confusing/inconsistent to return key_decoded with the prefix in the multibase case. |
So, I'll admit I am a little weaker in this area specifically, so please correct any assumptions or knowledge gaps. I think it makes sense to return the actual 32 bytes we're looking for in the majority of cases, as the 2 leading bytes are only there really for transport within the DID Doc (so that it can be explicit vs contextually determined, which is the purpose behind the Multibase Encoding Scheme as I understand it). However, I could maybe see there being use cases where it's better to get the 'raw' value present in the DID Doc and pass that along still containing the additional context of the encoding scheme and multicodec (such as: an API that just returns the publicKeyMultibase and it's removed from the context of the original DIDDoc, so the end developer could use the the contextual info). So, if that's even a valid concern, is there a way to get the 'raw' value (so including the first two bytes)? Baring that singular question, the PR looks good. 👍 |
Thanks for your review @JamesKEbert, yes there's some ways to do this. So in the case that you are holding the
key_field: PublicKeyField
let PublicKeyField::Multibase { public_key_multibase } = key_field else { panic!("") };
// public_key_multibase is the original string in my experience of using this part of the crate, i kind the first option of using the I actually don't really need the fix in this PR anymore, bcus i realised |
Cool, glad to hear there's routes if someone would want to get access to the keytype + key bytes.
Agreed |
* impl fix and remove misleading api Signed-off-by: George Mulhearn <gmulhearn@anonyome.com> * fix up numalgo 2 tests Signed-off-by: George Mulhearn <gmulhearn@anonyome.com> --------- Signed-off-by: George Mulhearn <gmulhearn@anonyome.com> Co-authored-by: George Mulhearn <gmulhearn@anonyome.com>
* impl fix and remove misleading api Signed-off-by: George Mulhearn <gmulhearn@anonyome.com> * fix up numalgo 2 tests Signed-off-by: George Mulhearn <gmulhearn@anonyome.com> --------- Signed-off-by: George Mulhearn <gmulhearn@anonyome.com> Co-authored-by: George Mulhearn <gmulhearn@anonyome.com>
The PublicKeyField::key_decoded (i.e. "give me the raw public key bytes") was returning the wrong bytes for the
publicKeyMultibase
verification method field. Particularly, it was returning the [multikey-type + public key bytes] (i.e. the bytes prefixed with the key type), whereas we want it to just return the public key bytes (to align with what the other PublicKeyField variants decode to).The unit tests were previously self-fulfilling in testing this, so it was not spotted. An easy way to spot the error is looking at what the
PUBLIC_KEY_HEX
test vector used to be; a 34 byte long array.. when's the last time you've seen a public key that is 34 bytes long! (it's because we were leaving the 2 byte multikey type prefix in there[236, 1]
(a.k.a. 0xEC: https://github.com/multiformats/multicodec/blob/master/table.csv#L97)).I also removed the key_decoded solution for JWK, as it was incorrect too; it was just returning the JSON serialized JWK as if it is the public key bytes. IMO the
to_vec
method of theJsonWebKey
is misleading for this reason, so i removed it (i was expecting it to return the public key bytes..).