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

Basic Deno support #1448

Merged
merged 3 commits into from Dec 7, 2021
Merged

Conversation

Hexagon
Copy link
Contributor

@Hexagon Hexagon commented Dec 4, 2021

Related to Discussion #1447

Changes made:

  • Update README.md to indicate experimental support for Deno
  • Add extra checks in src/*, to avoid directly accessing objects/properties not defined in a Deno environment, specifically navigator.userAgent and navigator.hardwareCurrency

Tested with:

  • npm test
  • npm run browsertest
  • npm run lint
  • Example 1-3 from README.md#examples, using Deno 1.16.4, both JavaScript and TypeScript mode

Testable by:

  1. Make sure openpgpjs is checked out locally
  2. If not already done - run npm run build to generate dist/openpgp.mjs
  3. Create a test application - test.js - (in this example in a directory adjacent to openpgpjs ../denotest)
// Import locally
import * as openpgp from '../openpgpjs/dist/openpgp.mjs';

(async () => {
    const message = await openpgp.createMessage({ binary: new Uint8Array([0x01, 0x01, 0x01]) });
    const encrypted = await openpgp.encrypt({
        message, // input as Message object
        passwords: ['secret stuff'], // multiple passwords possible
        format: 'binary' // don't ASCII armor (for Uint8Array output)
    });
    console.log(encrypted); // Uint8Array

    const encryptedMessage = await openpgp.readMessage({
        binaryMessage: encrypted // parse encrypted bytes
    });
    const { data: decrypted } = await openpgp.decrypt({
        message: encryptedMessage,
        passwords: ['secret stuff'], // decrypt with password
        format: 'binary' // output as Uint8Array
    });
    console.log(decrypted); // Uint8Array([0x01, 0x01, 0x01])
})();
  1. Run test application deno run test.js
  2. It works (or should, at least)!
  3. If implemented, this would be a fully working deno application
import * as openpgp from '<deno land url>/openpgp/dist/openpgp.mjs';
p
(async () => {
    const message = await openpgp.createMessage({ binary: new Uint8Array([0x01, 0x01, 0x01]) });
    const encrypted = await openpgp.encrypt({
        message, // input as Message object
        passwords: ['secret stuff'], // multiple passwords possible
        format: 'binary' // don't ASCII armor (for Uint8Array output)
    });
    console.log(encrypted); // Uint8Array

    const encryptedMessage = await openpgp.readMessage({
        binaryMessage: encrypted // parse encrypted bytes
    });
    const { data: decrypted } = await openpgp.decrypt({
        message: encryptedMessage,
        passwords: ['secret stuff'], // decrypt with password
        format: 'binary' // output as Uint8Array
    });
    console.log(decrypted); // Uint8Array([0x01, 0x01, 0x01])
})();

@Hexagon Hexagon force-pushed the feature/initial-deno-support branch from 7def5e5 to 3877f7b Compare Dec 4, 2021
@Hexagon
Copy link
Contributor Author

Hexagon commented Dec 4, 2021

All tests passing now, force pushed some fixes as no one seem to have looked at it yet.

@Hexagon Hexagon force-pushed the feature/initial-deno-support branch from 3877f7b to 037a641 Compare Dec 4, 2021
@Hexagon
Copy link
Contributor Author

Hexagon commented Dec 4, 2021

Another force push fixing linting

@SeparateRecords
Copy link

SeparateRecords commented Dec 5, 2021

navigator.hardwareConcurrency is actually available in stable Deno, from 1.13 (August)

@Hexagon
Copy link
Contributor Author

Hexagon commented Dec 5, 2021

navigator.hardwareConcurrency is actually available in stable Deno, from 1.13 (August)

Oh! Got an error (in Deno) saying navigator isn't defined, so I made an assumption that everything following navigator is undefined. Need to have a closer look at what was going on.

@Hexagon Hexagon force-pushed the feature/initial-deno-support branch from 037a641 to 1874aca Compare Dec 5, 2021
@Hexagon
Copy link
Contributor Author

Hexagon commented Dec 5, 2021

Slightly simplified after feedback from @SeparateRecords, thanks!

Copy link
Member

@twiss twiss left a comment

Hello 👋 Thanks for the PR! Some small comments below.

README.md Outdated
Import as an ES6 module, from an .mjs file on a CDN of your choice.

```js
import * as openpgp from "https://cdn.jsdelivr.net/npm/openpgp@5/dist/openpgp.mjs";
Copy link
Member

@twiss twiss Dec 6, 2021

Choose a reason for hiding this comment

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

For a security-related module like OpenPGP.js, I would prefer not to suggest to use a CDN, at least without something like Subresource Integrity (don't know if it's possible to add it here?)

Copy link
Contributor Author

@Hexagon Hexagon Dec 6, 2021

Choose a reason for hiding this comment

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

Maybe deno.land/x can be seen as a more trusted source? This will require a maintaner to register the module at https://deno.land/x, and add a webhook to github.

For now, maybe we can keep it anonymous, with an example showing a local import?

Edit: Suggested a more anonymous approach in latest commit, see new single comment.

Copy link
Member

@twiss twiss Dec 7, 2021

Choose a reason for hiding this comment

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

Alrighty, yeah. We can do that after merging this.

Copy link
Contributor Author

@Hexagon Hexagon Dec 7, 2021

Choose a reason for hiding this comment

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

Sweet 👍

src/crypto/hash/index.js Outdated Show resolved Hide resolved
src/util.js Outdated Show resolved Hide resolved
test/general/streaming.js Outdated Show resolved Hide resolved
test/worker/application_worker.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Hexagon Hexagon requested a review from twiss Dec 6, 2021
twiss
twiss approved these changes Dec 7, 2021
@twiss twiss merged commit ce5174d into openpgpjs:main Dec 7, 2021
9 checks passed
@renatoathaydes
Copy link

renatoathaydes commented Jan 24, 2022

Hi, I am not sure if this is related as I am not using Deno, but I see that one of the problems in this issue was references to navigator. That's probably also causing errors when I try to use this lib in a Cloudflare Worker, as pointed out to me by @twiss on Gitter.

My question on StackOverflow has more details.

I have imports like this:

import { encrypt, readKey, createMessage, decryptKey, readPrivateKey } from 'openpgp/lightweight';

And my npm dependency is on this merged branch, so I expected it would work:

    "node_modules/openpgp": {
      "version": "5.1.0",
      "resolved": "git+ssh://git@github.com/openpgpjs/openpgpjs.git#bd13edfc884cf6b6b7f715099529ac6a656733c6",
      "license": "LGPL-3.0+",
      "dependencies": {
        "asn1.js": "^5.0.0"
      },
      "engines": {
        "node": ">= 8.0.0"
      }
    },

The error I get when I run wrangler dev:

webpack 5.67.0 compiled successfully in 1785 ms
Error: Something went wrong with the request to Cloudflare...
Uncaught ReferenceError: navigator is not defined
  at line 782 in ./node_modules/openpgp/dist/lightweight/openpgp.min.mjs
  at line 808 in __webpack_require__
  at line 528 in ./src/crypto.ts
  at line 808 in __webpack_require__
  at line 652 in ./src/handler.ts
  at line 808 in __webpack_require__
  at line 1047
  at line 1052
  at line 1054
 [API code: 10021]

This happens with openpgp/lightweight and openpgp.

If you think this is unrelated, I can open a new issue.

@twiss
Copy link
Member

twiss commented Jan 24, 2022

@renatoathaydes Yeah, please create a separate issue or PR for this :)

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

4 participants