-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
213 support for symmetric key within encryptedthing and decryptedthing #215
213 support for symmetric key within encryptedthing and decryptedthing #215
Conversation
Co-authored-by: Marcus Pousette <marcus.pousette@live.com>
Co-authored-by: Marcus Pousette <marcus.pousette@live.com>
packages/utils/crypto/src/aes256.ts
Outdated
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.
@marcus-pousette What do you think about this key type? All good?
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.
@marcus-pousette Updated the README
I want to suggest the following interface for the new export type KeypairFromPublicKey<T> = T extends X25519PublicKey
? X25519PublicKey extends T
? X25519Keypair
: Ed25519Keypair
: Ed25519Keypair;
export interface Keychain {
// Add a key to the keychain.
// Represents keys internally as X25519 and Aes256.
// Transforms Ed25519 keys to X25519 keys internally?
import(keypair: Ed25519Keypair | X25519Keypair | Aes256Key, id: Uint8Array): Promise<void>;
// This is only really relevant for asymmetric keys? -> No changes
exportByKey<
T extends Ed25519PublicKey | X25519PublicKey,
Q = KeypairFromPublicKey<T>
>(
publicKey: T
): Promise<Q | undefined>;
// ID's are the sha256 hashes of the public key (or the symmetric key itself)
// Throws if no key can be found of type `type` with id `id`?
exportById<
T = "ed25519" | "x25519" | "aes256",
Q = T extends "ed25519" ? Ed25519Keypair : T extends "x25519" ? X25519Keypair : Aes256Key
>(
id: Uint8Array,
type: T
): Promise<Q | undefined>;
} |
@marcus-pousette Please check out the new Keychain interface in /src/packages/keychain/index.ts. Feedback most welcome |
I think it looks good. But there should be perhaps be an exportByKey should work for symmetric keys, like let say the sha256 base64 hash of it or something. Not fully landed yet. Need to play around with it and see. Basically
Also I was thinking about the AES256 name, it is a 32 bit key, but libsodium crypto_secretbox is actually XSalsa20. But anyway both of these use a random 32 byte secret. Perhaps this AES256 class should just be named ByteKey or something and perhaps have a length restriction to 32 bytes |
Regarding the This is how it is currently for encrypt. DecryptedThing Some thoughts: For type EncryptWithAsymmetric = X25519Keypair | { publicKey: X25519PublicKey, encrypt: (data: Uint8array, publicKey:X25519PublicKey, receiver: X25519PublicKey) => Uint8array // second argument is publickey associated with choosen keypair to encrypt the message with
encrypt(receivers: X25519PublicKey[], with: EncryptWithAsymmetric) and for the symmetric key (I am writing them out as different types now for simplicitly type EncryptWithSymmetric = Uint8arrayWith32Length | { selectedKeyHash: Uint8array, encrypt: (data: Uint8array, selectedKeyHash: Uint8array) => Uint8array)
encrypt(with: EncryptWithSymmetric) OR there could be a utility that takes an EncryptionProvider into some kind of "session" like const getEncryptFunction = (provider: EncryptionProvider, args): (data) => Uint8array => { ... deal with provider in some way ... }
const getEncryptFunction = (keypair: X25519Keypair): (data) => Uint8array => { return libsodium crypto box easy (keypair) } and then you would only have (in DecryptedThing) encrypt: (fn: ReturnType<typeof getEncryptFunction>) {
do stuff with fn which might be based on a EncryptionProvider or just a keypair
} The whole point with EncryptionProvider is to allow all crypto functions to be performed without keys leaving the Provider. Will continue on this, but I am just thinking out loud here. |
7ef6a45
to
1f1facc
Compare
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.
@marcus-pousette Please check out my review on your changes
packages/utils/keychain/src/index.ts
Outdated
export interface Keychain { | ||
// Add a key to the keychain. | ||
import( | ||
parameters: ( |
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.
Maybe parameters should just be Keypair
for asym keys and Bytekey
for symmetric? If someone wants to pass in a specific key like x25519 they can still use the X25519Keypair
and pass it in
packages/utils/keychain/src/index.ts
Outdated
export interface Keychain { | ||
// Add a key to the keychain. | ||
import( | ||
parameters: ( |
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.
Maybe parameters should just be Keypair
for asym keys and Bytekey
for symmetric? If someone wants to pass in a specific key like x25519 they can still use the X25519Keypair
and pass it in
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 might be possible to just have a "key" parameter and absorb everything with it. That will be easier to use from the user side. Just call
await keychain.import(key, id?) // optional id
(but reject publickeys because it does not make sense to import a public key x))
@@ -20,6 +22,8 @@ export abstract class Keypair { | |||
toPeerId(): Promise<PeerId> { | |||
throw new Error("Not implemented"); | |||
} | |||
|
|||
// TODO: Should we add not implemented errors for .create and and .from as well? | |||
} |
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.
@marcus-pousette What do you think - errors for Key.create()
and Key.from()
?
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.
might be tricky to do with the types. Because it has to be generic. For example if there is a from(PeerID) that works for publickeys, that type make no sense for bytekey for example.
I would say dont add until its necessary. (Lazy approach usually is the way)
type KeyResolver = <PublicKey extends X25519PublicKey | Uint8Array>( | ||
key: PublicKey | ||
) => | ||
| (PublicKey extends X25519PublicKey ? X25519Keypair : Uint8Array) |
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.
Could we return Keypair
here instead of Uint8Array
?
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.
The Uint8array referred to the symmetric key with length 32.. the one that is going to be used with XSalsa20Poly1305 encryption scheme
type KeyResolver = <PublicKey extends X25519PublicKey | Uint8Array>( | ||
key: PublicKey | ||
) => | ||
| (PublicKey extends X25519PublicKey ? X25519Keypair : Uint8Array) |
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.
... And I can see that Keyresolver is an integral part in the local encrypter and decrypter. That assumes that the dev needs to have access to the keys at the moment the encrypter and decrypter are initialized - What might not be the case for the master encryption (coming from LIT e.g.). Should we therefore also allow passing an encrypt: (plainText: Uint8Array) => Uint8Array
and decrypt: (cipherText: Uint8Array) => Uint8Array
to initialize the encrypter and decrypter?
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.
Yea. But in that case the LIT is not a "local" decrypter encrypter.
If you look into the encrypt method of DecryptedThing you see that the first argument is EncryptProvide which allows you to pass
(plainText: Uint8Array) => Uint8Array
More or less. basically, the arguments of decrypt and encrypt methods are functional, and can bee whatever you want.
It might perhaps be interesting to see a (integration) test case where LIT is used soo we can make sure it works with this crypto abstraction (?).
return compare(this.key, other.key) === 0; | ||
} | ||
|
||
// TODO: What should be preprended to this string here? |
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.
What do you think about this?
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.
There needs to an instanceof check here
if (other instanceof ByteKey) {
return equals(this.key, other.key);
}
return false;
}) | ||
); | ||
|
||
const ks = receiverX25519PublicKeys.map((receiverPublicKey) => { |
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.
I think we should also encrypt for the sender - so take key.publicKey
into the recipientPublicKeys array?
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.
I went into this question a few months ago. But I realize that a valid case which needs to be coverred is when the sender generated a throwaway X25519Keypair and just want to send a message to someone and never read it themselves
then it will be redudant to also encrypt for the sender.
BUT
**when you do encryption in the document store, the sender is adding themselves to the receivers array because beofre the encrypt function is called because it is very likely that when you put documents into a document store you also want to read the documents later. **
^
|
|
I need to verify this
f692297
to
015dae4
Compare
Hello again! I have cherry some of your commits from this PR and included it in the Keychain release (2 months ago (?)) Here is the
Also some of the Keychain commits has been cherry picked but I had to change quite a few things to get everything working well. See https://github.com/dao-xyz/peerbit/tree/master/packages/utils/keychain for the current implementation Sadly Github does not pickup you as the author of the commits, even though I cherry picked from your branch using the git command. This means you sadly don't show up on the contributors list even if you have done some work here. If you know a solution to fix this please let me know :) The current solution that exist (in master) is perhaps not perfect but a good way to get into the generics you want. I will close this PR in favor of opening new PRs targeting up to date code |
EncryptedThing.decrypt()
andDecryptedThing.encrypt()