servalproject / serval-dna

The Serval Project's core daemon that implements Distributed Numbering Architecture (DNA), MDP, VoMP, Rhizome, MeshMS, etc.
http://servalproject.org
Other
170 stars 81 forks source link

Identity did & name length #131

Closed CryptoCopter closed 6 years ago

CryptoCopter commented 6 years ago

When you try to set the name and/or did of an identity, the values are silently truncated at 63 and 31 bytes respectively.

It would be nice if you could produce an error when the input size exceeds these limits rather than just silently dropping letters. If that is not possible, then it would nice if this behaviour could at least be documented.

On a realted note: the 5-digit minimum for DIDs is actually not enforced anywhere.

quixotique commented 6 years ago

I agree that there should be an error instead of truncation. There should also be separate commands for testing whether a DID or Name is valid, that simply produce a non-zero exit status if not, to help scripts and other CLI clients avoid attempting to set invalid values.

quixotique commented 6 years ago

The 5-char minimum length is enforced in dataformats.c:279 which is used by the keyring set did command. Do you have a test case that shows it is not enforced?

CryptoCopter commented 6 years ago

I see, I was not aware of that. I encountered theses issues when testing the REST interface and using the GET /restful/keyring/SID/set endpoint you can set the DID to fewer than 5 digits.

Speaking of that - why does this endpoint use a GET-request - shouldn't that be POST since it's manipulating remote data?

quixotique commented 6 years ago

Indeed, the Keyring REST API does not validate the did parameter, but the CLI API does. I will post a fix shortly.

Also, neither API validates the name parameter, so characters could sneak through that break the CLI output, such as LF or CR. I will also fix this.

You are correct that the REST keyring add and set operations should be POST not GET, in order to comply with the HTTP protocol, which states that GET should be idempotent and safe. Performing several identical GET keyring add operations will add one new identity per request, which is definitely not idempotent. However, several identical GET keyring set operations in a row are idempotent -- the first one will make the change and all the subsequent ones will not change anything, so it is not so bad.

I will consider modifying the REST API accordingly, but I must survey existing clients to ensure that this will not break anything.

CryptoCopter commented 6 years ago

Do you have an ETA for when the changes of 5eb19f1a16394c581e286804d602f9ea32eff19c will be merged into the main development-branch?

lakeman commented 6 years ago

Looks like 5eb19f1 is already in development, and is unrelated to this issue?

CryptoCopter commented 6 years ago

oops, I'm sorry, I copied the wrong hash - 52d5ee55ba4dd76b7baae20b4f08c1cbd8338b89 fixes this but appears to only exist on the 'ios' branch