feat(open-payments): add HTTP message signatures #722

wilsonianb · 2022-11-01T21:39:39Z

Changes proposed in this pull request

Require private key and key id for ~~createClient~~createAuthenticatedClient
Add createUnauthenticatedClient limited to payment pointer and connection requests, which do not require 👆

Context

fixes Generate httpsig for internal OP calls #587

Checklist

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

sabineschaller · 2022-11-16T13:03:57Z

packages/open-payments/src/client/requests.test.ts

-            Authorization: 'GNAP accessToken',
-            Signature: 'TODO',
-            'Signature-Input': 'TODO'
+            Authorization: 'GNAP accessToken'


Can we add some sort of exception for this?

Do you mean for codeql?
We could open an issue to exclude test files in https://github.com/interledger/rafiki/blob/main/.github/workflows/codeql-analysis.yml

Let's do that.

Ignore CodeQL hardcoded-credentials in tests #750

wilsonianb · 2022-11-01T21:57:13Z

packages/open-payments/src/client/requests.test.ts

+          )};keyid="${keyId}";alg="ed25519"`
+        )
+        .get('/incoming-payment')
+        // TODO: verify signature


Should we move the meat of auth's signature validation into open-payments (after #672 is merged)?

What do you mean by open-payments?

https://github.com/interledger/rafiki/tree/main/packages/open-payments

Does that make sense to add it in here though? I feel like sig validation is done on the "receiver" side , but the client is more of the "caller" in our scenario

I'm thinking the "receiver"/RS will also be using open-payments at least for the types (why I was reluctant to call open-payments a "client" library), so why not also provide httpsig verification?

@wilsonianb wouldn't the RS also use it for sig creation when requesting resources at other RS's?

Correct, the RS will also use ~~createClient~~createAuthenticatedClient to send (signed) requests.

Ah ok, the receiver will just import the lib and use some of those sig verification methods as part of the middleware (how it's done now) -> makes sense

packages/open-payments/src/client/signatures.ts

packages/open-payments/src/client/requests.ts

mkurapov · 2022-11-04T16:14:09Z

packages/open-payments/src/client/requests.ts

+    },
+    null,
+    {
+      runWhen: (config) => !!config.headers['Authorization']


Because we don't need Authorization header on the AS calls, this would effectively handle both AS and RS with no problem, correct?

I think there are some AS calls that require the Authorization header
https://datatracker.ietf.org/doc/html/draft-ietf-gnap-core-protocol#section-7.3.1.1

I was actually thinking that the grant initiation request might be the only one with it, but I'm not sure

Does that mean that the auth server open payment specs are missing the signature-input and signature header parameters in the open api spec files? If they need to provide an Authorization token?

Yeah, looks like that's the case.
I just opened:

Add signature headers to AS endpoints open-payments#209

@wilsonianb for my own understanding, wouldn't grant initiation be the only AS request without the Authorization: GNAP {accessToken} header?

yeah, I think that's correct

mkurapov · 2022-11-04T16:14:28Z

packages/open-payments/src/client/requests.ts

  axiosInstance.defaults.headers.common['Content-Type'] = 'application/json'

+  axiosInstance.interceptors.request.use(


Confirming: does this apply to all GET, POST, DELETE requests?

I believe so, but it can be filtered.

wilsonianb · 2022-11-08T00:29:16Z

packages/open-payments/src/client/index.ts

+  privateKey: KeyLike
+  keyId: string


Update AS to query client keys at payment pointer jwks #731

Thinking about 👆, if there are clients that only want to use open-payments to query payment pointers and/or their keys and/or connections, it seems like it should be possible to create a client without requiring privateKey/keyId.

Should open-payments createClient be split into:

createAuthenticatedClient

createUnauthenticatedClient

?
The latter would return the subset of requests that are possible without httpsig.

@wilsonianb the explicit split makes sense to me

mkurapov · 2022-11-08T01:05:43Z

packages/open-payments/src/client/requests.ts

+          method: config.method.toUpperCase(),
+          url: config.url,
+          // https://github.com/axios/axios/issues/5089#issuecomment-1297761617
+          headers: JSON.parse(JSON.stringify(config.headers)),


What are the concerns with using just config.headers ?

At some point, I was getting the error described in that axios github issue.
But now I can't reproduce. (Tests pass with `config.headers)
I'll tentatively remove the workaround.

mkurapov · 2022-11-10T18:56:51Z

packages/open-payments/src/client/index.ts

+  privateKey: KeyLike
+  keyId: string


@wilsonianb the explicit split makes sense to me

mkurapov · 2022-11-11T19:10:23Z

packages/open-payments/src/client/requests.test.ts

+          )};keyid="${keyId}";alg="ed25519"`
+        )
+        .get('/incoming-payment')
+        // TODO: verify signature


Does that make sense to add it in here though? I feel like sig validation is done on the "receiver" side , but the client is more of the "caller" in our scenario

Remove Axios headers workaround.

sabineschaller · 2022-11-16T12:50:35Z

packages/backend/src/config/app.ts

@@ -112,6 +112,7 @@ export const Config = {
    'AUTH_SERVER_SPEC',
    'https://raw.githubusercontent.com/interledger/open-payments/77462cd0872be8d0fa487a4b233defe2897a7ee4/auth-server-open-api-spec.yaml'
  ),
+  keyId: envString('KEY_ID', 'rafiki'),


Should the default be something that includes internal?

Can you elaborate?

I was just wondering if rafiki is a good value for the default. But now that I have thought a bit more about it, I think it is.

sabineschaller · 2022-11-16T12:57:35Z

packages/open-payments/src/client/index.ts

+  }
+}
+
+export interface CreateOpenPaymentClientArgs


Should we call this CreateAuthenticatedClientArgs? I think it makes it more readable.

sabineschaller · 2022-11-16T12:58:42Z

packages/open-payments/src/client/index.ts

+  incomingPayment: IncomingPaymentRoutes
+}
+
+export const createClient = async (


And then I'd call this createAuthenticatedClient.

sabineschaller · 2022-11-16T13:03:35Z

packages/open-payments/src/client/requests.test.ts

+          )};keyid="${keyId}";alg="ed25519"`
+        )
+        .get('/incoming-payment')
+        // TODO: verify signature


@wilsonianb wouldn't the RS also use it for sig creation when requesting resources at other RS's?

sabineschaller · 2022-11-16T13:03:57Z

packages/open-payments/src/client/requests.test.ts

-            Authorization: 'GNAP accessToken',
-            Signature: 'TODO',
-            'Signature-Input': 'TODO'
+            Authorization: 'GNAP accessToken'


Can we add some sort of exception for this?

mkurapov · 2022-11-16T19:53:27Z

packages/open-payments/src/client/signatures.ts

+    components.push('authorization')
+  }
+  if (request.body) {
+    // TODO: 'content-digest'


Might be good to reference #655 in the comment, if we do that at all?

mkurapov · 2022-11-16T20:02:18Z

packages/open-payments/src/client/signatures.ts

+  keyId
+}: SignOptions): Promise<SignatureHeaders> => {
+  const components = ['@method', '@target-uri']
+  if (request.headers['Authorization']) {


do we care about casing on Authorization ?

It appears to matter.
The sets headers properly if accessToken provided test fails if I make this lower case.

mkurapov · 2022-11-16T20:22:57Z

packages/open-payments/src/client/requests.test.ts

+          )};keyid="${keyId}";alg="ed25519"`
+        )
+        .get('/incoming-payment')
+        // TODO: verify signature


Ah ok, the receiver will just import the lib and use some of those sig verification methods as part of the middleware (how it's done now) -> makes sense

sabineschaller · 2022-11-17T08:40:53Z

packages/backend/src/config/app.ts

@@ -112,6 +112,7 @@ export const Config = {
    'AUTH_SERVER_SPEC',
    'https://raw.githubusercontent.com/interledger/open-payments/77462cd0872be8d0fa487a4b233defe2897a7ee4/auth-server-open-api-spec.yaml'
  ),
+  keyId: envString('KEY_ID', 'rafiki'),


I was just wondering if rafiki is a good value for the default. But now that I have thought a bit more about it, I think it is.

feat(open-payments): add HTTP message signatures

5da5664

github-actions bot added pkg: open-payments type: source Changes business logic type: tests Testing related labels Nov 1, 2022

github-advanced-security bot found potential problems Nov 1, 2022

View reviewed changes

wilsonianb marked this pull request as ready for review November 1, 2022 21:54

wilsonianb requested review from mkurapov, dclipp and njlie November 1, 2022 21:54

wilsonianb commented Nov 1, 2022

View reviewed changes

sabineschaller reviewed Nov 3, 2022

View reviewed changes

packages/open-payments/src/client/signatures.ts Outdated Show resolved Hide resolved

packages/open-payments/src/client/requests.ts Show resolved Hide resolved

wilsonianb mentioned this pull request Nov 3, 2022

663/mk/use open payments client #706

Merged

4 tasks

mkurapov reviewed Nov 4, 2022

View reviewed changes

wilsonianb added 2 commits November 4, 2022 15:38

Merge branch 'main' into bw-httpsig

02d25b9

chore(backend): construct client with privateKey + keyId

97db6d0

github-actions bot added the pkg: backend Changes in the backend package. label Nov 4, 2022

wilsonianb requested review from mkurapov and sabineschaller November 4, 2022 21:41

wilsonianb commented Nov 8, 2022

View reviewed changes

mkurapov reviewed Nov 11, 2022

View reviewed changes

chore(open-payments): add unauthenticated client

eedca17

Remove Axios headers workaround.

wilsonianb requested a review from mkurapov November 15, 2022 14:45

sabineschaller reviewed Nov 16, 2022

View reviewed changes

chore(open-payments): rename to (create)AuthenticatedClient

9e779f2

wilsonianb requested a review from sabineschaller November 16, 2022 14:15

mkurapov previously approved these changes Nov 16, 2022

View reviewed changes

chore(open-payments): reference content-digest issue

039445c

wilsonianb dismissed mkurapov’s stale review via 039445c November 16, 2022 22:05

mkurapov approved these changes Nov 16, 2022

View reviewed changes

sabineschaller approved these changes Nov 17, 2022

View reviewed changes

wilsonianb merged commit 53c4a33 into main Nov 17, 2022

wilsonianb deleted the bw-httpsig branch November 17, 2022 14:11

wilsonianb mentioned this pull request Nov 23, 2022

feat(auth): validate content-digest header #777

Merged

4 tasks

mkurapov mentioned this pull request Nov 28, 2022

Create Open Payments client library #663

Closed

		axiosInstance.defaults.headers.common['Content-Type'] = 'application/json'

		axiosInstance.interceptors.request.use(

feat(open-payments): add HTTP message signatures #722

feat(open-payments): add HTTP message signatures #722

Conversation

wilsonianb commented Nov 1, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

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

wilsonianb Nov 16, 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

wilsonianb Nov 8, 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

wilsonianb commented Nov 1, 2022 •

edited

Loading

wilsonianb Nov 16, 2022 •

edited

Loading

wilsonianb Nov 8, 2022 •

edited

Loading