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

Xmzheng/client sdk callformat lib #962

Merged
merged 12 commits into from Jul 6, 2022
Merged

Conversation

xmzheng
Copy link
Contributor

@xmzheng xmzheng commented Jun 1, 2022

add encryption transport lib to web-ts. Add same test logic for derive_symmetric_key for golang, rust and typescript.

@xmzheng xmzheng requested a review from nhynes June 1, 2022 02:56
client-sdk/ts-web/rt/package.json Outdated Show resolved Hide resolved
client-sdk/ts-web/package.json Outdated Show resolved Hide resolved
case types.CALLFORMAT_ENCRYPTED_X25519DEOXYSII:
if (cfg && cfg.publicKey) {
const privateKey = ed.utils.randomPrivateKey();
const publicKey = ed.curve25519.scalarMultBase(privateKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

could we move this code for generating a keypair into the mrae file, to even out the level of abstraction here

Copy link
Contributor

Choose a reason for hiding this comment

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

reopening: you've instead moved key generation from callformat to the caller of encodeCall. we need to discuss this. but I need to review other reviewers' comments to see if there was anything else about this

client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/package.json Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Show resolved Hide resolved
client-sdk/ts-web/rt/src/mrae.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/types.ts Outdated Show resolved Hide resolved
@pro-wh
Copy link
Contributor

pro-wh commented Jun 1, 2022

oh also everybody's not been making "fmt" changes as separate commits, so could I ask you not to have that as a separate commit either

@pro-wh
Copy link
Contributor

pro-wh commented Jun 1, 2022

also can you link to the other languages' implementations?

@xmzheng
Copy link
Contributor Author

xmzheng commented Jun 1, 2022

@pro-wh thanks for your comments, I will update my diff.

this diff related to other implementations: oasisprotocol/oasis-core#4772

client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/mrae.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/mrae.ts Outdated Show resolved Hide resolved
privateKey: Uint8Array,
): Uint8Array {
const sharedKey = deriveSymmetricKey(publicKey, privateKey);
var aead = new deoxysii.AEAD(sharedKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

This also has a non-trivial setup cost such that the cipher could (should) be cached. It's worse because the JS DeoxysII is not hw accelerated. For consistency with the other impls, though, I won't make a fuss about this. This is also not the lowest hanging fruit–just annoying.

Copy link
Contributor

Choose a reason for hiding this comment

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

how much setup cost is there in this setup? I see from the syntax that this creates an object, which must have some kind of state. is the setup separable from the lifetime of this state at all?

if it is, maybe we could expose something like nacl's 'box before' and 'box/open after' system https://www.npmjs.com/package/tweetnacl/v/1.0.3#naclboxbeforetheirpublickey-mysecretkey

Copy link
Contributor

Choose a reason for hiding this comment

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

this code doesn't have a concept of a session, so any repeated calls incur overhead of setting up the shared secret and initializing deoxysii state. if there were sessions, the deoxysii setup and x25519 could be saved

client-sdk/ts-web/rt/test/mrae.test.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/test/mrae.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

thanks for the changes so far

client-sdk/ts-web/rt/src/mrae.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/package.json Outdated Show resolved Hide resolved
call: types.Call,
format: types.CallFormat,
config?: EncodeConfig,
): Promise<[types.Call, MetaEncryptedX25519DeoxysII?]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

comparing with this

// It returns the encoded call and any metadata needed to successfully decode the result.
func EncodeCall(call *types.Call, cf types.CallFormat, cfg *EncodeConfig) (*types.Call, interface{}, error) {
, it looks like the intention is to allow metadata types other than the x25519deoxysii. maybe we should declare it as unknown, similar to the interface{} they use in the go version? what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, you could use something like this to implement generic associated types. Go doesn't have that ability, which is why it's interface{}.

With the aim of not suggesting sweeping changes to the codebase, unknown is fine since at least it won't be a breaking change when other metadatas are added. It'll just be inconvenient to inspect the metadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

I guess I'll check on that sample later today 🙊

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh we used to do this thing in Go where there would be an overarching struct with a named field for each variant.

ok I loaded up the sample, but it's not clear how to go between associated Metadata.* and CallFormat.* type. (seems incidental and slightly abusive that CallFormat.Plain = Metadata.Plain.)

another answer would be to have multiple typescript declaration overloads of such an encode function. as nhynes says though, feel free not to and use unknown instead

I think it's fine that these types come out of encode relatively opaque. users will only pass them verbatim to submit and decode, respectively, neither of which internally could take advantage more type info

client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
case types.CALLFORMAT_ENCRYPTED_X25519DEOXYSII:
if (cfg && cfg.publicKey) {
const privateKey = ed.utils.randomPrivateKey();
const publicKey = ed.curve25519.scalarMultBase(privateKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

reopening: you've instead moved key generation from callformat to the caller of encodeCall. we need to discuss this. but I need to review other reviewers' comments to see if there was anything else about this

import * as types from './types';
import * as mrae from './mrae';

var deoxysii = require('deoxysii');
Copy link
Contributor

Choose a reason for hiding this comment

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

643221f#diff-3253cbee91843f44bfc8dd19a126548d016d25c34bf4ce34f3b8f1f93dd9b47fR3-R4

the sad answer is still to do that and have typescript ignore that error

client-sdk/ts-web/rt/src/callformat.ts Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/rt/src/mrae.ts Outdated Show resolved Hide resolved
client-sdk/ts-web/package.json Outdated Show resolved Hide resolved
call: types.Call,
format: types.CallFormat,
config?: EncodeConfig,
): Promise<[types.Call, MetaEncryptedX25519DeoxysII?]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, you could use something like this to implement generic associated types. Go doesn't have that ability, which is why it's interface{}.

With the aim of not suggesting sweeping changes to the codebase, unknown is fine since at least it won't be a breaking change when other metadatas are added. It'll just be inconvenient to inspect the metadata.

client-sdk/ts-web/rt/src/mrae.ts Outdated Show resolved Hide resolved
@xmzheng xmzheng requested a review from pro-wh June 27, 2022 16:29
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

some philosophical stuff

return oasis.misc.fromCBOR(pt) as types.CallResult;
} else if (result.fail) {
throw new Error(
`callformat: failed call: module :${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`callformat: failed call: module :${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,
`callformat: failed call: module: ${result.fail.module} code: ${result.fail.code} message: ${result.fail.message}`,

Copy link
Contributor

Choose a reason for hiding this comment

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

other than that, is this right for us to throw in a "decode" function? like wouldn't be more fitting of the function's objectives to return a CallResult that indicates a runtime failure? or is result.fail not the kind of error I'm thinking of?

edit: ugh the go side does this. I hope they have a good reason......................................

);
}

throw new Error(`callformat: unexpected result: ${result.ok}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like maybe we'd want to see the whole result object, so that we could see if it were completely the wrong type or something. purely speculative, as we don't any experience of this happening. but would that be okay with you?

but then on the other hand, we'd probably only see "[object Object]" 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

what I usually do is attach the result/cause to what I throw. something like this can be used to keep TS happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

source 👍

throw new Error('callformat: runtime call data public key not set');
}
const rawCall = oasis.misc.toCBOR(call);
const sealedCall = mrae.boxSeal(nonce, rawCall, null, config.publicKey.key, sk);
Copy link
Contributor

Choose a reason for hiding this comment

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

philosophical question about the layers of abstraction here:

  • this callformat asks for x25519 deoxysii
  • the primitive we've currently selected to occupy the mrae ts module is x25519 deoxysii
  • we use the mrae module here

but the mrae module is named agnostically. does that mean we're not sure it'll always be x25519 deoxysii? if we change out the selected crypto primitive, how would we do it? change it directly and fix this callsite? add the new primitive in a new module instead? redefine the callformat as well?

or is the binding of x25519 deoxysii to the oasis projects' notion of mrae a permanent thing?

on the go side

mraeDeoxysii "github.com/oasisprotocol/oasis-core/go/common/crypto/mrae/deoxysii"
, the module is named deoxysii 🤷

client-sdk/ts-web/rt/src/callformat.ts Show resolved Hide resolved
client-sdk/ts-web/rt/src/callformat.ts Show resolved Hide resolved
privateKey: Uint8Array,
): Uint8Array {
const sharedKey = deriveSymmetricKey(publicKey, privateKey);
var aead = new deoxysii.AEAD(sharedKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

how much setup cost is there in this setup? I see from the syntax that this creates an object, which must have some kind of state. is the setup separable from the lifetime of this state at all?

if it is, maybe we could expose something like nacl's 'box before' and 'box/open after' system https://www.npmjs.com/package/tweetnacl/v/1.0.3#naclboxbeforetheirpublickey-mysecretkey

@xmzheng xmzheng requested review from nhynes and pro-wh July 2, 2022 02:56
@xmzheng xmzheng force-pushed the xmzheng/client-sdk-callformat-lib branch from d6744fe to 6a271c9 Compare July 6, 2022 16:08
@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #962 (ed84b41) into main (b4efbe8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #962   +/-   ##
=======================================
  Coverage   67.96%   67.96%           
=======================================
  Files         128      128           
  Lines       11017    11017           
=======================================
  Hits         7488     7488           
  Misses       3497     3497           
  Partials       32       32           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4efbe8...ed84b41. Read the comment docs.

const pairs = nacl.box.keyPair()
const publicKey = pairs.publicKey
const rawCall: oasisRT.types.Call = {
format: oasisRT.transaction.CALLFORMAT_ENCRYPTED_X25519DEOXYSII,
Copy link
Contributor

Choose a reason for hiding this comment

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

some tabs here, could you convert to spaces

* encodeCallWithNonceAndKeys encodes a call based on its configured call format.
* It returns the encoded call and any metadata needed to successfully decode the result.
*/
export function encodeCallWithNonceAndKeys(
Copy link
Contributor

Choose a reason for hiding this comment

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

in some cases, we're making cryptography-related operations unnecessarily async, e.g. hashing a public key into an address. this is so that we could more easily change implementations in the future, if a more preferred library comes out exposing an async interface. or if the primitives we use later become available in the async web crypto interface and we could use that.

on the other hand, in other places, such as converting between binary and bech32 addresses, it's a synchronous interface. I think these places are the ones where it's a more niche kind of operation, and there's a single well established library for doing it, and that library does it synchronously.

I think this particular case is closer to the first scenario. but this is a late coming comment, and I understand we're interested in merging this right away to support some ongoing work elsewhere, so eh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to change the signature.

Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

thanks for the changes, let's get this in so people can start using it

): Promise<[types.Call, unknown]> {
const nonce = randomBytes(deoxysii.NonceSize);
const keyPair = nacl.box.keyPair();
return await encodeCallWithNonceAndKeys(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do I need to use await here? I think I can directly return the Promise, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

you're right about that. stylistically I've preferred to await inside because it makes it easier to move code around

@xmzheng xmzheng merged commit 84c1789 into main Jul 6, 2022
@xmzheng xmzheng deleted the xmzheng/client-sdk-callformat-lib branch July 6, 2022 20:44
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.

None yet

3 participants