From dc616d9e07ae878f6df558234681ed3c326a1844 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 18 Apr 2023 01:45:12 +0100 Subject: [PATCH 01/13] dev docs: Add key manager API sketch. This is the first draft of the key manager API. I don't expect this to be the final version of the API, and I'm sure there are plenty of improvements to be made. This is mostly a request for comments. Closes #834 Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 239 ++++++++++++++++++++++++++++++++ 1 file changed, 239 insertions(+) create mode 100644 doc/dev/notes/key-management.md diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md new file mode 100644 index 000000000..690d656ee --- /dev/null +++ b/doc/dev/notes/key-management.md @@ -0,0 +1,239 @@ +# Key management backend + +## Motivation +Arti will need to be able to manage various types of keys, including: + * HS client authorization keys (`KS_hsd_desc_enc`, `KS_hsc_intro_auth`) + * HS service keys (`KS_hs_id`, `KS_hs_desc_sign`) + * relay keys + * dirauth keys + * ... + +This document describes a possible design for a key manager that can read, add, +or remove keys from persistent storage. It is based on some ideas proposed by +@Diziet. + +See also: [#728] + +## Usage example + +```rust + let key_id: HsClientSecretKeyIdentity = + (LocalUserIdentity, HsId, HsClientSecretKeySpecifier).into(); + let sk: Option = keymgr.get::(key_id)?; +``` + +## Proposed configuration changes + +```toml +# Key manager options +[keys] +# The root of the key store, if the key store type is "arti" or "ctor". +keystore_dir = "" +# The key store type. +# If set to "arti", we will use Arti's disk key storage format. +# If set to "ctor", we will use the storage format used by C Tor. +# If set to "hsm", we will store the keys on a smartcard (TODO hs: this will +# require further config and API changes). +keystore_kind = "arti" +``` + +## Key identities + +A key by itself is not very useful. In order for Arti to be able to use it, it +needs to know what the key is supposed to be used for. As such, we need to store +some metadata alongside each key. + +C Tor achieves this using a passwd-style format (see the `CLIENT AUTHORIZATION` +section of the `tor` manpage), which stores keys and their associated metadata +in the same file. + +In addition to the C Tor key format, Arti will also be able to store keys +in [OpenSSH format]. The metadata will be serialized and stored in the `comment` +string of the key. The key metadata we're interested in is: +* `arti_path`: the location of the key in the Arti key store. +* `ctor_path`: the location of the key in the C Tor key store. +* `hsm_slot`: the slot where the key is located on the smartcard +* a string that includes the key name (from the spec), as well as information + about that specific instance of the key: + ``` + { + "type": "HsClientSecretKeyIdentity", + "key_name": "k_hsc_desc_enc", + "service": "foo.onion", + "user_identity": "wombat" + } + ``` + This string will be included in the `comment` section of any newly generated + key. It is also expected to be present (and in the correct format) for all + existing keys in an Arti key store. + +Keys make their metadata available via the `KeyIdentity` trait. + +### Metadata format + +TODO: decide what the metadata is serialized to + +### The `KeyIdentity` trait + +```rust + /// The path of a key in the Arti key store. + pub struct ArtiPath(PathBuf); + + /// The path of a key in the C Tor key store. + pub struct CTorPath(PathBuf); + + /// The location of a key on a smartcard + pub struct HsmLocation { + /// The slot where the key is located. + slot: usize + // TODO hs: what other fields does this need? + } + + /// Information about where a particular key could be stored. + pub trait KeyIdentity: Serialize + Deserialize { + type CTorKey; + + /// The location of the key in the Arti key store. + fn arti_path(&self) -> ArtiPath; + + /// The location of the key in the C Tor key store (if supported). + fn ctor_path(&self) -> Option; + + /// The slot where the key is located on the smartcard. + fn hsm_slot(&self) -> Option; + + /// The string representation of this key identity; + fn string_rep(self) -> String { + // TODO hs: serialize as what? + serde_json::to_string(&self) + } + + // TODO hs: other methods? + } + + /// The result of deserializing the comment field of an openSSH key. + // TODO hs: we might not actually need this + #[derive(Serialize, Deserialize)] + enum KeyIdentityResult { + /// A known key identity. + Key(K), + /// An unsupported key type. + Unknown(String), + } + + /// An identifier for a client/relay/... + pub enum LocalUserIdentity(String); + + struct HsClientSecretKeyIdentity { ... } + + + impl KeyIdentity for HsClientSecretKeyIdentity { + ... + } + +``` + +## Proposed key manager API + +```rust +pub trait EncodableKey { + /// The underlying key type. + fn key_type() -> KeyType + where + Self: Sized; +} + +// Implement `EncodableKey` for all the key types we wish to support. +impl EncodableKey for ed25519::SecretKey { + ... +} + +... + +enum KeyStoreKind { + /// A C Tor-style key store. + CTor(PathBuf), + /// An Arti key store that uses OpenSSH key format. + Arti(PathBuf), + /// An HSM key store (TODO hs: figure out how this is going to work). + Hsm, +} + +/// The key manager. +struct KeyMgr { + /// The type of persistent store. + store_kind: KeyStoreKind, + ... +} + +impl KeyMgr { + /// Read a key from the key store, attempting to deserialize it as `K`. + pub fn get( + &self, + key_id: &dyn KeyIdentity + ) -> Result> { + match self.store_kind { + KS_hs_ipt_sidKeyStoreKind::Arti(keystore) => { + let key_path = keystore.join(key_id.arti_path()); + if !key_path.exists() { + // Not found + return Ok(None); + } + + let raw_key = fs::read(key_path)?; + // TODO hs: compare the comment field of the key with + // key_id.string_rep(), returning an error (or maybe `Ok(None)`) if + // they don't match + K::arti_decode(raw_key, key_id).map(Some) + } + KeyStoreKind::CTor(keystore) => { + let key_path = keystore.join(key_id.ctor_path()); + if !key_path.exists() { + // Not found + return Ok(None); + } + + let raw_key = fs::read(key_path)?; + K::ctor_decode(raw_key, key_id).map(Some) + } + KeyStoreKind::Hsm => unimplemented!(), + } + } + + /// Write `key` to the key store. + pub fn insert( + &self, + key_id: &dyn KeyIdentity, + key: K, + ) -> Result<()> { + for store in &self.key_store { + if store.has_key_bundle(key_id) { + return store.insert(&key, key_id, K::key_type()); + } + KeyStoreKind::CTor(keystore) => { + let key_path = keystore.join(key_id.ctor_path()); + let key = K::arti_encode(key_id)?; + + fs::write(key_path, key) + } + KeyStoreKind::Hsm => unimplemented!(), + } + } + + pub fn remove( + &self, + key_id: &dyn KeyIdentity, + ) -> Result<()> { + unimplemented!() + } +} +``` + +### Outstanding questions + +1. Do we want the key manager to be able to load each key from a different + storage medium (i.e. do not fix a specific `store_type` at configuration + time)? If so, the API described here will need to be revised. + +[#728]: https://gitlab.torproject.org/tpo/core/arti/-/issues/728 +[OpenSSH format]: https://coolaj86.com/articles/the-openssh-private-key-format/ From fb6d5dc0d9309907451cad9903f273d76fb5b39e Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 18 Apr 2023 16:05:01 +0100 Subject: [PATCH 02/13] dev docs: Add some impls for `LocalUserIdentity`. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 690d656ee..415c36a7e 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -122,7 +122,10 @@ TODO: decide what the metadata is serialized to } /// An identifier for a client/relay/... - pub enum LocalUserIdentity(String); + #[derive(AsRef, Into, ...)] + pub struct LocalUserIdentity(String); + + impl FromStr for LocalUserIdentity { /* check syntax rules */ } struct HsClientSecretKeyIdentity { ... } From 28bac87d9674a092b051ba46acd2bb8166b08879 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Wed, 19 Apr 2023 13:29:37 +0100 Subject: [PATCH 03/13] dev docs: Allow multiple key stores to be in use at the same time. The key manager needs to be flexible enough to support loading keys from one of several key stores. This is because when we add support for smart cards, users will want to be able to store some keys on the smart card, and others in one of the disk key stores (for example). Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 110 ++++++++++++++++++++++++++------ 1 file changed, 92 insertions(+), 18 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 415c36a7e..405b24d23 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -22,19 +22,101 @@ See also: [#728] let sk: Option = keymgr.get::(key_id)?; ``` +## Key stores + +The key manager is an interface to one or more key stores. + +Supported key stores: +* C Tor key store: an on-disk store that is backwards-compatible with C Tor (new + keys are stored in the format used by C Tor, and any existing keys are + expected to be in this format too). +* Arti key store: an on-disk store that stores keys in OpenSSH key + format. + +In the future we plan to also support HSM-based key stores. + +## Key passphrases + +OpenSSH keys can have passphrases. While the first version of the key manager +won't be able to handle such keys, we will add passphrase support at some point +in the future. + ## Proposed configuration changes +We introduce a new `[keys]` section for configuring the key stores. The +`[keys.permissions]` section specifies the permissions all top-level key store +directories are expected to have. It serves a similar purpose to +`[storage.permissions]`. + +Initially, it will only be possible to configure two disk-backed key stores: the +Arti key store via the `[keys.arti_store]` section, and the C Tor key store via +the `[keys.ctor_store]` section. Future versions will be able to support the +configuration of arbitrary key store implementations. + +The order of the key store sections is important, because keys are looked up in +each of the configured key stores, in the order they are specified in the +config. For example, if the Arti key store comes before the C Tor key store in +the config, when prompted to retrieve a key, the key manager will search +for the key in the Arti key store before checking the C Tor one: + ```toml -# Key manager options +[keys.arti_store] +... + +[keys.ctor_store] +... +``` + +Note that both key stores are optional. It is possible to run Arti without +configuring a key store (for example, when running Arti as a client). + +TODO hs: pick reasonable default values for the various top-level key store +directories (`root_dir`, `client_dir`, `key_dir`). + +```toml +# Key store options [keys] -# The root of the key store, if the key store type is "arti" or "ctor". -keystore_dir = "" -# The key store type. -# If set to "arti", we will use Arti's disk key storage format. -# If set to "ctor", we will use the storage format used by C Tor. -# If set to "hsm", we will store the keys on a smartcard (TODO hs: this will -# require further config and API changes). -keystore_kind = "arti" + +# Describe the filesystem permissions to enforce. +[keys.permissions] +# If set to true, we ignore all filesystem permissions. +#dangerously_trust_everyone = false + +# What user (if any) is trusted to own files and directories? ":current" means +# to trust the current user. +#trust_user = ":current" + +# What group (if any) is trusted to have read/write access to files and +# directories? ":selfnamed" means to trust the group with the same name as the +# current user, if that user is a member. +#trust_group = ":username" + +# If set, gives a path prefix that will always be trusted. For example, if this +# option is set to "/home/", and we are checking "/home/username/.cache", then +# we always accept the permissions on "/" and "/home", but we check the +# permissions on "/home/username" and "/home/username/.cache". +# +# (This is not the default.) +# +# ignore_prefix = "/home/" +#ignore_prefix = "" + +# The Arti key store. +[keys.arti_store] +# The root of the key store. All keys are stored somewhere in the `root_dir` +# heirarchy +root_dir = "" + +# The C Tor key store. +[keys.ctor_store] +# The client authorization key directory (if running Arti as a client). +# +# This corresponds to C Tor's ClientOnionAuthDir option. +client_dir = "" +# The key directory. +# +# This corresponds to C Tor's KeyDirectory option. +key_dir = "" ``` ## Key identities @@ -48,8 +130,7 @@ section of the `tor` manpage), which stores keys and their associated metadata in the same file. In addition to the C Tor key format, Arti will also be able to store keys -in [OpenSSH format]. The metadata will be serialized and stored in the `comment` -string of the key. The key metadata we're interested in is: +in OpenSSH format. The key metadata we're interested in is: * `arti_path`: the location of the key in the Arti key store. * `ctor_path`: the location of the key in the C Tor key store. * `hsm_slot`: the slot where the key is located on the smartcard @@ -232,11 +313,4 @@ impl KeyMgr { } ``` -### Outstanding questions - -1. Do we want the key manager to be able to load each key from a different - storage medium (i.e. do not fix a specific `store_type` at configuration - time)? If so, the API described here will need to be revised. - [#728]: https://gitlab.torproject.org/tpo/core/arti/-/issues/728 -[OpenSSH format]: https://coolaj86.com/articles/the-openssh-private-key-format/ From 49ff17901b37c283e53d4783300d9c861aa5335a Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Wed, 19 Apr 2023 17:05:21 +0100 Subject: [PATCH 04/13] dev docs: clarify what a "key identity" is. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 124 ++++++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 30 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 405b24d23..8215884bf 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -122,43 +122,107 @@ key_dir = "" ## Key identities A key by itself is not very useful. In order for Arti to be able to use it, it -needs to know what the key is supposed to be used for. As such, we need to store -some metadata alongside each key. +needs to also know the _role_ of the key (i.e., what that key is supposed to be +used for), among other things. As such, we need to store some metadata alongside +each key. -C Tor achieves this using a passwd-style format (see the `CLIENT AUTHORIZATION` -section of the `tor` manpage), which stores keys and their associated metadata -in the same file. +For client authorization keys, C Tor achieves this using a passwd-style format +(see the `CLIENT AUTHORIZATION` section of the `tor` manpage), which stores keys +and their associated metadata in the same file. Other keys don't have other +metadata than their _role_ (i.e. "Ed25519 permanent identity key", "medium-term +Ed25519 signing key", etc.), which can be unambiguously determined by looking at +the path/file name of the key file (e.g. the role of +`/ed25519_master_id_private_key` is "Ed25519 permanent identity +key of a relay", `/private_key` is "the private key of +the hidden service whose data is stored at ``", etc.). -In addition to the C Tor key format, Arti will also be able to store keys -in OpenSSH format. The key metadata we're interested in is: -* `arti_path`: the location of the key in the Arti key store. -* `ctor_path`: the location of the key in the C Tor key store. -* `hsm_slot`: the slot where the key is located on the smartcard -* a string that includes the key name (from the spec), as well as information - about that specific instance of the key: - ``` - { - "type": "HsClientSecretKeyIdentity", - "key_name": "k_hsc_desc_enc", - "service": "foo.onion", - "user_identity": "wombat" - } - ``` - This string will be included in the `comment` section of any newly generated - key. It is also expected to be present (and in the correct format) for all - existing keys in an Arti key store. +For this reason, we introduce the concept of a "key identity" (specified for +each supported key type via the `KeyIdentity` trait). A "key identity" uniquely +identifies an instance of a type of key. From an implementation standpoint, +`KeyIdentity` implementers must specify: +* `arti_path`: the location of the key in the Arti key store. This also serves + as a unique identifier for a particular instance of a key. +* `ctor_path`: the location of the key in the C Tor key store (optional). -Keys make their metadata available via the `KeyIdentity` trait. +For the keys stored in the Arti key store (i.e. in the OpenSSH format), the +`KeyIdentity::arti_path()` is converted to a string and placed in the `coment` +field of the key. When retrieving keys from the Arti store, the key manager +compares the `comment` of the stored key with the `KeyIdentity::arti_path()` of +the requested key identity. If the values match, it retrieves the key. +Otherwise, it returns an error. -### Metadata format +To identify the path of a key in the Arti key store, the key manager prepends +`keys.arti_store.root_dir` to `KeyIdentity::arti_path()` and appends an +extension. -TODO: decide what the metadata is serialized to +For example, an Arti key store might have the following structure (note that +each path within the `keys.arti_store.root_dir` directory, minus the extension, +is the `arti_path` of a particular key): +``` + +├── alice # HS client identity "alice" +│   ├── foo.onion +│ │   ├── hsc_desc_enc # arti_path = "alice/foo.onion/hsc_desc_enc" +│ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ │ # descriptors of foo.onion") +│ │   └── hsc_intro_auth # arti_path = "alice/foo.onion/hsc_intro_auth" +│ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing +│ │ # signatures to prove to foo.onion she is authorized") +│ │ # Note: this is not implemented in C Tor +│   └── bar.onion +│    ├── hsc_desc_enc # arti_path = "alice/foo.onion/hsc_desc_enc" +│ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ # descriptors of bar.onion") +│    └── hsc_intro_auth # arti_path = "alice/bar.onion/hsc_intro_auth" +│ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing +│ # signatures to prove to bar.onion she is authorized") +├── bob # HS client identity "bob" +│   └── foo.onion +│    ├── hsc_desc_enc # arti_path = "bob/foo.onion/hsc_desc_enc" +│ │ # (HS client Bob's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ # descriptors of foo.onion") +│    └── hsc_intro_auth # arti_path = "bob/foo.onion/hsc_intro_auth" +│ # (HS client Bob's ed25519 hsc_intro_auth keypair for computing +│ # signatures to prove to foo.onion he is authorized") +│ # Note: this is not implemented in C Tor +│ +├── baz.onion # Hidden service baz.onion +│   ├── authorized_clients # The clients authorized to access baz.onion +│ │   └── dan +│ │ └── hsc_desc_enc # arti_path = "baz.onion/authorized_clients/dan/hsc_desc_enc.pub" +│ │ # (The public part of HS client Dan's x25519 hsc_desc_enc keypair for +│ │ # decrypting baz.onions descriptors) +│ │ +│ │ +│ │   +│   ├── hs_id # arti_path = "baz.onion/hs_id" (baz.onion's identity key) +│   └── hs_blind_id # arti_path = "baz.onion/hs_blind_id" (baz.onion's blinded identity key) +├── Carol # Relay Carol +│   └── ... +└── .... +``` + +### Comment field format + +TODO hs: decide what format to store the `arti_path` in. One option would be +to encode it as an email address: `@_artikeyid.arti.torproject.org`. +Another would be to simply store it as-is. ### The `KeyIdentity` trait ```rust /// The path of a key in the Arti key store. - pub struct ArtiPath(PathBuf); + /// + /// NOTE: There is a 1:1 mapping between a value that implements + /// `KeyIdentity` and its corresponding `ArtiPath`. + /// A `KeyIdentity` can be converted to an `ArtiPath`, + /// but the reverse conversion is not supported. + // + // TODO hs: restrict the character set and syntax for values of this type + // (it should not be possible to construct an ArtiPath out of a String that + // uses disallowed chars, or one that is in the wrong format (TBD exactly what + // this format is supposed to look like) + pub struct ArtiPath(String); /// The path of a key in the C Tor key store. pub struct CTorPath(PathBuf); @@ -172,9 +236,9 @@ TODO: decide what the metadata is serialized to /// Information about where a particular key could be stored. pub trait KeyIdentity: Serialize + Deserialize { - type CTorKey; - /// The location of the key in the Arti key store. + /// + /// This also acts as a unique identifier for a specific key instance. fn arti_path(&self) -> ArtiPath; /// The location of the key in the C Tor key store (if supported). @@ -266,7 +330,7 @@ impl KeyMgr { let raw_key = fs::read(key_path)?; // TODO hs: compare the comment field of the key with - // key_id.string_rep(), returning an error (or maybe `Ok(None)`) if + // key_id.arti_path(), returning an error (or maybe `Ok(None)`) if // they don't match K::arti_decode(raw_key, key_id).map(Some) } From 9af07e91a2f05b483f8277ed49da0d12d72f1dfe Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Wed, 19 Apr 2023 18:14:05 +0100 Subject: [PATCH 05/13] dev docs: Remove HSM APIs. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 8215884bf..1259f2933 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -227,13 +227,6 @@ Another would be to simply store it as-is. /// The path of a key in the C Tor key store. pub struct CTorPath(PathBuf); - /// The location of a key on a smartcard - pub struct HsmLocation { - /// The slot where the key is located. - slot: usize - // TODO hs: what other fields does this need? - } - /// Information about where a particular key could be stored. pub trait KeyIdentity: Serialize + Deserialize { /// The location of the key in the Arti key store. @@ -243,17 +236,6 @@ Another would be to simply store it as-is. /// The location of the key in the C Tor key store (if supported). fn ctor_path(&self) -> Option; - - /// The slot where the key is located on the smartcard. - fn hsm_slot(&self) -> Option; - - /// The string representation of this key identity; - fn string_rep(self) -> String { - // TODO hs: serialize as what? - serde_json::to_string(&self) - } - - // TODO hs: other methods? } /// The result of deserializing the comment field of an openSSH key. @@ -303,8 +285,6 @@ enum KeyStoreKind { CTor(PathBuf), /// An Arti key store that uses OpenSSH key format. Arti(PathBuf), - /// An HSM key store (TODO hs: figure out how this is going to work). - Hsm, } /// The key manager. @@ -344,7 +324,6 @@ impl KeyMgr { let raw_key = fs::read(key_path)?; K::ctor_decode(raw_key, key_id).map(Some) } - KeyStoreKind::Hsm => unimplemented!(), } } @@ -364,7 +343,6 @@ impl KeyMgr { fs::write(key_path, key) } - KeyStoreKind::Hsm => unimplemented!(), } } From 0be6e0a4caed0236e4fba1d678f6c99fb4b7d300 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Wed, 19 Apr 2023 20:11:58 +0100 Subject: [PATCH 06/13] dev docs: Remove KeyIdentityResult. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 1259f2933..64cda854d 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -238,16 +238,6 @@ Another would be to simply store it as-is. fn ctor_path(&self) -> Option; } - /// The result of deserializing the comment field of an openSSH key. - // TODO hs: we might not actually need this - #[derive(Serialize, Deserialize)] - enum KeyIdentityResult { - /// A known key identity. - Key(K), - /// An unsupported key type. - Unknown(String), - } - /// An identifier for a client/relay/... #[derive(AsRef, Into, ...)] pub struct LocalUserIdentity(String); From 37493b6bd3eed413a6244ab79763cfc6e6d551b1 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 24 Apr 2023 17:15:58 +0100 Subject: [PATCH 07/13] dev docs: Add namespacing for client/hs/relay/.. keys. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 81 +++++++++++++++++---------------- 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 64cda854d..fd6df2e15 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -160,46 +160,49 @@ each path within the `keys.arti_store.root_dir` directory, minus the extension, is the `arti_path` of a particular key): ``` -├── alice # HS client identity "alice" -│   ├── foo.onion -│ │   ├── hsc_desc_enc # arti_path = "alice/foo.onion/hsc_desc_enc" -│ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS -│ │ │ # descriptors of foo.onion") -│ │   └── hsc_intro_auth # arti_path = "alice/foo.onion/hsc_intro_auth" -│ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing -│ │ # signatures to prove to foo.onion she is authorized") -│ │ # Note: this is not implemented in C Tor -│   └── bar.onion -│    ├── hsc_desc_enc # arti_path = "alice/foo.onion/hsc_desc_enc" -│ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS -│ │ # descriptors of bar.onion") -│    └── hsc_intro_auth # arti_path = "alice/bar.onion/hsc_intro_auth" -│ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing -│ # signatures to prove to bar.onion she is authorized") -├── bob # HS client identity "bob" -│   └── foo.onion -│    ├── hsc_desc_enc # arti_path = "bob/foo.onion/hsc_desc_enc" -│ │ # (HS client Bob's x25519 hsc_desc_enc keypair for decrypting the HS -│ │ # descriptors of foo.onion") -│    └── hsc_intro_auth # arti_path = "bob/foo.onion/hsc_intro_auth" -│ # (HS client Bob's ed25519 hsc_intro_auth keypair for computing -│ # signatures to prove to foo.onion he is authorized") -│ # Note: this is not implemented in C Tor +├── client +│ ├── alice # HS client identity "alice" +│ │   ├── foo.onion +│ │ │   ├── hsc_desc_enc # arti_path = "client/alice/foo.onion/hsc_desc_enc" +│ │ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ │ │ # descriptors of foo.onion") +│ │ │   └── hsc_intro_auth # arti_path = "client/alice/foo.onion/hsc_intro_auth" +│ │ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing +│ │ │ # signatures to prove to foo.onion she is authorized") +│ │ │ # Note: this is not implemented in C Tor +│ │   └── bar.onion +│ │    ├── hsc_desc_enc # arti_path = "client/alice/foo.onion/hsc_desc_enc" +│ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ │ # descriptors of bar.onion") +│ │    └── hsc_intro_auth # arti_path = "client/alice/bar.onion/hsc_intro_auth" +│ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing +│ │ # signatures to prove to bar.onion she is authorized") +│ └── bob # HS client identity "bob" +│    └── foo.onion +│    ├── hsc_desc_enc # arti_path = "client/bob/foo.onion/hsc_desc_enc" +│ │ # (HS client Bob's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ # descriptors of foo.onion") +│    └── hsc_intro_auth # arti_path = "client/bob/foo.onion/hsc_intro_auth" +│ # (HS client Bob's ed25519 hsc_intro_auth keypair for computing +│ # signatures to prove to foo.onion he is authorized") +│ # Note: this is not implemented in C Tor +├── hs +│ └── baz.onion # Hidden service baz.onion +│    ├── authorized_clients # The clients authorized to access baz.onion +│ │   └── dan +│ │ └── hsc_desc_enc # arti_path = "hs/baz.onion/authorized_clients/dan/hsc_desc_enc.pub" +│ │ # (The public part of HS client Dan's x25519 hsc_desc_enc keypair for +│ │ # decrypting baz.onions descriptors) +│ │ +│ │ +│ │   +│    ├── hs_id # arti_path = "hs/baz.onion/hs_id" (baz.onion's identity key) +│    └── hs_blind_id # arti_path = "hs/baz.onion/hs_blind_id" (baz.onion's blinded identity key) │ -├── baz.onion # Hidden service baz.onion -│   ├── authorized_clients # The clients authorized to access baz.onion -│ │   └── dan -│ │ └── hsc_desc_enc # arti_path = "baz.onion/authorized_clients/dan/hsc_desc_enc.pub" -│ │ # (The public part of HS client Dan's x25519 hsc_desc_enc keypair for -│ │ # decrypting baz.onions descriptors) -│ │ -│ │ -│ │   -│   ├── hs_id # arti_path = "baz.onion/hs_id" (baz.onion's identity key) -│   └── hs_blind_id # arti_path = "baz.onion/hs_blind_id" (baz.onion's blinded identity key) -├── Carol # Relay Carol -│   └── ... -└── .... +├── relay +│ └── Carol # Relay Carol +│    └── ... +... ``` ### Comment field format From 6c6e03ec4af627d7b0d5b603d46ff89ed8ac19d7 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 24 Apr 2023 17:16:42 +0100 Subject: [PATCH 08/13] dev docs: Remove outdated reference to the comment field. We decided against using it. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index fd6df2e15..847285a36 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -205,12 +205,6 @@ is the `arti_path` of a particular key): ... ``` -### Comment field format - -TODO hs: decide what format to store the `arti_path` in. One option would be -to encode it as an email address: `@_artikeyid.arti.torproject.org`. -Another would be to simply store it as-is. - ### The `KeyIdentity` trait ```rust From 3202aa33c5cec7280cb5b7bb203bc9a1e5b81b03 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 24 Apr 2023 17:18:52 +0100 Subject: [PATCH 09/13] dev docs: Remove unnecessary trait bounds. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 847285a36..ff902355c 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -225,7 +225,7 @@ is the `arti_path` of a particular key): pub struct CTorPath(PathBuf); /// Information about where a particular key could be stored. - pub trait KeyIdentity: Serialize + Deserialize { + pub trait KeyIdentity { /// The location of the key in the Arti key store. /// /// This also acts as a unique identifier for a specific key instance. From a5ff3191e93ba006b06af331de6860084c7ec04e Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 24 Apr 2023 17:41:37 +0100 Subject: [PATCH 10/13] dev docs: Update KeyMgr implementation based on latest discussions. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 425 +++++++++++++++++++++++--------- 1 file changed, 313 insertions(+), 112 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index ff902355c..c0f3fdb18 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -17,9 +17,19 @@ See also: [#728] ## Usage example ```rust - let key_id: HsClientSecretKeyIdentity = - (LocalUserIdentity, HsId, HsClientSecretKeySpecifier).into(); - let sk: Option = keymgr.get::(key_id)?; +let client_id = HsClientIdentity::from_str("alice")?; + +let intro_auth_key_id: HsClientSecretKeyIdentity = + (client_id, hs_id, HsClientKeyRole::IntroAuth).into(); + +// Get KP_hsc_intro_auth +let sk: Option = keymgr.get::(&intro_auth_key_id)?; + +// Alternatively, instead of returning a type-erased value, KeyStore::get could return a `Key` +// (TODO hs: come up with a better name), `Key` being an enum over all supported key types. +// `Key` could then have a `Key::as` convenience function that returns the underlying key (or +// an error if none of the variants have the requested type): +// let sk: Option = keymgr.get(&key_id)?.as::().unwrap(); ``` ## Key stores @@ -144,60 +154,49 @@ identifies an instance of a type of key. From an implementation standpoint, as a unique identifier for a particular instance of a key. * `ctor_path`: the location of the key in the C Tor key store (optional). -For the keys stored in the Arti key store (i.e. in the OpenSSH format), the -`KeyIdentity::arti_path()` is converted to a string and placed in the `coment` -field of the key. When retrieving keys from the Arti store, the key manager -compares the `comment` of the stored key with the `KeyIdentity::arti_path()` of -the requested key identity. If the values match, it retrieves the key. -Otherwise, it returns an error. - -To identify the path of a key in the Arti key store, the key manager prepends -`keys.arti_store.root_dir` to `KeyIdentity::arti_path()` and appends an -extension. - For example, an Arti key store might have the following structure (note that each path within the `keys.arti_store.root_dir` directory, minus the extension, is the `arti_path` of a particular key): ``` ├── client -│ ├── alice # HS client identity "alice" +│ ├── alice # HS client identity "alice" │ │   ├── foo.onion -│ │ │   ├── hsc_desc_enc # arti_path = "client/alice/foo.onion/hsc_desc_enc" -│ │ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS -│ │ │ │ # descriptors of foo.onion") -│ │ │   └── hsc_intro_auth # arti_path = "client/alice/foo.onion/hsc_intro_auth" -│ │ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing -│ │ │ # signatures to prove to foo.onion she is authorized") -│ │ │ # Note: this is not implemented in C Tor +│ │ │   ├── hsc_desc_enc.arti_priv # arti_path = "client/alice/foo.onion/hsc_desc_enc" +│ │ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ │ │ # descriptors of foo.onion") +│ │ │   └── hsc_intro_auth.arti_priv # arti_path = "client/alice/foo.onion/hsc_intro_auth" +│ │ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing +│ │ │ # signatures to prove to foo.onion she is authorized") +│ │ │ # Note: this is not implemented in C Tor │ │   └── bar.onion -│ │    ├── hsc_desc_enc # arti_path = "client/alice/foo.onion/hsc_desc_enc" -│ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS -│ │ │ # descriptors of bar.onion") -│ │    └── hsc_intro_auth # arti_path = "client/alice/bar.onion/hsc_intro_auth" -│ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing -│ │ # signatures to prove to bar.onion she is authorized") -│ └── bob # HS client identity "bob" +│ │    ├── hsc_desc_enc.arti_priv # arti_path = "client/alice/foo.onion/hsc_desc_enc" +│ │ │ # (HS client Alice's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ │ # descriptors of bar.onion") +│ │    └── hsc_intro_auth.arti_priv # arti_path = "client/alice/bar.onion/hsc_intro_auth" +│ │ # (HS client Alice's ed25519 hsc_intro_auth keypair for computing +│ │ # signatures to prove to bar.onion she is authorized") +│ └── bob # HS client identity "bob" │    └── foo.onion -│    ├── hsc_desc_enc # arti_path = "client/bob/foo.onion/hsc_desc_enc" -│ │ # (HS client Bob's x25519 hsc_desc_enc keypair for decrypting the HS -│ │ # descriptors of foo.onion") -│    └── hsc_intro_auth # arti_path = "client/bob/foo.onion/hsc_intro_auth" -│ # (HS client Bob's ed25519 hsc_intro_auth keypair for computing -│ # signatures to prove to foo.onion he is authorized") -│ # Note: this is not implemented in C Tor +│    ├── hsc_desc_enc.arti_priv # arti_path = "client/bob/foo.onion/hsc_desc_enc" +│ │ # (HS client Bob's x25519 hsc_desc_enc keypair for decrypting the HS +│ │ # descriptors of foo.onion") +│    └── hsc_intro_auth.arti_priv # arti_path = "client/bob/foo.onion/hsc_intro_auth" +│ # (HS client Bob's ed25519 hsc_intro_auth keypair for computing +│ # signatures to prove to foo.onion he is authorized") +│ # Note: this is not implemented in C Tor ├── hs -│ └── baz.onion # Hidden service baz.onion -│    ├── authorized_clients # The clients authorized to access baz.onion +│ └── baz.onion # Hidden service baz.onion +│    ├── authorized_clients # The clients authorized to access baz.onion │ │   └── dan -│ │ └── hsc_desc_enc # arti_path = "hs/baz.onion/authorized_clients/dan/hsc_desc_enc.pub" -│ │ # (The public part of HS client Dan's x25519 hsc_desc_enc keypair for -│ │ # decrypting baz.onions descriptors) +│ │ └── hsc_desc_enc.arti_pub # arti_path = "hs/baz.onion/authorized_clients/dan/hsc_desc_enc" +│ │ # (The public part of HS client Dan's x25519 hsc_desc_enc keypair for +│ │ # decrypting baz.onions descriptors) │ │ │ │ │ │   -│    ├── hs_id # arti_path = "hs/baz.onion/hs_id" (baz.onion's identity key) -│    └── hs_blind_id # arti_path = "hs/baz.onion/hs_blind_id" (baz.onion's blinded identity key) +│    ├── hs_id.arti_priv # arti_path = "hs/baz.onion/hs_id" (baz.onion's identity key) +│    └── hs_blind_id.arti_priv # arti_path = "hs/baz.onion/hs_blind_id" (baz.onion's blinded identity key) │ ├── relay │ └── Carol # Relay Carol @@ -208,45 +207,72 @@ is the `arti_path` of a particular key): ### The `KeyIdentity` trait ```rust - /// The path of a key in the Arti key store. +/// The path of a key in the Arti key store. +/// +/// NOTE: There is a 1:1 mapping between a value that implements +/// `KeyIdentity` and its corresponding `ArtiPath`. +/// A `KeyIdentity` can be converted to an `ArtiPath`, +/// but the reverse conversion is not supported. +// +// TODO hs: restrict the character set and syntax for values of this type +// (it should not be possible to construct an ArtiPath out of a String that +// uses disallowed chars, or one that is in the wrong format (TBD exactly what +// this format is supposed to look like) +pub struct ArtiPath(PathBuf); + +/// The path of a key in the C Tor key store. +pub struct CTorPath(PathBuf); + +/// The "identity" of a key. +/// +/// `KeyIdentity::arti_path()` uniquely identifies an instance of a key. +pub trait KeyIdentity { + /// The location of the key in the Arti key store. /// - /// NOTE: There is a 1:1 mapping between a value that implements - /// `KeyIdentity` and its corresponding `ArtiPath`. - /// A `KeyIdentity` can be converted to an `ArtiPath`, - /// but the reverse conversion is not supported. - // - // TODO hs: restrict the character set and syntax for values of this type - // (it should not be possible to construct an ArtiPath out of a String that - // uses disallowed chars, or one that is in the wrong format (TBD exactly what - // this format is supposed to look like) - pub struct ArtiPath(String); + /// This also acts as a unique identifier for a specific key instance. + fn arti_path(&self) -> ArtiPath; - /// The path of a key in the C Tor key store. - pub struct CTorPath(PathBuf); + /// The location of the key in the C Tor key store (if supported). + fn ctor_path(&self) -> Option; +} - /// Information about where a particular key could be stored. - pub trait KeyIdentity { - /// The location of the key in the Arti key store. - /// - /// This also acts as a unique identifier for a specific key instance. - fn arti_path(&self) -> ArtiPath; +/// An identifier for an HS client. +#[derive(AsRef, Into, ...)] +struct HsClientIdentity(String); - /// The location of the key in the C Tor key store (if supported). - fn ctor_path(&self) -> Option; +impl FromStr for HsClientIdentity { /* check syntax rules */ } + +/// The role of a HS client key. +enum HsClientKeyRole { + /// A key for deriving keys for decrypting HS descriptors (KP_hsc_desc_enc). + DescEnc, + /// A key for computing INTRODUCE1 signatures (KP_hsc_intro_auth). + IntroAuth, +} + +struct HsClientSecretKeyIdentity { + /// The client associated with this key. + client_id: HsClientIdentity, + /// The hidden service this authorization key is for. + hs_id: HsId, + /// The role of the key. + role: HsClientKeyRole, +} + +impl KeyIdentity for HsClientSecretKeyIdentity { + fn arti_path(&self) -> ArtiPath { + ArtiPath( + Path::new("client") + .join(self.client_id.to_string()) + .join(self.hs_id.to_string()) + .join(self.role.to_string()), + ) } - /// An identifier for a client/relay/... - #[derive(AsRef, Into, ...)] - pub struct LocalUserIdentity(String); - - impl FromStr for LocalUserIdentity { /* check syntax rules */ } - - struct HsClientSecretKeyIdentity { ... } - - - impl KeyIdentity for HsClientSecretKeyIdentity { + fn ctor_path(&self) -> Option { ... } +} ``` @@ -262,56 +288,39 @@ pub trait EncodableKey { // Implement `EncodableKey` for all the key types we wish to support. impl EncodableKey for ed25519::SecretKey { - ... + fn key_type() -> KeyType { + KeyType::Ed25519Private + } } - ... -enum KeyStoreKind { - /// A C Tor-style key store. - CTor(PathBuf), - /// An Arti key store that uses OpenSSH key format. - Arti(PathBuf), -} - /// The key manager. +#[derive(Default)] struct KeyMgr { - /// The type of persistent store. - store_kind: KeyStoreKind, - ... + /// The underlying persistent stores. + key_store: Vec>, } impl KeyMgr { /// Read a key from the key store, attempting to deserialize it as `K`. - pub fn get( - &self, - key_id: &dyn KeyIdentity - ) -> Result> { - match self.store_kind { - KS_hs_ipt_sidKeyStoreKind::Arti(keystore) => { - let key_path = keystore.join(key_id.arti_path()); - if !key_path.exists() { - // Not found - return Ok(None); - } + pub fn get(&self, key_id: &dyn KeyIdentity) -> Result> { + // Check if the requested key identity exists in any of the key stores: + for store in &self.key_store { + let key = store.get(key_id, K::key_type())?; - let raw_key = fs::read(key_path)?; - // TODO hs: compare the comment field of the key with - // key_id.arti_path(), returning an error (or maybe `Ok(None)`) if - // they don't match - K::arti_decode(raw_key, key_id).map(Some) - } - KeyStoreKind::CTor(keystore) => { - let key_path = keystore.join(key_id.ctor_path()); - if !key_path.exists() { - // Not found - return Ok(None); - } - - let raw_key = fs::read(key_path)?; - K::ctor_decode(raw_key, key_id).map(Some) + if key.is_some() { + // Found it! Now try to downcast it to the right type (the + // downcast should _not_ fail, because K::key_type() tells the + // store to return a key of type `K` constructed from the key + // material read from disk) + return key + .map(|k| k.downcast::().map(|k| *k).map_err(|e| /* bug */ ...)) + .transpose(); } } + + // Not found + Ok(None) } /// Write `key` to the key store. @@ -342,4 +351,196 @@ impl KeyMgr { } ``` +## Proposed key store API + +The key manager reads from (and writes to) the configured key stores. The key +stores all implement the `KeyStore` trait: + +```rust +/// A generic key store. +pub trait KeyStore { + /// Retrieve the key identified by `key_id`. + fn get(&self, key_id: &dyn KeyIdentity, key_type: KeyType) -> Result>; + + /// Write `key` to the key store. + fn insert(&self, key: &dyn EncodableKey, key_id: &dyn KeyIdentity, key_type: KeyType) -> Result<()>; + + /// Remove the specified key. + fn remove(&self, key_id: &dyn KeyIdentity, key_type: KeyType) -> Result<()>; +} +``` + +We will initially support 2 key store implementations (one for the C Tor key +store, and another for the Arti store): +```rust + +impl KeyStore for ArtiNativeKeyStore { + fn get(&self, key_id: &dyn KeyIdentity, key_type: KeyType) -> Result> { + let key_path = self.key_path(key_id, key_type); + + let input = match fs::read(key_path) { + Ok(input) => input, + Err(e) if matches!(e.kind(), ErrorKind::NotFound) => return Ok(None), + Err(e) => return Err(...), + }; + + key_type.read_ssh_format_erased(&input).map(Some) + } + + fn insert(&self, key: &dyn EncodableKey, key_id: &dyn KeyIdentity, key_type: KeyType) -> Result<()> { + let key_path = self.key_path(key_id, key_type); + + let ssh_format = key_type.write_ssh_format(key)?; + fs::write(key_path, ssh_format).map_err(|_| ())?; + + Ok(()) + } + + fn remove(&self, key_id: &dyn KeyIdentity, key_type: KeyType) -> Result<()> { + let key_path = self.key_path(key_id, key_type); + + fs::remove_file(key_path).map_err(|e| ...)?; + + Ok(()) + } +} + +impl ArtiNativeKeyStore { + /// The path on disk of the key with the specified identity and type. + fn key_path(&self, key_id: &dyn KeyIdentity, key_type: KeyType) -> PathBuf { + self.keystore_dir + .join(key_id.arti_path().0) + .join(key_type.extension()) + } +} + +impl KeyStore for CTorKeyStore { + ... +} + +#[derive(Copy, Clone, ...)] +pub enum KeyType { + Ed25519Private, + Ed25519Public, + + X25519StaticSecret, + X25519Public, + // ...plus all the other key types we're interested in. +} + +impl KeyType { + /// The file extension for a key of this type. + /// + /// We use nonstandard extensions to prevent keys from being used in unexpected ways (e.g. if + /// the user renames a key from KP_hsc_intro_auth.arti_priv to KP_hsc_intro_auth.arti_priv.old, + /// the arti key store should disregard the backup file). + /// + /// The key stores will ignore any files that don't have a recognized extension. + pub fn extension(&self) -> &'static str { + // TODO hs: come up with a better convention for extensions. + if self.is_private() { + ".arti_priv" + } else { + ".arti_pub" + } + } + + /// Whether the key is public or private. + fn is_private(&self) -> bool { + match self { + // Secret key types + KeyType::Ed25519Private | KeyType::X25519StaticSecret => true, + // Public key types + KeyType::Ed25519Public | KeyType::X25519Public => false, + } + } +} + +pub enum Algorithm { + Ed25519, + X25519, + ... +} + +impl Algorithm { + fn as_str(&self) -> &'static str { + ... + } +} + +``` + +The `ArtiNativeKeyStore` uses the `SshKeyType` implementation of `KeyType` +to read and write OpenSSH key files: +```rust + +pub trait SshKeyType: Send + Sync + 'static { + fn ssh_algorithm(&self) -> Algorithm; + + /// Read an OpenSSH key, parse the key material into a known key type, returning the + /// type-erased value. + /// + /// The caller is expected to downcast the value returned to a concrete type. + fn read_ssh_format_erased(&self, input: &[u8]) -> Result; + + /// Encode an OpenSSH-formatted key. + fn write_ssh_format(&self, key: &dyn EncodableKey) -> Result>; +} + +impl SshKeyType for KeyType { + fn ssh_algorithm(&self) -> Algorithm { + ... + } + + fn read_ssh_format_erased(&self, input: &[u8]) -> Result { + match self { + KeyType::Ed25519Private => { + let sk = ssh_key::PrivateKey::from_bytes(input).map_err(|_| ())?; + + // Build the expected key type (i.e. convert ssh_key key types to the key types + // we use internally). + let sk = match sk.key_data() { + KeypairData::Ed25519(kp) => { + ed25519::SecretKey ::from_bytes(&kp.private.to_bytes())? + } + _ => { + // bug + return Err(...); + } + }; + + Ok(Box::new(sk)) + } + KeyType::Ed25519Public => { + let pk = ssh_key::PublicKey::from_bytes(input).map_err(|_| ())?; + + // Build the expected key type (i.e. convert ssh_key key types to the key types + // we use internally). + let pk = match pk.key_data() { + KeyData::Ed25519(pk) => ed25519::PublicKey::from_bytes(&pk.0)?, + _ => { + // bug + return Err(...); + } + }; + + Ok(Box::new(pk)) + } + KeyType::X25519StaticSecret | KeyType::X25519Public => { + // The ssh-key crate doesn't support arbitrary key types. We'll probably + // need a more general-purpose crate for parsing OpenSSH (one that allows + // arbitrary values for the algorithm), or to roll our own (we + // could also fork ssh-key and modify it as required). + todo!() + } + } + } + + fn write_ssh_format(&self, key: &dyn EncodableKey) -> Result> { + /* Encode `key` in SSH key format. */ + } +} + +``` + [#728]: https://gitlab.torproject.org/tpo/core/arti/-/issues/728 From 67061688a6a3473fa5faa0963efc5c6dff872cea Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 24 Apr 2023 19:57:27 +0100 Subject: [PATCH 11/13] dev docs: Remove incoherent waffle. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index c0f3fdb18..d046e464e 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -131,25 +131,10 @@ key_dir = "" ## Key identities -A key by itself is not very useful. In order for Arti to be able to use it, it -needs to also know the _role_ of the key (i.e., what that key is supposed to be -used for), among other things. As such, we need to store some metadata alongside -each key. - -For client authorization keys, C Tor achieves this using a passwd-style format -(see the `CLIENT AUTHORIZATION` section of the `tor` manpage), which stores keys -and their associated metadata in the same file. Other keys don't have other -metadata than their _role_ (i.e. "Ed25519 permanent identity key", "medium-term -Ed25519 signing key", etc.), which can be unambiguously determined by looking at -the path/file name of the key file (e.g. the role of -`/ed25519_master_id_private_key` is "Ed25519 permanent identity -key of a relay", `/private_key` is "the private key of -the hidden service whose data is stored at ``", etc.). - -For this reason, we introduce the concept of a "key identity" (specified for -each supported key type via the `KeyIdentity` trait). A "key identity" uniquely -identifies an instance of a type of key. From an implementation standpoint, -`KeyIdentity` implementers must specify: +We introduce the concept of a "key identity" (specified for each supported key +type via the `KeyIdentity` trait). A "key identity" uniquely identifies an +instance of a type of key. From an implementation standpoint, `KeyIdentity` +implementers must specify: * `arti_path`: the location of the key in the Arti key store. This also serves as a unique identifier for a particular instance of a key. * `ctor_path`: the location of the key in the C Tor key store (optional). From 5407e599ab2560a4cae12c60fce21430c972c11c Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 24 Apr 2023 19:57:43 +0100 Subject: [PATCH 12/13] dev docs: Fill out insert/remove APIs. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 41 +++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index d046e464e..7ac7b05d0 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -264,6 +264,7 @@ impl KeyIdentity for HsClientSecretKeyIdentity { ## Proposed key manager API ```rust +/// A key that can be stored in, or retrieved from, a `KeyStore.` pub trait EncodableKey { /// The underlying key type. fn key_type() -> KeyType @@ -308,7 +309,14 @@ impl KeyMgr { Ok(None) } - /// Write `key` to the key store. + /// Insert the specified key into the appropriate key store. + /// + /// If the key bundle (key family?) of this `key` exists in one of the key stores, the key is + /// inserted there. Otherwise, the key is inserted into the first key store. + /// + /// If the key already exists, it is overwritten. + /// + /// TODO hs: update the API to return a Result> here (i.e. the old key) pub fn insert( &self, key_id: &dyn KeyIdentity, @@ -318,20 +326,35 @@ impl KeyMgr { if store.has_key_bundle(key_id) { return store.insert(&key, key_id, K::key_type()); } - KeyStoreKind::CTor(keystore) => { - let key_path = keystore.join(key_id.ctor_path()); - let key = K::arti_encode(key_id)?; - - fs::write(key_path, key) - } } + + // None of the stores has the key bundle of key_id, so we insert the key into the first key + // store. + if let Some(store) = self.key_store.first() { + return store.insert(&key, key_id, K::key_type()); + } + + // Bug: no key stores were configured + Err(...) } - pub fn remove( + /// Remove the specified key. + /// + /// If the key exists in multiple key stores, this will only remove it from the first one. An + /// error is returned if none of the key stores contain the specified key. + pub fn remove( &self, key_id: &dyn KeyIdentity, ) -> Result<()> { - unimplemented!() + for store in &self.key_store { + match store.remove(key_id, K::key_type()) { + Ok(()) => return Ok(()), + Err(e) if e is NotFound => continue, + Err(e) => return Err(e), + } + } + + Err(not found) } } ``` From 17ff3a6f6a08fe33df1f3fe1630444dfcc976f3a Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Tue, 25 Apr 2023 12:07:30 +0100 Subject: [PATCH 13/13] dev docs: Add a few lines about handling concurrent access. Signed-off-by: Gabriela Moldovan --- doc/dev/notes/key-management.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/doc/dev/notes/key-management.md b/doc/dev/notes/key-management.md index 7ac7b05d0..aca87cb9d 100644 --- a/doc/dev/notes/key-management.md +++ b/doc/dev/notes/key-management.md @@ -551,4 +551,18 @@ impl SshKeyType for KeyType { ``` +## Concurrent access for disk-based key stores + +The key stores will allow concurrent modification by different processes. In +order to implement this safely without locking, the key store operations (get, +insert, remove) will need to be atomic. Reading and removing keys atomically is +trivial. To create/import a key atomically, we write the new key to a temporary +file before using `rename(2)` to atomically replace the existing one (this +ensures preexisting keys are replaced atomically). + +Note: on Windows, we can't use `rename` to atomically replace an existing file +with a new one (`rename` returns an error if the destination path already +exists). As such, on Windows we will need some sort of synchronization mechanism +(unless it exposes some other APIs we can use for atomic renaming). + [#728]: https://gitlab.torproject.org/tpo/core/arti/-/issues/728