Skip to content
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

Make PrincipalId abstract #1128

Merged
merged 4 commits into from
Jan 18, 2020
Merged

Make PrincipalId abstract #1128

merged 4 commits into from
Jan 18, 2020

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Jan 17, 2020

done in this commit:

  • Abstract PrincipalId
  • Equality, ordering
  • Primitive conversion to blob
    (meaning that hashing, pretty-printing can be done in stdlib)
  • IDL mapping as blob.

@nomeata
Copy link
Collaborator Author

nomeata commented Jan 17, 2020

This is the ASAP implementation called for by Andreas in
https://dfinity.slack.com/archives/CPL67E7MX/p1579249416002000?thread_ts=1579204528.239900&cid=CPL67E7MX

For Linkedup, we also need stdlib.PrincipalId.hash(). (done)
Although it looks like Linkedup uses the hash as the user id, that’s a gaping security hole, because the hash is not crytographic!

@nomeata
Copy link
Collaborator Author

nomeata commented Jan 17, 2020

Also, should we map this, when used as a parameter in the IDL, to

  • a blob (probably not)
  • a service {} (since we want users to be indistinguishable from canisters)
  • a fresh principal_id?

@ggreif
Copy link
Contributor

ggreif commented Jan 17, 2020

hash is not crytographic!

Cannot be, it is only 32 bits!

@nomeata
Copy link
Collaborator Author

nomeata commented Jan 17, 2020

hash is not crytographic!

Cannot be, it is only 32 bits!

Right! (I wasn’t complaining about the hash, but the misuse). Let’s continue that LinkedUp discussion in dfinity/linkedup#6 (comment)

@crusso
Copy link
Contributor

crusso commented Jan 17, 2020

Do we need blob in the IDL for anything else, why not rename IDL blob to Principal ID. Also, I'm really note sure we should identify users with canisters - aren't there operations on canister but not users (like upgrade) that don't make sense?

@nomeata
Copy link
Collaborator Author

nomeata commented Jan 17, 2020

Do we need blob in the IDL for anything else

blob = vec nat8 in the IDL, and that’s of course a useful alias.

Also, I'm really note sure we should identify users with canisters - aren't there operations on canister but not users (like upgrade) that don't make sense?

Well, you can only upgrade a canister if are its admin (or later: if you have the capability). For users, nobody gets that ability.

There are always things you can do to some canisters that you can’t do to other canisters or users.

done in this commit:

 * Abstract `PrincipalId`
 * Equality, ordering
 * Primitive conversion to blob
   (meaning that hashing, pretty-printing can be done in stdlib)
 * IDL mapping as blob.
@nomeata nomeata force-pushed the joachim/principal-id branch from 799d030 to 090fd53 Compare January 17, 2020 10:29
@nomeata
Copy link
Collaborator Author

nomeata commented Jan 17, 2020

Ok, this should be all we need to to make the caller abstract, without breaking too much.

IDL type discussion can happen later if we don't want that bike shed to hold up this ASAP change.

@nomeata nomeata requested a review from rossberg January 17, 2020 10:33
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case you need expedited delivery.

Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, my only suggestion would be to rename it to simply Principal -- the Id suffix seems redundant.

@matthewhammer
Copy link
Contributor

Although it looks like Linkedup uses the hash as the user id, that’s a gaping security hole, because the hash is not crytographic!

@stanleygjones knows about this. We've already corrected it together, once. I think he reverted it because he was missing this PR. He can confirm.

@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Jan 18, 2020
@mergify mergify bot merged commit 5111672 into master Jan 18, 2020
@mergify mergify bot deleted the joachim/principal-id branch January 18, 2020 12:59
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jan 18, 2020
nomeata added a commit that referenced this pull request Feb 16, 2020
nomeata added a commit that referenced this pull request Feb 18, 2020
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.

5 participants