refactor: #1819 signature and secret renamings #4179
Conversation
etorreborre
force-pushed
the
etorreborre/refactor/#1819-signature-secret-renamings
branch
from
b00b098 to
ab72cf7
Compare
this is shows that this is a more specific kind of signature than the Signature type in types.rs
in order to avoid clashes I had to temporarily have a PrivateKeyAttributes to support a key label a further commit will separate the label out
etorreborre
force-pushed
the
etorreborre/refactor/#1819-signature-secret-renamings
branch
from
ab72cf7 to
5c399b8
Compare
etorreborre
force-pushed
the
etorreborre/refactor/#1819-signature-secret-renamings
branch
from
5c399b8 to
6812066
Compare
| #[derive(Serialize, Deserialize, Clone, Zeroize, Encode, Decode)] | ||
| #[zeroize(drop)] | ||
| #[cbor(transparent)] | ||
| pub struct SecretKey(#[n(0)] SecretKeyVec); | ||
| pub struct PrivateKey(#[n(0)] PrivateKeyVec); |
There was a problem hiding this comment.
Could you please elaborate what is the reasoning on that renaming?
There was a problem hiding this comment.
That's the one presented in the description, the fact that the SecretVault API talks about secrets but return key ids. It would be better to make the naming consistent. What would you propose to solve this discrepancy?
There was a problem hiding this comment.
Oh, I thought renaming SecretVault -> KeyVault and SecretKey -> PrivateKey are two separate things, are they related?
| #[derive(Serialize, Deserialize, Copy, Clone, Debug, Encode, Decode, Eq, PartialEq, Zeroize)] | ||
| #[rustfmt::skip] | ||
| #[cbor(index_only)] | ||
| pub enum SecretType { | ||
| pub enum KeyType { |
There was a problem hiding this comment.
I'm not sure renaming Secret -> Key is technically correct. Key is more narrow term, for example I don't think a Buffer with sensitive data can be called a Key
There was a problem hiding this comment.
Yes and that might be a hint that we should separate this data from the rest. For example in hasher_impl.rs we try to retrieve the salt and fail at runtime if it doesn't have the right type. Maybe this is an indication that this kind of data (that is always Ephemeral as far as I understand) should be stored separately inside a Vault.
There was a problem hiding this comment.
Agree, having different types for these things may be a great idea, but before we have that I'm not sure this renaming makes sense to me
| use ockam_core::{Encodable, Result}; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| /// Key change data creation | ||
| #[derive(Serialize, Deserialize, Debug, Clone)] | ||
| pub struct CreateKeyChangeData { | ||
| prev_change_id: ChangeIdentifier, | ||
| key_attributes: PrivateKeyAttributes, | ||
| key_label: String, |
There was a problem hiding this comment.
That would probably break backwards compatibility and therefore break Ockam Orchestrator
There was a problem hiding this comment.
Interesting, that raises 2 questions in my mind:
- do we have tests to check the compatibility with Orchestrator? Because they should have been broken with that change
- we should probably have a data structure that is purely used to communicate with the Orchestrator. Are we sending the full structure of changes as-is to the Orchestrator? Where is that done?
There was a problem hiding this comment.
Change history is overly complex, we can break backward compatibility here once but let's do it soon and in one sweep.
There was a problem hiding this comment.
We use rust sidecar for Identity implementation, so there are no second implementation and no second structure for Identity. The problem is that we have existing persistent identities in our system (e.g. Orchestrator has its Identity stored somewhere in its db), which probably won't deserialize because of the changed structure. Unfortunately, there is no backwards compatibility test
There was a problem hiding this comment.
Can't we create one right now by hardcoding a fully populated encoded Identity in the test?
| @@ -244,6 +244,15 @@ impl KeyAttributes { | |||
| length, | |||
| } | |||
| } | |||
|
|
|||
| /// Create default key attributes | |||
| pub fn default_attributes() -> Self { | |||
There was a problem hiding this comment.
I think being explicit when you generate a secret could be very useful
There was a problem hiding this comment.
Do you mean that we should actually not have such a function?
|
I'm closing this PR for now. In particular since it highlighted the need to do some preparation work on:
|
mrinalwadhwa
deleted the
etorreborre/refactor/#1819-signature-secret-renamings
branch
The purpose of this PR is to get a more consistent terminology:
A
Signatureis a just a list of bytes transferred over the wireAn
IdentityChangeSignatureis a signature authenticating anIdentityChange. This gives it more context and avoid the naming clashes identified in Module structure of the Rust libraries needs cleaning up #1819the
SecretVaultinterface has been renamed toKeyVaultsince it is dealing with keys and returningKeyIds. So instead of "generating a secret and getting back a key id" we "generate a key and get back a key id"this change has been extended to
KeyAttributes(instead ofSecretAttributesfor the attributes of a key)the above renaming clashed with the existing
KeyAttributestruct (containing both a key label and some key attributes). That structure is actually not very useful since a key label is used independently from the key attributes: