-
Notifications
You must be signed in to change notification settings - Fork 98
feat: Add identity stage1 signing #462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a500928
bfd55e7
93366c3
b17201b
81bc873
c2e9e66
bbf8616
4ff91d2
6e628bc
6e3eeb6
50d3a25
6949f48
bae8077
cff25b4
cd2c685
8ea519f
e866c0d
54f093b
4fd305c
520ba74
c1e4002
6184fe7
6abb4fd
7996cb6
876d3f2
9207303
b0e78d5
5620eb5
9b7a0eb
682b2e4
45f1a72
5e3c608
1b2e798
167cbc3
d92b9bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,5 +4,6 @@ members = [ | |
| "src/dfx_derive", | ||
| "src/dfx_info", | ||
| "src/ic_http_agent", | ||
| "src/ic_identity_manager", | ||
| "src/serde_idl", | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| use ic_http_agent::to_request_id; | ||
| use ic_http_agent::AgentError; | ||
| use ic_http_agent::Blob; | ||
| use ic_http_agent::RequestId; | ||
| use ic_http_agent::SignedMessage; | ||
| use ic_http_agent::Signer; | ||
| use std::path::PathBuf; | ||
|
|
||
| pub struct Identity(ic_identity_manager::Identity); | ||
|
|
||
| impl Identity { | ||
| /// Construct a new identity handling object, providing given | ||
| /// configuration. | ||
| pub fn new(identity_config_path: PathBuf) -> Self { | ||
| Self( | ||
| // We panic as discussed, as that should not be the | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to avoid comments like this, as they don't age well. Imagine you join a company, and you see this comment in the code base. Who wrote it? Who discussed it with whom? What is the purpose of this comment? If this issue matters, then: "TODO(eftychis): Escalate to Dom and return error instead of panic." or whatever, or better still, file a ticket "Return error instead of panic". If it doesn't matter that much, then omit the comment. It should be clear from the code that it panics, and just as clear that in theory someone could make it return an error instead. |
||
| // case. I personally prefer this to be an error. | ||
| ic_identity_manager::Identity::new(identity_config_path) | ||
| .expect("Expected a valid identity configuration"), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| impl Signer for Identity { | ||
| fn sign<'a>( | ||
| &self, | ||
| request: Box<(dyn erased_serde::Serialize + Send + Sync + 'a)>, | ||
| ) -> Result< | ||
| ( | ||
| RequestId, | ||
| Box<dyn erased_serde::Serialize + Send + Sync + 'a>, | ||
| ), | ||
| AgentError, | ||
| > { | ||
| let request_id = to_request_id(&request).map_err(AgentError::from)?; | ||
| let signature_tuple = self | ||
| .0 | ||
| .sign(Blob::from(request_id).as_slice()) | ||
| .map_err(|e| AgentError::SigningError(e.to_string()))?; | ||
| let signature = Blob::from(signature_tuple.signature.clone()); | ||
| let sender_pubkey = Blob::from(signature_tuple.public_key); | ||
| let signed_request = SignedMessage { | ||
| request_with_sender: request, | ||
| signature, | ||
| sender_pubkey, | ||
| }; | ||
| Ok((request_id, Box::new(signed_request))) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use super::*; | ||
| use proptest::prelude::*; | ||
| use serde::Serialize; | ||
| use tempfile::tempdir; | ||
|
|
||
| // Dummy proptest checking request id is correct for now. | ||
| proptest! { | ||
| #[test] | ||
| fn request_id_identity(request: String) { | ||
| let dir = tempdir().unwrap(); | ||
|
|
||
| #[derive(Clone,Serialize)] | ||
| struct TestAPI { inner : String} | ||
| let request = TestAPI { inner: request}; | ||
|
|
||
| let request_with_sender = request.clone(); | ||
| let actual_request_id = to_request_id(&request_with_sender).expect("Failed to produce request id"); | ||
|
|
||
| let signer = Identity::new(dir.into_path()); | ||
| let request_id = signer.sign(Box::new(request)).expect("Failed to sign").0; | ||
| assert_eq!(request_id, actual_request_id) | ||
| }} | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| [package] | ||
| name = "ic-identity-manager" | ||
| version = "0.1.0" | ||
| authors = ["DFINITY Stiftung"] | ||
| edition = "2018" | ||
|
|
||
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
|
|
||
| [dependencies] | ||
| openssl = "0.10.28" | ||
| pem = "0.7.0" | ||
| ring = "0.16.11" | ||
| serde = { version = "1.0", features = ["derive"] } | ||
| ic-http-agent = { path = "../ic_http_agent" } | ||
|
|
||
|
|
||
| [dev-dependencies] | ||
| serde_cbor = "0.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this just
identity.rs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually why have this here (I just saw this was in dfx) instead of re-using the
ic_identity_manager?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We define the trait in this crate. This would avoid building the whole agent to build the identity crate. And in effect, rebuilding the identity crate every time a small change happens in the agent, which should receive more frequent updates. (I know now types lives under agent, but as suggested and also requested by @chenyan-dfinity, we should have a spec types crate or something along these lines).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put yourself in someone making a webserver which talks to the IC as a backend (in Rust). They would use
ic_http_agentto talk to the IC, but since the IC needs a signature, they'll need to provide aSignersomehow. Should they copy-paste this whole file everywhere, or just useic_identity_manager?Alternatively, what's the case for a program that uses
ic_identity_managerwithout usingic_http_agentat all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my PR, the separation was clear;
ic_pem_identitywas for using a PEM (it was an implementation of an identity, with the goal to have maybeic_yubikey_identitylater or similar). The goal was also to reduce transitive dependencies; we don't need to have yubikey-related code if we use pem files.In this case, I don't know what
ic_identity_manageris (still). Is it a manager? Is it the only implementation we'll provide for identity and Signing?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a lib, it is to be used indirectly. I have gotten a bit mixed messages as to the usage of the agent and its API from the last few discussions, so I stand a bit confused. May I suggest we write down how we want people to use it and define the hierarchy and the pros and cons of each? Rushing ahead a bit, perhaps a thin layer defining an API ahead optimizes for both external users and build times.
I think for now it is fine to optimize for build time and leave the trait implementation there, and then move it in agent_api or something? Or maybe just in the agent. (I think the agent crate as you noted has gotten a bit overloaded, so perhaps clarifying its scope will help. We should think as crates as our compilation, scoping and test buddies.)
ic_identity_managershould house all the implementations, as was mentioned in the documentation in the previous PR, (e.g. like how actix, has multiple actor types). I can run ahead and say we need a common type that deals with multiple principals and associated key pairs and their sources (but I would like to avoid starting this long conversation again). I think it should be the only implementation provided by us at least. (And while someone might decide to write their own agent, they probably should be using our types and identity functionality and not rewrite them. Although I see little point for anyone to rewrite any of the above anytime soon just to say there is a non-dfinity implementation. Thoughts? Perhaps not to be discussed here.) The Signer trait is there to provide separation and mocking ability.