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

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

Merged
merged 6 commits into from
Nov 17, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/open-payments/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
},
"dependencies": {
"axios": "^1.1.2",
"http-message-signatures": "^0.1.2",
"openapi": "workspace:../openapi",
"pino": "^8.4.2"
}
Expand Down
7 changes: 6 additions & 1 deletion packages/open-payments/src/client/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { KeyLike } from 'crypto'
import { createOpenAPI, OpenAPI } from 'openapi'
import createLogger, { Logger } from 'pino'
import config from '../config'
Expand All @@ -19,6 +20,8 @@ import { AxiosInstance } from 'axios'
export interface CreateOpenPaymentClientArgs {
requestTimeoutMs?: number
logger?: Logger
privateKey: KeyLike
keyId: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@wilsonianb wilsonianb Nov 8, 2022

Choose a reason for hiding this comment

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

Should open-payments createClient be split into:

  • createAuthenticatedClient
  • createUnauthenticatedClient

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@wilsonianb the explicit split makes sense to me

}

export interface ClientDeps {
Expand All @@ -34,9 +37,11 @@ export interface OpenPaymentsClient {
}

export const createClient = async (
args?: CreateOpenPaymentClientArgs
args: CreateOpenPaymentClientArgs
): Promise<OpenPaymentsClient> => {
const axiosInstance = createAxiosInstance({
privateKey: args.privateKey,
keyId: args.keyId,
requestTimeoutMs:
args?.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS
})
Expand Down
61 changes: 51 additions & 10 deletions packages/open-payments/src/client/requests.test.ts
Original file line number Diff line number Diff line change
@@ -1,37 +1,73 @@
/* eslint-disable @typescript-eslint/no-empty-function */
import { createAxiosInstance, get } from './requests'
import { generateKeyPairSync } from 'crypto'
import nock from 'nock'
import { mockOpenApiResponseValidators, silentLogger } from '../test/helpers'

describe('requests', (): void => {
const logger = silentLogger
const privateKey = generateKeyPairSync('ed25519').privateKey
const keyId = 'myId'

describe('createAxiosInstance', (): void => {
test('sets timeout properly', async (): Promise<void> => {
expect(
createAxiosInstance({ requestTimeoutMs: 1000 }).defaults.timeout
createAxiosInstance({ requestTimeoutMs: 1000, privateKey, keyId })
.defaults.timeout
).toBe(1000)
})
test('sets Content-Type header properly', async (): Promise<void> => {
expect(
createAxiosInstance({ requestTimeoutMs: 0 }).defaults.headers.common[
'Content-Type'
]
createAxiosInstance({ requestTimeoutMs: 0, privateKey, keyId }).defaults
.headers.common['Content-Type']
).toBe('application/json')
})
})

describe('get', (): void => {
const axiosInstance = createAxiosInstance({ requestTimeoutMs: 0 })
const axiosInstance = createAxiosInstance({
requestTimeoutMs: 0,
privateKey,
keyId
})
const baseUrl = 'http://localhost:1000'
const responseValidators = mockOpenApiResponseValidators()

beforeAll(() => {
jest.spyOn(axiosInstance, 'get')
})

afterEach(() => {
jest.useRealTimers()
})

test('sets headers properly if accessToken provided', async (): Promise<void> => {
nock(baseUrl).get('/incoming-payment').reply(200)
// https://github.com/nock/nock/issues/2200#issuecomment-1280957462
jest
.useFakeTimers({
doNotFake: [
'nextTick',
'setImmediate',
'clearImmediate',
'setInterval',
'clearInterval',
'setTimeout',
'clearTimeout'
]
})
.setSystemTime(new Date())

const scope = nock(baseUrl)
.matchHeader('Signature', /sig1=:([a-zA-Z0-9+/]){86}==:/)
.matchHeader(
'Signature-Input',
`sig1=("@method" "@target-uri" "authorization");created=${Math.floor(
Date.now() / 1000
)};keyid="${keyId}";alg="ed25519"`
)
.get('/incoming-payment')
// TODO: verify signature
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by open-payments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@wilsonianb wilsonianb Nov 16, 2022

Choose a reason for hiding this comment

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

Correct, the RS will also use createClientcreateAuthenticatedClient to send (signed) requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

.reply(200)

await get(
{ axiosInstance, logger },
Expand All @@ -42,20 +78,24 @@ describe('requests', (): void => {
responseValidators.successfulValidator
)

scope.done()

expect(axiosInstance.get).toHaveBeenCalledWith(
`${baseUrl}/incoming-payment`,
{
headers: {
Authorization: 'GNAP accessToken',
Signature: 'TODO',
'Signature-Input': 'TODO'
Authorization: 'GNAP accessToken'

Check failure

Code scanning / CodeQL

Hard-coded credentials

The hard-coded value "GNAP accessToken" is used as [authorization header](1).
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some sort of exception for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}
)
})

test('sets headers properly if accessToken is not provided', async (): Promise<void> => {
nock(baseUrl).get('/incoming-payment').reply(200)
const scope = nock(baseUrl)
.matchHeader('Signature', (sig) => sig === undefined)
.matchHeader('Signature-Input', (sigInput) => sigInput === undefined)
.get('/incoming-payment')
.reply(200)

await get(
{ axiosInstance, logger },
Expand All @@ -64,6 +104,7 @@ describe('requests', (): void => {
},
responseValidators.successfulValidator
)
scope.done()

expect(axiosInstance.get).toHaveBeenCalledWith(
`${baseUrl}/incoming-payment`,
Expand Down
32 changes: 28 additions & 4 deletions packages/open-payments/src/client/requests.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import axios, { AxiosInstance } from 'axios'
import { KeyLike } from 'crypto'
import { ResponseValidator } from 'openapi'
import { ClientDeps } from '.'
import { createSignatureHeaders } from './signatures'

interface GetArgs {
url: string
Expand All @@ -26,9 +28,7 @@ export const get = async <T>(
const { data, status } = await axiosInstance.get(url, {
headers: accessToken
? {
Authorization: `GNAP ${accessToken}`,
Signature: 'TODO',
'Signature-Input': 'TODO'
Authorization: `GNAP ${accessToken}`
}
: {}
})
Expand Down Expand Up @@ -65,12 +65,36 @@ export const get = async <T>(

export const createAxiosInstance = (args: {
requestTimeoutMs: number
privateKey: KeyLike
keyId: string
sabineschaller marked this conversation as resolved.
Show resolved Hide resolved
}): AxiosInstance => {
const axiosInstance = axios.create({
timeout: args.requestTimeoutMs
})

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

axiosInstance.interceptors.request.use(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so, but it can be filtered.

async (config) => {
const sigHeaders = await createSignatureHeaders({
request: {
method: config.method.toUpperCase(),
url: config.url,
// https://github.com/axios/axios/issues/5089#issuecomment-1297761617
headers: JSON.parse(JSON.stringify(config.headers)),
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the concerns with using just config.headers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

body: config.data
},
privateKey: args.privateKey,
keyId: args.keyId
})
config.headers['Signature'] = sigHeaders['Signature']
config.headers['Signature-Input'] = sigHeaders['Signature-Input']
return config
},
null,
{
runWhen: (config) => !!config.headers['Authorization']
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think that's correct

}
)

return axiosInstance
}
52 changes: 52 additions & 0 deletions packages/open-payments/src/client/signatures.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { sign, KeyLike } from 'crypto'
import {
httpis as httpsig,
Algorithm,
RequestLike,
Signer
} from 'http-message-signatures'

interface SignOptions {
request: RequestLike
privateKey: KeyLike
keyId: string
}

interface SignatureHeaders {
Signature: string
'Signature-Input': string
}

const createSigner = (privateKey: KeyLike): Signer => {
const signer = async (data: Buffer) => sign(null, data, privateKey)
signer.alg = 'ed25519' as Algorithm
return signer
}

export const createSignatureHeaders = async ({
request,
privateKey,
keyId
}: SignOptions): Promise<SignatureHeaders> => {
const components = ['@method', '@target-uri']
if (request.headers['Authorization']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we care about casing on Authorization ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

components.push('content-length', 'content-type')
}
const { headers } = await httpsig.sign(request, {
components,
parameters: {
created: Math.floor(Date.now() / 1000)
},
keyId,
signer: createSigner(privateKey),
format: 'httpbis'
})
return {
Signature: headers['Signature'] as string,
'Signature-Input': headers['Signature-Input'] as string
}
}
sabineschaller marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 8 additions & 1 deletion packages/open-payments/src/test/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { generateKeyPairSync } from 'crypto'
import createLogger from 'pino'
import { createAxiosInstance } from '../client/requests'
import { ILPStreamConnection, IncomingPayment } from '../types'
Expand All @@ -9,7 +10,13 @@ export const silentLogger = createLogger({
level: 'silent'
})

export const defaultAxiosInstance = createAxiosInstance({ requestTimeoutMs: 0 })
export const keyId = 'default-key-id'

export const defaultAxiosInstance = createAxiosInstance({
requestTimeoutMs: 0,
keyId,
privateKey: generateKeyPairSync('ed25519').privateKey
})

export const mockOpenApiResponseValidators = () => ({
successfulValidator: ((data: unknown): data is unknown =>
Expand Down
6 changes: 6 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.