feat(backend,frontend,mock-ase): use hmac signature to secure admin api #2632
Conversation
github-actions
bot
added
pkg: backend
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
njlie
force-pushed
the
nl/2218/admin-api-security-signature
branch
2 times, most recently
from
92708f1 to
5474689
Compare
njlie
force-pushed
the
nl/2218/admin-api-security-signature
branch
from
db0ea0b to
08c0fe9
Compare
njlie
requested review from
BlairCurrey,
mkurapov,
sabineschaller and
JoblersTune
| .replace(/{{([A-Za-z]\w+)}}/g, (_, key) => bru.getEnvVar(key)) | ||
| .replace( | ||
| /{{([A-Za-z]\w+)}}/g, | ||
| (_, key) => bru.getVar(key) || bru.getEnvVar(key) |
There was a problem hiding this comment.
Bruno Collection variables take precedence over Bruno Environment Variables, per the docs.
There was a problem hiding this comment.
But we don't have collection variables, do we?
There was a problem hiding this comment.
I didn't check every API request for this, but Deposit Asset Liquidity definitely does with transferId and idempotencyKey.
| @@ -138,6 +138,8 @@ export const Config = { | |||
| signatureSecret: process.env.SIGNATURE_SECRET, // optional | |||
| signatureVersion: envInt('SIGNATURE_VERSION', 1), | |||
|
|
|||
| apiSecret: process.env.API_SECRET, // optional | |||
There was a problem hiding this comment.
My understanding is that the apiSecret is an optional environment variable that secures the admin api if provided, and if not, it's unsecured. I'm just wondering if we want it to be secure by default instead? I guess this way is more backwards compatible I just find my initial assumptions were secure by default.
Noticed when I was surprised the integration tests didn't fail.
There was a problem hiding this comment.
I couldn't remember where I heard this, but I had thought I'd heard at some point that securing the admin apio should be optional.
There was a problem hiding this comment.
Yeah I recall that as well. Just wondering if it should be opt-in or opt-out. It's not necessarily a suggestion, just thinking out loud.
I generally assume people should have to explicitly opt-out of the more secure way but perhaps in this specific case it doesnt matter? Since its currently insecure and not meant to be exposed to the internet. I think the configuration for opt-in (how you have it now) is probably simpler than enabling by default (by expecting apiSecret unless some additional env var is set to bypass it or something).
| const authLink = setContext((request, { headers }) => { | ||
| if (!process.env.SIGNATURE_SECRET || !process.env.SIGNATURE_VERSION) | ||
| return { headers } | ||
| const timestamp = Math.round(new Date().getTime() / 1000) | ||
| const version = process.env.SIGNATURE_VERSION | ||
|
|
||
| const { query, variables, operationName } = request | ||
| const formattedRequest = { | ||
| variables, | ||
| operationName, | ||
| query: print(query) | ||
| } | ||
|
|
||
| const payload = `${timestamp}.${canonicalize(formattedRequest)}` | ||
| const hmac = createHmac('sha256', process.env.SIGNATURE_SECRET) | ||
| hmac.update(payload) | ||
| const digest = hmac.digest('hex') | ||
|
|
||
| return { | ||
| headers: { | ||
| ...headers, | ||
| signature: `t=${timestamp}, v${version}=${digest}` | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| const httpLink = createHttpLink({ | ||
| uri: process.env.GRAPHQL_URL ?? 'http://localhost:3001/graphql' | ||
| }) | ||
|
|
There was a problem hiding this comment.
It would be nice if we could share this between frontend and mock-account-servicing-entity. And potentially integration in the future. I recall @sabineschaller brought up a similar need for something else recently (I don't recall what exactly).
I could definitely see a case for a lib, rafiki-lib, internal (to borrow a Go convention), or something else, where this and anything else we wanted to share internally (standardized pino log factory function?).
I guess we dont necessarily need to do that in this PR but but this certainly illustrates the point.
...ly locally/Peer-to-Peer Cross Currency Payment/Create Receiver (remote Incoming Payment).bru
Outdated
Show resolved
Hide resolved
njlie
force-pushed
the
nl/2218/admin-api-security-signature
branch
from
f8aa97a to
1c3da27
Compare
njlie
force-pushed
the
nl/2218/admin-api-security-signature
branch
from
3be6f98 to
f8e4101
Compare
njlie
force-pushed
the
nl/2218/admin-api-security-signature
branch
from
f8e4101 to
4483b51
Compare
Changes proposed in this pull request
backendAdmin Server that validates a signature comprised of a timestamp and a HMAC digest of the request body.ApolloLinkto the Apollo Clients offrontendandmock-asethat generates a signature from a timestamp and the request body.Context
Fixes #2218 and fixes #2596 and fixes #2568.
This signature scheme is pulled from the one used for the webhook service on the backend:
https://github.com/interledger/rafiki/blob/main/packages/backend/src/webhook/service.ts#L218-L235
Checklist
fixes #number