Skip to content
This repository has been archived by the owner on Oct 23, 2020. It is now read-only.

Add initial prototype #1

Merged
merged 8 commits into from
Aug 10, 2019
Merged

Conversation

tniessen
Copy link
Member

I finally had time to restructure and rewrite at least a part of my prototype. It is absolutely rudimentary, but I think it can still serve as a basic structure.

For reference, @sam-github, @mhdawson and I discussed possible WebCrypto implementations at the summit in Berlin. We decided to explore possibilities within external, non-native packages first and to discuss a built-in or native module at a later point. See nodejs/admin#366.

(Sorry Sam, it took slightly longer than I claimed it would.)

@tniessen tniessen requested a review from sam-github July 29, 2019 11:59
@rvagg
Copy link
Member

rvagg commented Jul 30, 2019

Wow, that's a lot of work in there @tniessen! good job. That manual AES stuff is a bit annoying, maybe we can push that functionality into core though, wrap it around our OpenSSL calls. For now just make it work is a good rule to follow.

Re testing and "It is our intention to add Web Platform Tests (WPT) at some point", what is that point and why not start doing it now? Is there no way to start importing portions of the WebCrypto conformance tests to guide development?

A couple of stylistic concerns:

  1. Is Babel really necessary? Dependency bloat where it's unnecessary and the layers of complexity it adds contributes to contribution barriers. From what I can see the only thing it's contributing here is translating import to require, can you justify its inclusion? If you have target versions of Node in mind you can just use linting rules to keep conformance.
  2. More comprehensive linting rules up front will save a lot of pain and argument later on. I'd suggest just adopting standard (or semi-standard if you insist), you can just pull the eslint rules as separate packages for those if you'd prefer. Or, pull the nodejs/node rules if you'd prefer them. Whatever it is, setting tight boundaries now will make reviewing code later easier.

@gengjiawen
Copy link
Member

I am -1 on Babel if we target it to Node:12.

@tniessen
Copy link
Member Author

tniessen commented Jul 30, 2019

@rvagg Thank you for the feedback!

That manual AES stuff is a bit annoying, maybe we can push that functionality into core though, wrap it around our OpenSSL calls.

We could, even though the implementation probably wouldn't be much prettier since OpenSSL does not support it natively, and I don't think we should try to move it into node core or OpenSSL until someone comes up with a reasonable use case. I thought about this quite a lot while implementing it and I still don't know why any length != 128 would be advisable.

Is there no way to start importing portions of the WebCrypto conformance tests to guide development?

I did not spend a lot of time looking at the WPTs yet or how to include them. I never worked with WPTs before and I am not sure how to run these tests outside of browsers.

Is Babel really necessary? [...] can you justify its inclusion?

Probably not, I just prefer the ESM syntax over CJS. I assume you are suggesting to convert the package to CJS instead of "native" ESM?

Or, pull the nodejs/node rules if you'd prefer them.

Good idea, I'll do that.

@gengjiawen
Copy link
Member

Probably not, I just prefer the ESM syntax over CJS.

+1 on ESM. But we can use esm with --experimental-modules or try Typescript use "module": "commonjs" as an option.

@rvagg
Copy link
Member

rvagg commented Jul 31, 2019

Probably not, I just prefer the ESM syntax over CJS. I assume you are suggesting to convert the package to CJS instead of "native" ESM?

My suggestion would be CJS, just plain, no flags, no compiling, .js files that can be run straight. We're just talking about swapping import for require after all and we get to save huge complexity and dependencies.

@ljharb
Copy link
Member

ljharb commented Jul 31, 2019

Definitely please do not rely on anything that requires a node flag :-)

@tniessen
Copy link
Member Author

tniessen commented Aug 1, 2019

Okay, I removed babel, rewrote the code in CJS, and added Node.js linting rules. Hope I didn't miss anything.

@mcollina
Copy link
Member

mcollina commented Aug 1, 2019

Maybe we should integrate Travis and get a CI run?

@tniessen
Copy link
Member Author

tniessen commented Aug 1, 2019

There is a .travis.yml in the first commit, maybe someone with admin access can enable Travis? Will Travis use the config file from the PR or do we need to merge first?

@targos
Copy link
Member

targos commented Aug 1, 2019

Travis is enabled

@targos
Copy link
Member

targos commented Aug 1, 2019

I think it will use the config file from the PR but you need to push something to trigger it

@joyeecheung
Copy link
Member

joyeecheung commented Aug 1, 2019

I did a POC of porting the WPT runner in Node.js core for the WHATWG stream repo before, and using node-core-utils to pull down the resources from the WPT repo, it might be useful: instructions are in the OP of nodejs/whatwg-stream#2

The WPT runner was updated a bit since that but it should be only more independent of Node.js internals now.

@targos
Copy link
Member

targos commented Aug 1, 2019

@panva I don't know how to do it from a fork

@panva
Copy link
Member

panva commented Aug 1, 2019

@panva I don't know how to do it from a fork

Yeah I realized its a fork so I removed my message :) you were too quick to see it :)

@tniessen
Copy link
Member Author

tniessen commented Aug 1, 2019

I force-pushed the same commit with a different timestamp and it triggered Travis, thanks for setting it up @targos! :-)

@joyeecheung
Copy link
Member

On the ESM v.s. CJS issue: although this is initially just exploring implementing WebCrypto outside of core, note that we do not support ESM internally in Node.js core. The internal modules are not compiled in the same way as normal user land CJS modules either. To include a ESM in core we need to either transpile it to CJS (and place the transpiled files under either deps or lib depending on the maintenance strategy) or write another special loader for internal ESM.

@tniessen
Copy link
Member Author

tniessen commented Aug 1, 2019

Good point, if we intend to add this implementation to core at some point, that would be a problem as well. I think we agree that CJS is the better choice for now, even though it has some downsides.

Copy link

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

A couple small suggestions, but looks great. Doesn't have to be in this PR, but a checklist of remaining work would be interesting. It seems WPT test integration is one big thing, but despite how complete this looks, I guess it doesn't support all the algs or all the ops yet?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/algorithms.js Show resolved Hide resolved
lib/algorithms.js Show resolved Hide resolved
lib/algorithms/aes.js Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
test/.eslintrc.json Outdated Show resolved Hide resolved
@tniessen
Copy link
Member Author

Thanks for the thorough review, Sam!

@tniessen tniessen merged commit e6e483b into nodejs:master Aug 10, 2019
@tniessen tniessen deleted the initial-prototype branch August 10, 2019 12:54
Copy link

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants