Support working with ed25519 seeds in addition to raw keypairs.#1055
Support working with ed25519 seeds in addition to raw keypairs.#1055aqua wants to merge 3 commits intomeshcore-dev:devfrom
Conversation
Our ed25519 library uses a representation of its key pair that is largely incompatible with modern implementations, which mostly work with the original 32-byte seed; Peters' impentation represents the private key as the clamped sha512 of the seed. This change: - preserves the original seed when generating keys - adds CLI commands to obtain the seed via `get prv.seed`, under the same conditions as `get prv.key` is allowed - adds support for `set prv.key` to supply a seed, in which case the keypair will be re-generated from it. This is mostly to enable external key management using modern libraries, but could also be of use on devices where we don't have a trustworthy entropy source. I split Identity::writeTo(uint8_t*,size_t) into explicit forms for the thing being written; the original implementation wrote a different thing depending on the length, which would be ambiguous between pubkey and seed and cumbersome if it tried to return all three in one long buffer. Identity::readFrom() did not have that ambiguity problem because keys can't be set from pubkey alone, though it might be preferable to split readFrom() up as well and not use magic length values.
…d25519-seed-dev
446564
left a comment
446564
left a comment
There was a problem hiding this comment.
LGTM
tested on xiao nrf repeater, getting and setting work as advertised. all other functions remain operational.
| } else { | ||
| strcpy(reply, "Error, bad key"); | ||
| } | ||
| } else if (sender_timestamp == 0 && memcmp(config, "prv.seed ", 9) == 0) { // from serial command line only |
There was a problem hiding this comment.
Not too long ago, we allowed updating the private key via LoRa with set prv.key <hex>, so we might as well allow setting the private key from the seed via LoRa with set prv.seed <hex> as well.
For now, I believe the fetching of the private key will still remain serial only.
| } | ||
| } else if (sender_timestamp == 0 && memcmp(config, "prv.seed ", 9) == 0) { // from serial command line only | ||
| uint8_t seed[SEED_SIZE]; | ||
| bool success = mesh::Utils::fromHex(seed, SEED_SIZE, &config[9]); |
There was a problem hiding this comment.
Since there's no length validation for the provided seed, is there any issue with providing a seed that's too short or too long? I guess the remainder would just be all zeros...
There was a problem hiding this comment.
Utils::fromHex() does a check that strlen(src_hex)==dest_size*2, so it should give an error with a seed that's too short or too long (I'd test but I'm across an ocean from my nearest spare repeater until the weekend.)
| sprintf(reply, "> %s", tmp); | ||
| } else if (sender_timestamp == 0 && memcmp(config, "prv.seed", 8) == 0) { // from serial command line only | ||
| uint8_t seed[SEED_SIZE]; | ||
| auto len = _callbacks->getSelfId().writeSeedTo(seed, SEED_SIZE); |
There was a problem hiding this comment.
for devices that update firmware, but keep their existing identity, I'd assume this will just provide an output of full zeros. Probably fine, as you could check for full zeros to know there's no seed available. But wondering if it should return an error message instead...
There was a problem hiding this comment.
Assuming you mean here devices that were seeded before this code: yes, LocalIdentity::readFrom() zeroes the seed and then attempts to read over it if the seed is in the identity store, so if the identity were stored from an older revision it'll stay zeroed and this will output a zero string. I suppose returning an error would be more helpful but we should have something in the docs to say more than the firmware itself can.
@446564 thanks for the review. I've added some extra comments. |
3 participants
Replaces pull #1046, which I accidentally closed by being bad at github. Addresses #1044.
Our ed25519 library uses a representation of its key pair that is largely incompatible with modern implementations, which mostly work with the original 32-byte seed; Peters' impentation represents the private key as the clamped sha512 of the seed.
This change:
get prv.seed, under the same conditions asget prv.keyis allowedset prv.keyto supply a seed, in which case the keypair will be re-generated from it. This is mostly to enable external key management using modern libraries, but could also be of use on devices where we don't have a trustworthy entropy source.I split Identity::writeTo(uint8_t*,size_t) into explicit forms for the thing being written; the original implementation wrote a different thing depending on the length, which would be ambiguous between pubkey and seed and cumbersome if it tried to return all three in one long buffer. Identity::readFrom() did not have that ambiguity problem because keys can't be set from pubkey alone, though it might be preferable to split readFrom() up as well and not use magic length values.