feat(auth): httpsig request validation #387

njlie · 2022-06-29T23:58:32Z

Changes proposed in this pull request

Adds httpsig validation to requests where the client must present a public key.

Context

Closes #274.

Bumped jest to 28 to fix: jestjs/jest#10221 (comment), which was affecting one of the tests using fake-timers.
https://github.com/facebook/jest/releases/tag/v28.0.0

[jest-config] [BREAKING] Rename config option name to id (fix: rename config option name to id jestjs/jest#11981)

Checklist

Related issues linked using fixes #number
Tests added/updated
Documentation added
Make sure that all checks pass

matdehaast · 2022-07-20T09:01:08Z

packages/auth/src/accessToken/routes.ts

+  try {
+    const sig = ctx.headers['signature']
+    const sigInput = ctx.headers['signature-input']
+
+    if (
+      !sig ||
+      !sigInput ||
+      typeof sig !== 'string' ||
+      typeof sigInput !== 'string'
+    ) {
+      ctx.status = 400
+      ctx.body = {
+        error: 'invalid_request'
+      }
+      return
+    }
+
+    const verified = await deps.clientService.verifySigFromBoundKey(
+      sig,
+      sigInput,
+      body['access_token'],
+      ctx
    )
-    if (introspectionResult) {
-      ctx.body = introspectionToBody(introspectionResult)
+    if (!verified) {
+      ctx.status = 401
+      ctx.body = {
+        error: 'invalid_client'
+      }
+    }
+  } catch (err) {
+    if ((err as Error).name === 'InvalidSigInputError') {
+      ctx.status = 400
+      ctx.body = {
+        error: 'invalid_request'
+      }
+      return
    } else {
-      return ctx.throw(404, 'token not found')
+      ctx.status = 401
+      ctx.body = {
+        error: 'invalid_client'
+      }
+      return
    }
+  }


Should we consider extracting this logic into a helper function so we don't have to duplicate this everywhere?

Indeed, this should be better as a middleware we could place in front of the relevant routes. Will look into changing that.

packages/auth/src/grant/service.ts

packages/auth/src/grant/routes.ts

wilsonianb · 2022-08-03T19:21:23Z

package.json

    "lint-staged": ">=10",
    "prettier": "2.2.1",
-    "ts-jest": "^26.5.5",
+    "ts-jest": "^28.0.5",


I'm seeing this message when running tests:

ts-jest[versions] (WARN) Version 4.2.4 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=4.3.0 <5.0.0-0). Please do not report issues in ts-jest if you are using unsupported versions.

packages/auth/src/client/service.ts

packages/auth/src/grant/routes.ts

wilsonianb · 2022-08-03T19:52:32Z

packages/auth/src/app.ts

@@ -168,12 +169,15 @@ export class App {
    for (const path in openApi.paths) {
      for (const method in openApi.paths[path]) {
        if (isHttpMethod(method)) {
+          let useHttpSigMiddleware = false
          let route: (ctx: AppContext) => Promise<void>
          if (path.includes('continue')) {


Why no useHttpSigMiddleware for continuation requests?

When used for delegation in GNAP, these key binding mechanisms allow the AS to ensure that the keys presented by the client instance in the initial request are in control of the party calling any follow-up or continuation requests. To facilitate this requirement, the continuation response ([Section 3.1](https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol#section-3.1)) includes an access token bound to the client instance's key ([Section 2.3](https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol#section-2.3)), and that key (or its most recent rotation) MUST be proved in all continuation requests [Section 5](https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol#section-5). Token management requests [Section 6](https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol#section-6) are similarly bound to either the access token's own key or, in the case of bearer tokens, the client instance's key.

https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol#section-7.3

wilsonianb · 2022-08-03T20:07:15Z

packages/auth/src/accessToken/routes.ts

+  if (!body['access_token']) {
+    ctx.status = 400
+    ctx.body = {
+      error: 'invalid_request',


Are these error responses only necessary for the client (not the RO)?

It's only for the client. Though looking at the spec, it does seem like these are only necessary during the grant authorization flow and not necessarily for token management endpoints.

Yeah, I was thinking that since the RO would be sending token introspection requests that these error responses don't have to be used.

Isn't that covered by checking against the spec?

packages/auth/src/client/service.ts

wilsonianb · 2022-08-08T19:42:11Z

packages/auth/src/client/service.ts

+  } else if (path.includes('/token') && method === 'DELETE') {
+    keyName = 'managementId'
+    value = ctx.params['managementId']
+  } else {


Should continuation requests be included here?
#387 (comment)

Oh yeah, they should. Thanks for pointing that out.

Added now in: https://github.com/interledger/rafiki/blob/nl-httpsig/packages/auth/src/client/service.ts#L357-L370

wilsonianb · 2022-08-08T19:48:37Z

packages/auth/package.json

@@ -44,6 +44,6 @@
    "jest-openapi": "^0.14.2",
    "nock": "^13.2.4",
    "openapi-types": "^12.0.0",
-    "typescript": "^4.2.4"
+    "typescript": "^4.3.0"


I'm still getting the warning and so is GitHub Actions
#387 (comment)
https://github.com/interledger/rafiki/runs/7682195245?check_suite_focus=true

I wonder if typescript should be updated in the root package.json

I'll try it and see what happens

Bumping typescript breaks a lot of stuff

That doesn't make any sense because ^4.2.4 actually installs 4.7.2 currently as the caret means any non breaking changes.

wilsonianb · 2022-08-15T15:23:04Z

packages/auth/src/app.ts

-      ctx.status = 200
-    })
-
-    this.publicRouter.del('/auth/token/:id', accessTokenRoutes.revoke)


Why was this removed?

I guess because we have this section further up that also includes checking against the openapi spec (L168 - L1206). In the end, all of those individual routes should be moved into the block that also checks the openapi spec.

It is redundant to this block of code now. I originally stubbed it out when scaffolding the auth server but it didn't get removed once OpenApi was added.

wilsonianb · 2022-08-15T15:24:50Z

packages/auth/src/client/service.test.ts

+    // afterAll(async (): Promise<void> => {
+    //   nock.restore()
+    // })


wilsonianb · 2022-08-15T15:27:22Z

packages/auth/src/client/service.ts

+
+  const { body } = ctx.request
+  const { path, method } = ctx
+  // TODO: replace with HttpMethod types instead of string literals


wilsonianb · 2022-08-15T19:33:10Z

packages/auth/src/accessToken/routes.test.ts

+      const requestBody = {
+        access_token: v4(),
+        proof: 'httpsig',
+        resource_server: 'test'
+      }
+      const { signature, sigInput, contentDigest } = await generateSigHeaders(
+        url,
+        method,
+        requestBody
+      )
      const ctx = createContext(


Maybe you could add a wrapper for createContext that takes url, method, requestBody, etc as parameters and does generateSigHeaders + createContext instead of having to do this full setup in all these tests.

I think we can actually just remove it since the test doesn't run the middleware.

packages/auth/jest.config.js

sabineschaller · 2022-08-16T08:00:36Z

packages/auth/src/accessToken/routes.ts

+  if (!body['access_token']) {
+    ctx.status = 400
+    ctx.body = {
+      error: 'invalid_request',


Isn't that covered by checking against the spec?

sabineschaller · 2022-08-16T08:05:43Z

packages/auth/migrations/20220504163005_create_access_token_table.js

@@ -2,7 +2,7 @@ exports.up = function (knex) {
  return knex.schema.createTable('accessTokens', function (table) {
    table.uuid('id').notNullable().primary()
    table.string('value').notNullable().unique()
-    table.string('managementId').notNullable()
+    table.string('managementId').notNullable().unique()


Why do we introduce that? The id is already unique, so why adding another unique id?

I figured it would be the right practice to expose an external id instead of an internal database id. I should update it to a .uuid() instead of a .string() though since in practice it's always a uuid/v4.

I should update it to a .uuid() instead of a .string() though since in practice it's always a uuid/v4.

bump

sabineschaller · 2022-08-16T08:09:15Z

packages/auth/src/app.ts

-      ctx.status = 200
-    })
-
-    this.publicRouter.del('/auth/token/:id', accessTokenRoutes.revoke)


I guess because we have this section further up that also includes checking against the openapi spec (L168 - L1206). In the end, all of those individual routes should be moved into the block that also checks the openapi spec.

packages/auth/src/client/service.ts

github-actions bot added pkg: auth Changes in the GNAP auth package. type: source Changes business logic type: tests Testing related labels Jun 29, 2022

njlie force-pushed the nl-httpsig branch from 7af4265 to e2bdc7e Compare June 30, 2022 00:05

github-actions bot added pkg: backend Changes in the backend package. pkg: openapi pkg: rates Changes in the rates package. labels Jul 1, 2022

njlie force-pushed the nl-httpsig branch 2 times, most recently from 807b8dd to 739fa38 Compare July 5, 2022 18:01

njlie force-pushed the nl-httpsig branch from 7d688c9 to f9081f6 Compare July 13, 2022 21:49

njlie requested review from sabineschaller, dclipp and matdehaast July 13, 2022 22:14

njlie marked this pull request as ready for review July 13, 2022 22:40

matdehaast reviewed Jul 20, 2022

View reviewed changes

njlie force-pushed the nl-httpsig branch 2 times, most recently from c5147fd to 30c65a8 Compare August 1, 2022 20:56

njlie requested review from matdehaast and wilsonianb August 2, 2022 21:15

wilsonianb reviewed Aug 3, 2022

View reviewed changes

njlie requested a review from wilsonianb August 8, 2022 19:10

wilsonianb reviewed Aug 8, 2022

View reviewed changes

njlie requested a review from wilsonianb August 15, 2022 14:27

wilsonianb mentioned this pull request Aug 15, 2022

Update Typescript to >=4.3.0 #412

Closed

wilsonianb reviewed Aug 15, 2022

View reviewed changes

njlie force-pushed the nl-httpsig branch 3 times, most recently from 0f8a049 to 16deb5b Compare August 16, 2022 08:20

sabineschaller reviewed Aug 16, 2022

View reviewed changes

njlie requested a review from wilsonianb August 16, 2022 11:54

njlie added 22 commits August 24, 2022 14:58

fix: tests

eff5c1f

fix: rebase conflicts

d15a329

feat: review comments

06927d7

chore: bump typescript

b65208a

feat: middleware on /continue endpoint

ec80231

chore: bump typescript at project root

7518f22

chore: rollback typescript bump at root

7e5b5c0

feat: remove comments, cleanup TODO, rely on openapi for validation

ea3c56a

feat: clean up tests

5db1bd1

fix: build error

c7d6fa0

feat: remove .yarn

dffed48

chore: remove unnecessary calls from tests

b702055

fix: convert managementId to uuid

94d2950

chore: remove yarn.lock

91b41d2

chore: prettier

17d445d

chore: rename variables that were breaking tests for some reason

d3909e2

fix: build errors

d0ab02b

feat: dynamically generate keys for tests

2c318ed

chore: clean up unused deps, fix build errors

c65ccc0

fix: typos, consistent current date

8b6a1b5

feat: abstract direct model queries, create signature service

6516c7c

feat: fix issues from rebase

1e3d0a8

njlie force-pushed the nl-httpsig branch from 041ffc4 to 1e3d0a8 Compare August 24, 2022 16:08

njlie requested review from wilsonianb, sabineschaller and matdehaast August 24, 2022 16:09

matdehaast approved these changes Aug 24, 2022

View reviewed changes

njlie merged commit 872c877 into main Aug 25, 2022

njlie deleted the nl-httpsig branch August 25, 2022 07:20

wilsonianb mentioned this pull request Aug 29, 2022

Add httpsig to RS token introspection requests #576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): httpsig request validation #387

feat(auth): httpsig request validation #387

njlie commented Jun 29, 2022 •

edited

Loading

matdehaast Jul 20, 2022

njlie Jul 20, 2022 •

edited

Loading

wilsonianb Aug 3, 2022

wilsonianb Aug 3, 2022

wilsonianb Aug 3, 2022

njlie Aug 4, 2022

wilsonianb Aug 4, 2022

sabineschaller Aug 16, 2022

wilsonianb Aug 8, 2022

njlie Aug 8, 2022

njlie Aug 22, 2022

wilsonianb Aug 8, 2022

njlie Aug 8, 2022

njlie Aug 15, 2022

matdehaast Aug 23, 2022

wilsonianb Aug 15, 2022

sabineschaller Aug 16, 2022

njlie Aug 16, 2022

wilsonianb Aug 15, 2022

wilsonianb Aug 15, 2022

wilsonianb Aug 15, 2022

njlie Aug 16, 2022

sabineschaller Aug 16, 2022

sabineschaller Aug 16, 2022

njlie Aug 16, 2022

wilsonianb Aug 19, 2022

sabineschaller Aug 16, 2022

feat(auth): httpsig request validation #387

feat(auth): httpsig request validation #387

Conversation

njlie commented Jun 29, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

Choose a reason for hiding this comment

njlie Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njlie commented Jun 29, 2022 •

edited

Loading

njlie Jul 20, 2022 •

edited

Loading