Skip to content

Comments

feat: add PrincipalId and ic_pem_identity crate#450

Closed
hansl wants to merge 2 commits intomasterfrom
hansl/identity-sign-in
Closed

feat: add PrincipalId and ic_pem_identity crate#450
hansl wants to merge 2 commits intomasterfrom
hansl/identity-sign-in

Conversation

@hansl
Copy link
Contributor

@hansl hansl commented Mar 12, 2020

Sign messages and allow for PEM files to be read and used to sign.

In order to reduce dependencies, MessageWithSender and SignedMessage
now are not generics.

Notes

This PR actually does not work because the replica isn't up to spec. But it can serve as a starting point once the replica is fixed.

hansl added 2 commits March 11, 2020 22:30
Sign messages and allow for PEM files to be read and used to sign.

In order to reduce dependencies, MessageWithSender and SignedMessage
now are not generics.
@eftychis
Copy link
Contributor

eftychis commented Mar 12, 2020

Notes to discuss:

  1. We place the code that touches keys all around. If we want to do a review, we can't focus on one or two places.
  2. We don't always sign or authenticate. We can not go back to the replica and say, remove those Optionals and start enforcing that validation functionality. Also, our main path is the non-default option. (Although I do see an Id.pem created locally?)
  3. Everything seems focused around PEM files. Users pass a PEM key file, right? Won't the average user just generate the file and store inside the project directory, which is a git repo? Also as it is passed, we will have to ask for a passphrase, as the next step would be to support encrypted key files as the default. I am not sure how to make this painless for the user, as the argument is the key (the path, but you get the idea), not a profile id or something along those lines.
  4. Changes related to user identity now have to be spread all around dfx. I am not sure e.g. how to keep them focused, or how to provide the same functionality to e.g. rust-sdk or similar efforts. It is already bad enough we have to write this code twice, but the browser was always a special snowflake case.

Other notes (to self mainly): When receiving a key file we usually validate it on the spot as containing the right curve etc.

P.S. I would prefer we do not block ourselves or code on the replica behaviour. Adding more blockers does not help.

@eftychis
Copy link
Contributor

P.S. Thank you for the time you took to share this example, Hans, with your thoughts!

@eftychis
Copy link
Contributor

Hmm, also the principal should only be constructible by the signer -- hence placing it internally originally. The agent similarly to the handler see it as a Blob.

@hansl
Copy link
Contributor Author

hansl commented Mar 13, 2020

also the principal should only be constructible by the signer

I don't agree with that. They are blobs, so they can be constructed by anyone. They're also part of the public spec so they should be in ic_http_agent.

VALID principal IDs should be constructible by either the signer or the public API (e.g. when you get one from IDL). But the type itself should still be part of the crate responsible for exchanging with the replica. It's an agent type.

@hansl
Copy link
Contributor Author

hansl commented Mar 13, 2020

Let me take some time to answer your points;

We place the code that touches keys all around.

Keys are touched only in the ic_pem_identity crate. If you are talking about the PrincipalId type it's an API type and should be put with the rest of the API (like RequestId and Blob). We don't touch keys in there (but there is an argument named key which comes from the spec).

We don't always sign or authenticate.

That's fine for this PR. Stay focused. We can add this PR, which took me 3 hours, then add error messaging for users in another PR if they don't use identity, then add the dfx.json configuration for identity, then make identity mandatory. That's 5 PRs that should be spaced on 2 releases (we probably want to tell the user identity will be required).

Right now we have none of that, not even in an optional manner.

Users pass a PEM key file, right? Won't the average user just generate the file and store inside the project directory, which is a git repo?

We can change that code later, and have the PEM in the global ~/.cache/dfinity folder. We can have dfx id create and such. But we don't have to have that now.

we will have to ask for a passphrase

A prompt can be added later.

support encrypted key files as the default

Not default. If SSH does not have that as default, I think we'll be fine.

Changes related to user identity now have to be spread all around dfx.

There are 2 files changed: the argument which is part of all canister commands (and honestly that's all we need), and generating a new key which is in the new command. The environment changes are for allowing overloading the signer.

@eftychis
Copy link
Contributor

eftychis commented Mar 16, 2020

support encrypted key files as the default
Not default. If SSH does not have that as default, I think we'll be fine.

It is actually. You get prompted for a passphrase. You can pick it to be empty, but I hope we don't promote that.

@eftychis
Copy link
Contributor

We can change that code later, and have the PEM in the global ~/.cache/dfinity folder. We can have dfx id create and such. But we don't have to have that now.

I agree. I do that in #438 though. Can you look?

@hansl hansl closed this Apr 27, 2020
@hansl hansl deleted the hansl/identity-sign-in branch April 27, 2020 19:03
dfinity-bot added a commit that referenced this pull request Oct 26, 2020
## Changelog for advisory-db:
Branch: master
Commits: [rustsec/advisory-db@6e48979d...0ad26bc7](rustsec/advisory-db@6e48979...0ad26bc)

* [`5751a5f4`](rustsec/advisory-db@5751a5f) CI: bump rustsec-admin to v0.3.0-pre2 ([RustSec/advisory-db⁠#438](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/438))
* [`65441901`](rustsec/advisory-db@6544190) Add unmaintained crate advisory for stdweb ([RustSec/advisory-db⁠#439](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/439))
* [`acc73d95`](rustsec/advisory-db@acc73d9) CI: bump rustsec-admin to v0.3.0-pre3 ([RustSec/advisory-db⁠#440](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/440))
* [`691a7504`](rustsec/advisory-db@691a750) Assigned RUSTSEC-2020-0056 to stdweb ([RustSec/advisory-db⁠#441](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/441))
* [`8505c957`](rustsec/advisory-db@8505c95) Add unmaintained crate advisory for `block-cipher` ([RustSec/advisory-db⁠#442](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/442))
* [`fa616899`](rustsec/advisory-db@fa61689) Assigned RUSTSEC-2020-0057 to block-cipher ([RustSec/advisory-db⁠#443](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/443))
* [`5c9ebbfa`](rustsec/advisory-db@5c9ebbf) Add unmaintained crate advisory for `stream-cipher` ([RustSec/advisory-db⁠#444](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/444))
* [`218de91a`](rustsec/advisory-db@218de91) Assigned RUSTSEC-2020-0058 to stream-cipher ([RustSec/advisory-db⁠#445](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/445))
* [`681a2040`](rustsec/advisory-db@681a204) Add advisory for notable UB fix in libpulse-binding v2.6.0 ([RustSec/advisory-db⁠#435](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/435))
* [`1e48ac39`](rustsec/advisory-db@1e48ac3) Assigned RUSTSEC-2019-0038 to libpulse-binding ([RustSec/advisory-db⁠#446](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/446))
* [`59bee556`](rustsec/advisory-db@59bee55) Add advisory for use-after-frees fixed in libpulse-binding v1.2.1 ([RustSec/advisory-db⁠#433](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/433))
* [`146de2d8`](rustsec/advisory-db@146de2d) Assigned RUSTSEC-2018-0021 to libpulse-binding ([RustSec/advisory-db⁠#447](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/447))
* [`3b1f08f2`](rustsec/advisory-db@3b1f08f) Unyank RUSTSEC-2020-0011 ([RustSec/advisory-db⁠#448](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/448))
* [`3796cc00`](rustsec/advisory-db@3796cc0) README.md: point chat badge to Zulip ([RustSec/advisory-db⁠#449](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/449))
* [`1bf68e0d`](rustsec/advisory-db@1bf68e0) RUSTSEC-2020-0015: use wildcards in version req ([RustSec/advisory-db⁠#450](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/450))
* [`67a2144b`](rustsec/advisory-db@67a2144) RUSTSEC-2020-0015: remove wildcards ([RustSec/advisory-db⁠#451](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/451))
* [`0ad26bc7`](rustsec/advisory-db@0ad26bc) Bump `rustsec-admin` to v0.3.0 ([RustSec/advisory-db⁠#452](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/452))
mergify bot pushed a commit that referenced this pull request Oct 26, 2020
## Changelog for advisory-db:
Branch: master
Commits: [rustsec/advisory-db@6e48979d...0ad26bc7](rustsec/advisory-db@6e48979...0ad26bc)

* [`5751a5f4`](rustsec/advisory-db@5751a5f) CI: bump rustsec-admin to v0.3.0-pre2 ([RustSec/advisory-db⁠#438](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/438))
* [`65441901`](rustsec/advisory-db@6544190) Add unmaintained crate advisory for stdweb ([RustSec/advisory-db⁠#439](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/439))
* [`acc73d95`](rustsec/advisory-db@acc73d9) CI: bump rustsec-admin to v0.3.0-pre3 ([RustSec/advisory-db⁠#440](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/440))
* [`691a7504`](rustsec/advisory-db@691a750) Assigned RUSTSEC-2020-0056 to stdweb ([RustSec/advisory-db⁠#441](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/441))
* [`8505c957`](rustsec/advisory-db@8505c95) Add unmaintained crate advisory for `block-cipher` ([RustSec/advisory-db⁠#442](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/442))
* [`fa616899`](rustsec/advisory-db@fa61689) Assigned RUSTSEC-2020-0057 to block-cipher ([RustSec/advisory-db⁠#443](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/443))
* [`5c9ebbfa`](rustsec/advisory-db@5c9ebbf) Add unmaintained crate advisory for `stream-cipher` ([RustSec/advisory-db⁠#444](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/444))
* [`218de91a`](rustsec/advisory-db@218de91) Assigned RUSTSEC-2020-0058 to stream-cipher ([RustSec/advisory-db⁠#445](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/445))
* [`681a2040`](rustsec/advisory-db@681a204) Add advisory for notable UB fix in libpulse-binding v2.6.0 ([RustSec/advisory-db⁠#435](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/435))
* [`1e48ac39`](rustsec/advisory-db@1e48ac3) Assigned RUSTSEC-2019-0038 to libpulse-binding ([RustSec/advisory-db⁠#446](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/446))
* [`59bee556`](rustsec/advisory-db@59bee55) Add advisory for use-after-frees fixed in libpulse-binding v1.2.1 ([RustSec/advisory-db⁠#433](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/433))
* [`146de2d8`](rustsec/advisory-db@146de2d) Assigned RUSTSEC-2018-0021 to libpulse-binding ([RustSec/advisory-db⁠#447](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/447))
* [`3b1f08f2`](rustsec/advisory-db@3b1f08f) Unyank RUSTSEC-2020-0011 ([RustSec/advisory-db⁠#448](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/448))
* [`3796cc00`](rustsec/advisory-db@3796cc0) README.md: point chat badge to Zulip ([RustSec/advisory-db⁠#449](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/449))
* [`1bf68e0d`](rustsec/advisory-db@1bf68e0) RUSTSEC-2020-0015: use wildcards in version req ([RustSec/advisory-db⁠#450](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/450))
* [`67a2144b`](rustsec/advisory-db@67a2144) RUSTSEC-2020-0015: remove wildcards ([RustSec/advisory-db⁠#451](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/451))
* [`0ad26bc7`](rustsec/advisory-db@0ad26bc) Bump `rustsec-admin` to v0.3.0 ([RustSec/advisory-db⁠#452](http://r.duckduckgo.com/l/?uddg=https://github.com/RustSec/advisory-db/issues/452))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants