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 4 commits
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/backend/src/config/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Copy link
Member

Choose a reason for hiding this comment

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

Should the default be something that includes internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

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.

privateKey: parseOrProvisionKey(envString('PRIVATE_KEY_FILE', undefined)),

/** Frontend **/
Expand Down
7 changes: 6 additions & 1 deletion packages/backend/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,13 @@ export function initIocContainer(
return await createOpenAPI(config.authServerSpec)
})
container.singleton('openPaymentsClient', async (deps) => {
const config = await deps.use('config')
const logger = await deps.use('logger')
return createOpenPaymentsClient({ logger })
return createOpenPaymentsClient({
logger,
keyId: config.keyId,
privateKey: config.privateKey
})
})

/**
Expand Down
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
59 changes: 44 additions & 15 deletions 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 @@ -16,33 +17,61 @@ import {
import { createAxiosInstance } from './requests'
import { AxiosInstance } from 'axios'

export interface CreateOpenPaymentClientArgs {
requestTimeoutMs?: number
logger?: Logger
}

export interface ClientDeps {
axiosInstance: AxiosInstance
openApi: OpenAPI
logger: Logger
}

export interface OpenPaymentsClient {
incomingPayment: IncomingPaymentRoutes
ilpStreamConnection: ILPStreamConnectionRoutes
paymentPointer: PaymentPointerRoutes
}

export const createClient = async (
args?: CreateOpenPaymentClientArgs
): Promise<OpenPaymentsClient> => {
const createDeps = async (
args: Partial<CreateOpenPaymentClientArgs>
): Promise<ClientDeps> => {
const axiosInstance = createAxiosInstance({
privateKey: args.privateKey,
keyId: args.keyId,
requestTimeoutMs:
args?.requestTimeoutMs ?? config.DEFAULT_REQUEST_TIMEOUT_MS
})
const openApi = await createOpenAPI(config.OPEN_PAYMENTS_OPEN_API_URL)
const logger = args?.logger ?? createLogger()
const deps = { axiosInstance, openApi, logger }
return { axiosInstance, openApi, logger }
}

export interface CreateUnauthenticatedClientArgs {
requestTimeoutMs?: number
logger?: Logger
}

export interface UnauthenticatedClient {
ilpStreamConnection: ILPStreamConnectionRoutes
paymentPointer: PaymentPointerRoutes
}

export const createUnauthenticatedClient = async (
args: CreateOpenPaymentClientArgs
): Promise<UnauthenticatedClient> => {
const deps = await createDeps(args)

return {
ilpStreamConnection: createILPStreamConnectionRoutes(deps),
paymentPointer: createPaymentPointerRoutes(deps)
}
}

export interface CreateOpenPaymentClientArgs
Copy link
Member

Choose a reason for hiding this comment

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

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

extends CreateUnauthenticatedClientArgs {
privateKey: KeyLike
keyId: string
}

export interface OpenPaymentsClient extends UnauthenticatedClient {
incomingPayment: IncomingPaymentRoutes
}

export const createClient = async (
Copy link
Member

Choose a reason for hiding this comment

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

And then I'd call this createAuthenticatedClient.

args: CreateOpenPaymentClientArgs
): Promise<OpenPaymentsClient> => {
const deps = await createDeps(args)

return {
incomingPayment: createIncomingPaymentRoutes(deps),
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
31 changes: 27 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,35 @@ 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,
headers: config.headers,
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.