Allow set prv.key over LoRa, clear ACL and validate key

[oltaco]

Allows setting the private key for repeaters, room servers and sensors over LoRa via the set prv.key CLI command. The ACL is cleared when the key changes to remove invalid shared secrets, and the key is validated before being applied.

Note: After changing the private key, the device will reply with the new public key before rebooting.

Changes:

• Refactored ClientACL to store _fs reference
• Refactored CommonCLI to accept ClientACL& for use by set prv.key
• Added ClientACL::clear() function
• Added LocalIdentity::validatePrivateKey() function
• set prv.key now clears the ACL and is available over the mesh

Tested on:

• RAK4631: Repeater, Room Server, Sensor, Companion (companion now validates private key before setting)
• Heltec V3: Repeater

[liamcottle]

Wondering if we should update the identity file, but not the in memory identity until a reboot?

This would allow the user to replay the request until they get an OK response.
If the user sends the request over multiple hops, and doesn't get a reply, but it was applied, they'd not know.

Also potentially have the response include the new public key so the app could auto add as a new contact?

I'm currently away, so just throwing out quick notes :)

[ripplebiz]

Wondering if we should update the identity file, but not the in memory identity until a reboot?

This would allow the user to replay the request until they get an OK response. If the user sends the request over multiple hops, and doesn't get a reply, but it was applied, they'd not know.

Also potentially have the response include the new public key so the app could auto add as a new contact?

I'm currently away, so just throwing out quick notes :)

Yes, I like that idea

[oltaco]

Sounds good to me, I'll whip it up soon when I'm back at the desk.

[oltaco]

@ripplebiz @liamcottle It's not as straightforward as I thought, the problem is that the key is updated on disk and in memory at the same time by MyMesh::saveIdentity(). It would mean changing that to allow the option to only update on disk, and boot time logic to detect (somehow) that the key has changed and that we need to clear the ACL now.

Is the outgoing advert with the new key not enough of a signal that it worked? I'll have a think about what other options there might be, maybe a flag to delay the rekeying until after the reply is sent?

[ripplebiz]

@ripplebiz @liamcottle It's not as straightforward as I thought, the problem is that the key is updated on disk and in memory at the same time by MyMesh::saveIdentity(). It would mean changing that to allow the option to only update on disk, and boot time logic to detect (somehow) that the key has changed and that we need to clear the ACL now.

Is the outgoing advert with the new key not enough of a signal that it worked? I'll have a think about what other options there might be, maybe a flag to delay the rekeying until after the reply is sent?

You could just remove the:
self_id = new_id;

in saveIdentity(). Looks like CommonCLI is only one using this method. So could change the method to just save to file, and not the 'live' identity

[oltaco]

Ok, this is ready to go. One question though, do we want to keep the refactoring of ClientACL and passing it to CommonCLI?

It's not necessary anymore because the acl.clear() call has moved to MyMesh/SensorMesh but if we keep it then we could move the setperm and get acl handling from MyMesh::handleCommand() into CommonCLI::handleCommand().

[liamcottle]

Just had another quick skim, looks like it would still reboot right away, and we wouldn't be able to replay the request over and over to get a reply. I feel this will be problematic for the app side. I personally think we should force a manual reboot after changing identity file. Similar to how it's done for radio settings changes for repeaters. You change them, you get confirmation (maybe after multiple attempts), then you manually reboot to load those new settings.

Currently if I we send a request, the board might accept the command, rekey, send ok response, then reboot. But we may never get the reply due to multi hop issues, and maybe never get the flood advert on boot either.

I think we need to keep in mind the potential for nodes to have boot adverts disabled. The app ideally needs to get that new public key from the response, so being able to replay is important. Otherwise the app side would need to implement the ed25519 code as well, to be able to derive the public key from the private key. Some might argue you don't need the public key in the response if you just provided the private key...

I was actually thinking about this for the app, where the app could allow the user to generate privates until they find a selected public key prefix they like. Similar to the online tools, but since it's embedded in the app, it would be useable offline.

This leads me to another fun side quest... I want to bring up this older issue and PR:

• [Feature Request] Preserve and use the ed25519 seed to improve compatibility with modern implementations #1044
• Preserve and make the ed25519 key accessible via the CLI #1045

We currently throw away the seed when generating the private key, and once it's been put through sha512, it's impossible to retrieve the seed in the future. Most modern libs use the 32 byte seed value, instead of the 64 byte sha512 expanded private key version. Which means interacting with identity keys is a bit painful outside of MeshCore.

I wonder if we are working on these new CLI commands, we might as well tackle this issue about the seed at the same time. Then we could just send a 32 byte private seed over the CLI, instead of the 64 byte expanded private key. If the user is performing a rekey, this would force them into the new format that's compatible across mainstream encryption libraries.

Just a few thoughts to ponder :)

[oltaco]

Yep I get where you're coming from, the problem is still how do you know to clear the ACL on boot because the key has changed? You could write a file of course but I was really trying to avoid touching the filesystem 😂

I don't really know why we care about the reflection of the pubkey, because to me the fact that you're changing the private key implies that you're asking for a specific pubkey prefix so you already know what the pubkey will be because you generated for that reason.

I don't have any opinion on keeping the seed, but I do see what you're saying about forcing people onto the new format by only allowing set prv.key via seed. I assume the seed alone is enough to guarantee a prefix?
After a quick google I'm on board with pushing for keeping the seed instead of the expanded private key. Obviously a bit painful but it seems worth it.

[liamcottle]

I don't really know why we care about the reflection of the pubkey, because to me the fact that you're changing the private key implies that you're asking for a specific pubkey prefix so you already know what the pubkey will be because you generated for that reason.

Fair enough :) I still think having the ability to receive an OK response to confirm it actually happened is good. But I can probably live with it if it's too annoying to implement... I'm just thinking about the flow in the app side. There needs to be a way to tell the user it was successful. Otherwise they'll sit there clicking the button, and never get a reply, because it was successful, and now the keys have changed.

Yep I get where you're coming from, the problem is still how do you know to clear the ACL on boot because the key has changed? You could write a file of course but I was really trying to avoid touching the filesystem 😂

What about regenerating the shared_secret in the ACL list on boot instead of nuking it entirely when identity file changes?

That way if you had manually added a bunch of users to ACL without them needing a password, you wouldn't have to add them all back again. I guess the trade off here is now you need to do this on every reboot... Maybe could just check if one is invalid, and regenerate all shared secrets in that case... Similar to how resetContacts is called on companion boot to regenerate shared secrets for existing contacts.

I don't have any opinion on keeping the seed, but I do see what you're saying about forcing people onto the new format by only allowing set prv.key via seed. I assume the seed alone is enough to guarantee a prefix?

Yeah, ed25519 seeds are 32 bytes, and public keys are also 32 bytes. The seed gets sha512 hashed, along with a few other adjustments, to create the 64 byte expanded private key, which is what we save/use in MeshCore.

We only really need to save the 32 byte seed, but we actually compute the 64 byte expanded key, and throw away the original seed. Migrating to a new method that allows you to just pass in the 32 byte seed would mean we store less data in file system, we send less data over LoRa, and it becomes compatible with mainstream encryption libs.

The trade off here, is you wouldn't be able to import older identities, since no one has the seed for them. Ideally we move to newer standards, and this might be the way to go about it.

[oltaco]

What about regenerating the shared_secret in the ACL list on boot instead of nuking it entirely when identity file changes?

That way if you had manually added a bunch of users to ACL without them needing a password, you wouldn't have to add them all back again. I guess the trade off here is now you need to do this on every reboot... Maybe could just check if one is invalid, and regenerate all shared secrets in that case... Similar to how resetContacts is called on companion boot to regenerate shared secrets for existing contact

Unfortunately I don't think we can recompute shared_secret because the ClientACL struct doesn't keep the pubkey, only the secret. Yeah maybe recomputing shared_secret is the way to go, repeaters don't boot that often anyway so what's a few extra seconds?

Separately these days companions just compute shared_secret on demand to save on boot time :)

[oltaco]

Ok, following discussion with @liamcottle I've made the changes, now upon set prv.key the node will reply with
-> OK, reboot to apply! New pubkey: 1E56EA39BFDF8F5D9A3AB7EA3BE058ADA3A7C65F9FDB52FD5B25985BA972120B
and the ClientACL shared_keys will be recalculated on boot.
I've tested to make sure this is working properly using the request stats function from meshcore-cli.

I have left the CommonCLI/ClientACL refactor that allows access to ClientACL functions from CommonCLI in place, in case we want to move those command handlers out of MyMesh and into CommonCLI, what do you think @ripplebiz?

I also left the now unused ClientACL::clear() function in just in case we want to wire that up to a command to easily clear out the ACL in one go, but it can be just as easily removed if that's desired.

Once this is merged I think we should seriously consider moving towards seed instead of expanded private key as @liamcottle suggested.

[liamcottle]

Looking good thanks!

I'll load this on a few devices and give it a proper test.

Keeping ClientACL::clear() is a good idea, having the ability to nuke that over a CLI command at some stage might be useful. e.g: acl.clear. Someone previously requested that changing admin password would clear the ACL, however I would prefer if that was an explicit action. Potentially with the option to not remove the user performing the clear from ACL.

• (Enhancement) changing the password for read/write or admin operations should force re-auth #941

Keen to look at adding a new set prv.seed or similar command in a seperate PR that will allow for importing an identity from a 32 byte seed. This could automatically calculate the other internal values we use, so the identity saved to disk remains in the same format.

[liamcottle]

Tested rekeying of repeaters via companion over LoRa. All is working.

Ran erase and reboot commands on the repeater via USB CLI multiple times to get a collection of unique private keys:

Then sent set prv.key <hex> via the mobile app CLI console for the repeater, received the new public key in reply, confirmed it matches with the expected public key, then did a reboot. Once repeater rebooted I was able to login via the new public key. ACL was preserved.

Looks good to me!