chore: remove 'verify' function (#1047)

billiegoose · web-flow · commit 59e5b128f755 · 2020-02-14T17:28:31.000-05:00
BREAKING CHANGE: The `verify` function has been removed, and the `signature` property of TagObject renamed to `gpgsig` to match CommitObject. Since `log`, `readCommit`, and `readTag` all include the `gpgsig` and signing `payload` in their return values now, it is simpler and more efficient to do the verification outside of `isomorphic-git`. See the documentation for the `onSign` parameter for complete code examples.
diff --git a/.gitignore b/.gitignore
@@ -131,7 +131,6 @@ docs/setConfig.md
 docs/status.md
 docs/statusMatrix.md
 docs/tag.md
-docs/verify.md
 docs/version.md
 docs/walk.md
 docs/writeBlob.md
diff --git a/README.md b/README.md
@@ -37,7 +37,6 @@ The following environments are tested in CI and will continue to be supported un
 TODO:
 - [ ] I should probably normalize on timestamps and get rid of the `date` options.
 - [ ] I should probably remove `username`, `password`, `token`, and `oauth2format` and make everyone use `onAuth` callback for that.
-- [ ] Make sure that the payload output of `readTag` and `readCommit` is newline-correct to easily verify. Then remove the `verify` command.
 - [ ] Update the WebWorker Guide.
 - [ ] Update the Useful Snippets.
 
@@ -54,6 +53,7 @@ TODO:
   - [x] The (previously deprecated) `utils.auth` function was removed.
   - [x] The (previously deprecated) `utils.oauth2` was removed.
   - [x] The `config` function has been removed and replaced by `getConfig`, `getConfigAll`, and `setConfig`.
+  - [x] The `verify` function has been removed, but `log`, `readCommit`, and `readTag` all return the `gpgsig` and signing `payload` now. This actually makes verification simpler and more efficient, because it can be done in batch on `git.log` output and the `gpgsig` itself can be parsed and used to lookup the public key. See [onSign](https://isomorphic-git.org/docs/en/onSign) for complete code examples.
 
 ### Some function parameters have been removed or replaced:
   - [x] The undocumented parameter aliases `authUsername` and `authPassword` were removed.
@@ -253,7 +253,6 @@ unless there is a major version bump.
 - [status](https://isomorphic-git.github.io/docs/status.html)
 - [statusMatrix](https://isomorphic-git.github.io/docs/statusMatrix.html)
 - [tag](https://isomorphic-git.github.io/docs/tag.html)
-- [verify](https://isomorphic-git.github.io/docs/verify.html)
 - [version](https://isomorphic-git.github.io/docs/version.html)
 - [walk](https://isomorphic-git.github.io/docs/walk.html)
 - [writeBlob](https://isomorphic-git.github.io/docs/writeBlob.html)
diff --git a/__tests__/__snapshots__/test-exports.js.snap b/__tests__/__snapshots__/test-exports.js.snap
@@ -56,7 +56,6 @@ Array [
   "status",
   "statusMatrix",
   "tag",
-  "verify",
   "version",
   "walk",
   "writeBlob",
diff --git a/__tests__/__snapshots__/test-readTag.js.snap b/__tests__/__snapshots__/test-readTag.js.snap
@@ -12,10 +12,10 @@ This is a tag message.
 
 ",
   "tag": Object {
+    "gpgsig": undefined,
     "message": "This is a tag message.
 ",
     "object": "033417ae18b174f078f2f44232cb7a374f4c60ce",
-    "signature": undefined,
     "tag": "mytag",
     "tagger": Object {
       "email": "wmhilton@gmail.com",
diff --git a/__tests__/test-GitAnnotatedTag.js b/__tests__/test-GitAnnotatedTag.js
@@ -37,7 +37,7 @@ const tagObject = {
     timezoneOffset: 240
   },
   message: '0.0.9',
-  signature: `-----BEGIN PGP SIGNATURE-----
+  gpgsig: `-----BEGIN PGP SIGNATURE-----
 Version: GnuPG v1
 
 iQIcBAABAgAGBQJZ1BW2AAoJEJYJuKWSi6a5S6EQAJQkK+wIXijDf4ZfVeP1E7Be
diff --git a/__tests__/test-annotatedTag.js b/__tests__/test-annotatedTag.js
@@ -1,6 +1,6 @@
 /* eslint-env node, browser, jasmine */
 const { makeFixture } = require('./__helpers__/FixtureFS.js')
-const { annotatedTag, verify, resolveRef, readTag } = require('isomorphic-git')
+const { annotatedTag, resolveRef, readTag } = require('isomorphic-git')
 
 describe('annotatedTag', () => {
   it('creates an annotated tag to HEAD', async () => {
@@ -62,13 +62,14 @@ describe('annotatedTag', () => {
       signingKey: privateKey,
       onSign: pgp.sign
     })
-    const keys = await verify({
-      fs,
-      gitdir,
-      ref: 'latest',
-      publicKeys: publicKey,
-      onVerify: pgp.verify
+    const oid = await resolveRef({ fs, gitdir, ref: 'latest' })
+    const { tag, payload } = await readTag({ fs, gitdir, oid })
+    const { valid, invalid } = await pgp.verify({
+      payload,
+      publicKey,
+      signature: tag.gpgsig
     })
-    expect(keys[0]).toBe('f2f0ced8a52613c4')
+    expect(invalid).toEqual([])
+    expect(valid).toEqual(['f2f0ced8a52613c4'])
   })
 })
diff --git a/__tests__/test-commit.js b/__tests__/test-commit.js
@@ -1,9 +1,11 @@
+import { readCommit } from 'isomorphic-git'
+
 /* eslint-env node, browser, jasmine */
 const { makeFixture } = require('./__helpers__/FixtureFS.js')
 // @ts-ignore
 const snapshots = require('./__snapshots__/test-commit.js.snap')
 const registerSnapshots = require('./__helpers__/jasmine-snapshots')
-const { commit, verify, log } = require('isomorphic-git')
+const { commit, log } = require('isomorphic-git')
 
 describe('commit', () => {
   beforeAll(() => {
@@ -206,13 +208,13 @@ describe('commit', () => {
     expect(error.toJSON()).toMatchSnapshot()
   })
 
-  it('pgp plugin signing', async () => {
+  it('create signed commit', async () => {
     // Setup
     const { pgp } = require('@isomorphic-git/pgp-plugin')
     const { fs, gitdir } = await makeFixture('test-commit')
     // Test
     const { privateKey, publicKey } = require('./__fixtures__/pgp-keys.js')
-    await commit({
+    const oid = await commit({
       fs,
       gitdir,
       message: 'Initial commit',
@@ -225,14 +227,18 @@ describe('commit', () => {
       signingKey: privateKey,
       onSign: pgp.sign
     })
-    const keys = await verify({
+    const { commit: commitObject, payload } = await readCommit({
       fs,
       gitdir,
-      ref: 'HEAD',
-      publicKeys: publicKey,
-      onVerify: pgp.verify
+      oid
+    })
+    const { valid, invalid } = await pgp.verify({
+      payload,
+      publicKey,
+      signature: commitObject.gpgsig
     })
-    expect(keys[0]).toBe('f2f0ced8a52613c4')
+    expect(invalid).toEqual([])
+    expect(valid).toEqual(['f2f0ced8a52613c4'])
   })
 
   it('with timezone', async () => {
diff --git a/__tests__/test-writeObject.js b/__tests__/test-writeObject.js
@@ -325,7 +325,7 @@ minimisted(async function ({ _: [command, ...args], ...opts }) {
           timezoneOffset: 240
         },
         message: '0.0.9',
-        signature: `-----BEGIN PGP SIGNATURE-----
+        gpgsig: `-----BEGIN PGP SIGNATURE-----
 Version: GnuPG v1
 
 iQIcBAABAgAGBQJZ1BW2AAoJEJYJuKWSi6a5S6EQAJQkK+wIXijDf4ZfVeP1E7Be
diff --git a/__tests__/test-writeTag.js b/__tests__/test-writeTag.js
@@ -23,7 +23,7 @@ describe('writeTag', () => {
           timezoneOffset: 240
         },
         message: '0.0.9',
-        signature: `-----BEGIN PGP SIGNATURE-----
+        gpgsig: `-----BEGIN PGP SIGNATURE-----
 Version: GnuPG v1
 
 iQIcBAABAgAGBQJZ1BW2AAoJEJYJuKWSi6a5S6EQAJQkK+wIXijDf4ZfVeP1E7Be
diff --git a/docs/alphabetic.md b/docs/alphabetic.md
@@ -53,7 +53,6 @@ sidebar_label: Alphabetical Index
 - [status](status)
 - [statusMatrix](statusMatrix)
 - [tag](tag)
-- [verify](verify)
 - [version](version)
 - [walk](walk)
 - [writeBlob](writeBlob)
diff --git a/docs/onSign.md b/docs/onSign.md
@@ -1,30 +1,26 @@
 ---
-title: onSign & onVerify
-sidebar_label: onSign & onVerify
+title: onSign
+sidebar_label: onSign
 ---
 
 In order to use the PGP signing feature of [commit](./commit.md), you have to provide a PGP signing callback like so:
 
 ```js
-// using ES6 modules
-import { commit } from 'isomorphic-git'
 import { pgp } from '@isomorphic-git/pgp-plugin'
-commit({ ..., onSign: pgp.sign })
+git.commit({ ..., onSign: pgp.sign })
 ```
 
-Similarly, the PGP verification function of [verify](./verify.md) requires a PGP verifying callback:
+You can choose between an [OpenPGP.js implementation](https://github.com/isomorphic-git/openpgp-plugin) and an [isomorphic-pgp implementation](https://github.com/isomorphic-git/pgp-plugin)!
 
-```js
-// using ES6 modules
-import { commit } from 'isomorphic-git'
-import { pgp } from '@isomorphic-git/pgp-plugin'
-commit({ ..., onVerify: pgp.verify })
-```
+OpenPGP (recommended for node apps)
+- much wider support for different keys
+- LGPL (which probably means you can't bundle it into your application)
+- ~164kb gzipped
 
-## Choices!
-
-You can choose between an [OpenPGP.js plugin](https://github.com/isomorphic-git/openpgp-plugin) and an [isomorphic-pgp plugin](https://github.com/isomorphic-git/pgp-plugin)!
-The OpenPGP one has much wider support for different keys at the cost of a huge bundle size, while the isomorphic-pgp plugin has restricted key support but 1/10th the size.
+isomorphic-pgp (recommended for browser apps)
+- limited types of keys supported
+- MIT
+- ~21k gzipped
 
 ### Implementing your own `onSign` callback
 
@@ -38,42 +34,86 @@ The PGP signing function must implement the following API:
 | **secretKey** | string                         | an 'ASCII armor' encoded PGP key (technically can actually contain _multiple_ keys) |
 | return        | Promise\<{signature: string}\> | an 'ASCII armor' encoded "detached" signature                                       |
 
-### Implementing your own `onVerify` callback
+## Verifying Signatures
+
+To verify signed commits and signed annotated tag objects, you use the signature (`.gpgsig`) and the signing payload (`payload`) as returned from `log`, `readCommit`, or `readTag`.
 
-The PGP verifying function must implement the following API:
+```js
+// Verify a whole bunch of commits
+import { pgp } from '@isomorphic-git/pgp-plugin'
 
-#### async ({ payload, publicKey, signature}) => { valid, invalid }
+let commits = await git.log({ fs, dir, ref: 'master' })
+for (const { commit, payload } of commits) {
+  let { valid, invalid } = await pgp.verify({ payload, publicKey, signature: commit.gpgsig })
+  // valid is a string[] of the valid key ids
+  // invalid is a string[] of the invalid key ids. Ideally this is empty.
+}
+```
 
-| param         | type [= default]                                | description                                                                                                |
-| ------------- | ----------------------------------------------- | ---------------------------------------------------------------------------------------------------------- |
-| **payload**   | string                                          | a plaintext message                                                                                        |
-| **publicKey** | string                                          | an 'ASCII armor' encoded PGP key (technically can actually contain _multiple_ keys)                        |
-| **signature** | string                                          | an 'ASCII armor' encoded PGP "detached" signature (technically can actually contain _multiple_ signatures) |
-| return        | Promise\<{valid: string[], invalid: string[]}\> | two arrays containing the key ids of the valid and invalid signatures respectively                         |
+```js
+// Verify a commit object
+import { pgp } from '@isomorphic-git/pgp-plugin'
 
-For verification, `valid` will contain the key ids of all valid signatures, and `invalid` will contain the key ids of all the invalid signatures.
-For 99% of use cases, there will only be one signature, so one of those arrays will be empty and the other will contain  *the* signature.
-But it is theoretically possible for commits to have multiple signatures.
+let oid = await git.resolveRef({ fs, dir, ref: 'master' })
+let { commit, payload } = await git.readCommit({ fs, dir, oid })
+let { valid, invalid } = await pgp.verify({ payload, publicKey, signature: commit.gpgsig })
+// valid is a string[] of the valid key ids
+// invalid is a string[] of the invalid key ids
+```
 
-### Background
-Cryptographically signing commits via OpenPGP [[1](#footnote1)] is an important feature of git.
-It lets you verify that commits were really created by the stated author.
-Otherwise, anybody could make a commit using someone elses email.
+```js
+// Verify an annotated tag object
+import { pgp } from '@isomorphic-git/pgp-plugin'
+import { resolveRef, readCommit } from 'isomorphic-git'
+
+let oid = await resolveRef({ fs, dir, ref: 'v1.0.0' })
+let { tag, payload } = await readTag({ fs, dir, oid })
+let { valid, invalid } = await pgp.verify({ payload, publicKey, signature: tag.signature })
+// valid is a string[] of the valid key ids
+// invalid is a string[] of the invalid key ids
+```
+
+### A valid signature isn't enough!
+
+Note that simply verifying the signatures are valid is not sufficient to establish _trust_.
+You must also have reason to believe that the `publicKey` really does belong to the person who wrote the commit.
+You must also have a way to find the `publicKey` in the first place!
+
+So how _do_ you get the `publicKey`? Here are two ways, each with serious drawbacks. (Spoiler: there's no standard solution yet.)
+
+One thing you could do would be to use `commit.author.email` and `commit.committer.email`, match those to GitHub usernames (not a trivial task because their emails might be private), and then lookup the PGP key on GitHub. See [`ghkeys`](https://www.npmjs.com/package/ghkeys) for an implementation of username -> PGP key lookup. The downside to this is, it only works for commits, signed by users, who have public emails on GitHub, who have uploaded their PGP keys on GitHub. But on the positive side, you can be pretty sure the PGP key really does belong to that user, because GitHub is acting as the authority. To be extra careful, I think GitHub's API lets you check whether the email address is a verified email address or not.
+
+Another thing you could try is parse the PGP signature, extract the public key ID, and use the key ID to lookup the
+public key on a PGP keyserver like mit.pgp.edu. (Some code to do just that follows this paragraph.) One downside to this is it only works if people bother to upload their key to a PGP keyserver. Another downside to this is there is absolutely no security. Anybody can upload a key claiming to be `johnsmith@aol.com` or whatever. If you look up the PGP key from the signature, you also need to make sure that the email address associated with the publicKey is the same one used in `commit.author.email` (or `commit.committer.email`). If you rely on a public keyserver where anyone can claim to be `johnsmith@aol.com`, then you'll need to exploit the Web-of-Trust (where keys are signed by other keys, which are signed by other keys, and so on until you reach a "trusted key") or use some other way to decide keys are trustworthy.
+
+```js
+const extractKey = (gpgsig) => {
+  const m = Message.parse(gpgsig);
+  for (const p of m.packets) {
+    if (p.tag === 2 /* Signature Packet */) {
+      for (const s of p.packet.unhashed.subpackets) {
+        if (s.type === 16 /* Issuer */) {
+          return s.subpacket.issuer_s
+        }
+      }
+    }
+  }
+}
+
+const lookupKey = async (keyid) => {
+  let text = await (await fetch(`https://pgp.mit.edu/pks/lookup?op=get&search=0x${keyid}`)).text()
+  let matches = text.match(/-----BEGIN PGP PUBLIC KEY BLOCK-----(.|\n)*-----END PGP PUBLIC KEY BLOCK-----/)
+  if (matches) return matches[0]
+}
+```
 
-Including the current state-of-the-art library, `openpgpjs`, directly into `isomorphic-git`
-was impractical, due to the huge size of the library (over 1000KB) and license restrictions
-that probably would make including `openpgpjs` directly in a minified bundle with `isomorphic-git`
-illegal [[2](#footnote2)].
+You could do a "trust on first use" strategy where the first time you see a signed commit by `johnsmith@aol.com` you lookup the public key and save it, and then in the future if a signed commit by `johnsmith@aol.com` uses a different key, show a warning to the user that the key has changed. (This is very similar to the strategy used by SSH - maybe you've seen the famous message `The authenticity of host <IP ADDRESS> can't be established. RSA key fingerprint is <FINGERPRINT>. Are you sure you want to continue connecting (yes/no)?`)
 
-Therefore, `openpgpjs` support is provided by a separate npm package, under an LGPL license to match
-openpgpjs.
+If you're making a more enterprisey, application, you could send the user an email and verify the email that way. You could store the PGP keys that you've verified in a centralized database.
 
-I've also created my own PGP library, `isomorphic-pgp`, to push the envelope of what's possible within realistic bundle size budgets.
+Sadly, these two questions:
 
-#### Footnotes:
-<a name="footnote1">1</a>: [OpenPGP](https://www.openpgp.org/) is the [IETF standardization](https://tools.ietf.org/html/rfc4880) of the original PGP protocol. PGP stands for Pretty Good Privacy. Git's documentation refers to GPG, which is short for [GNU Privacy Guard](https://gnupg.org/), which is a particular implementation of the OpenPGP protocol.
+- Where do I find the public key?
+- Can I trust that the public key really belongs to this email address?
 
-<a name="footnote2">2</a>: The LGPL was written with C in mind, so you have to extrapolate what "statically linked" and "dynamically linked" mean in the JavaScript era).
-My own interpretation is that for OpenPGP.js to be "dynamically linked", end users must have a non-trivial way of swapping out the OpenPGP.js instance used by a website by default, with another version of the library (their "own" version).
-This is nearly impossible if the JavaScript is bundled and minified.
-It's also hard to do on existing websites, but distributing it as a separate file at least makes it doable.
+have no simple answers. However, if you're building a decentralized system where you auto-generate PGP keys for users, I'd recommend saving the public PGP keys in the git repo itself. That seems like an obvious place.
diff --git a/src/api/_index.js b/src/api/_index.js
@@ -49,7 +49,6 @@ export * from './resolveRef.js'
 export * from './status.js'
 export * from './statusMatrix.js'
 export * from './tag.js'
-export * from './verify.js'
 export * from './version.js'
 export * from './walk.js'
 export * from './writeBlob.js'
diff --git a/src/api/annotatedTag.js b/src/api/annotatedTag.js
@@ -25,8 +25,8 @@ import { normalizeAuthorObject } from '../utils/normalizeAuthorObject.js'
  * @param {Date} [args.tagger.date] - Set the tagger timestamp field. Default is the current date.
  * @param {number} [args.tagger.timestamp] - Set the tagger timestamp field. This is an alternative to using `date` using an integer number of seconds since the Unix epoch instead of a JavaScript date object.
  * @param {number} [args.tagger.timezoneOffset] - Set the tagger timezone offset field. This is the difference, in minutes, from the current timezone to UTC. Default is `(new Date()).getTimezoneOffset()`.
- * @param {string} [args.signature] - The signature attatched to the tag object. (Mutually exclusive with the `signingKey` option.)
- * @param {string} [args.signingKey] - Sign the tag object using this private PGP key. (Mutually exclusive with the `signature` option.)
+ * @param {string} [args.gpgsig] - The gpgsig attatched to the tag object. (Mutually exclusive with the `signingKey` option.)
+ * @param {string} [args.signingKey] - Sign the tag object using this private PGP key. (Mutually exclusive with the `gpgsig` option.)
  * @param {boolean} [args.force = false] - Instead of throwing an error if a tag named `ref` already exists, overwrite the existing tag. Note that this option does not modify the original tag object itself.
  *
  * @returns {Promise<void>} Resolves successfully when filesystem operations are complete
@@ -53,7 +53,7 @@ export async function annotatedTag ({
   ref,
   tagger: _tagger,
   message = ref,
-  signature,
+  gpgsig,
   object,
   signingKey,
   force = false
@@ -78,7 +78,7 @@ export async function annotatedTag ({
       ref,
       tagger,
       message,
-      signature,
+      gpgsig,
       object,
       signingKey,
       force
diff --git a/src/api/verify.js b/src/api/verify.js
diff --git a/src/api/writeObject.js b/src/api/writeObject.js
diff --git a/src/commands/annotatedTag.js b/src/commands/annotatedTag.js
diff --git a/src/commands/typedefs.js b/src/commands/typedefs.js
diff --git a/src/models/GitAnnotatedTag.js b/src/models/GitAnnotatedTag.js
diff --git a/src/models/GitCommit.js b/src/models/GitCommit.js
