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(integration): sign Admin API requests during integration tests #3177

Open
wants to merge 5 commits into
base: 2893/multi-tenancy-v1
Choose a base branch
from

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Dec 11, 2024

Changes proposed in this pull request

  • Now that API_SECRET is required in backend (for seeding operator tenant in [Multi-Tenant] Create operator tenant #3149), we need to have request signing for the Admin API in all places. This PR adds signature signing to the integration tests

Context

Related to #3149

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Make sure that all checks pass
  • Bruno collection updated (if necessary)
  • Documentation issue created with user-docs label (if necessary)
  • OpenAPI specs updated (if necessary)

@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Dec 11, 2024
@mkurapov mkurapov changed the title 3149/mk/fix signature validation feat(integration): sign Admin API requests during integration tests Dec 11, 2024
@mkurapov mkurapov marked this pull request as ready for review December 11, 2024 19:42
Comment on lines 89 to 93
signature: generateApiSignature(
config.adminApiSecret,
config.adminApiSignatureVersion,
requestBody
)
Copy link
Contributor Author

@mkurapov mkurapov Dec 13, 2024

Choose a reason for hiding this comment

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

Now that the API_SECRET is mandatory, sign graphql requests during tests as well. I was thinking of potentially bypassing signing in tests via flag like if (config.env === "test") don't add the auth middleware, but this was straightforward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

edit: because of our replay attack checks, going to use if (config.env === "test") don't add the auth middleware method

@mkurapov
Copy link
Contributor Author

Trying to debug some tests, we have test failures because of the fact that some tests send duplicate requests (with the same resulting signature). We explicitly check for duplicate requests in:

async function canApiSignatureBeProcessed(
signature: string,
ctx: AppContext,
config: IAppConfig
): Promise<boolean> {
const { timestamp } = getSignatureParts(signature)
const signatureTime = Number(timestamp) * 1000
const currentTime = Date.now()
const ttlMilliseconds = config.adminApiSignatureTtl * 1000
if (currentTime - signatureTime > ttlMilliseconds) return false
const redis = await ctx.container.use('redis')
const key = `signature:${signature}`
if (await redis.get(key)) return false
const op = redis.multi()
op.set(key, signature)
op.expire(key, ttlMilliseconds)
await op.exec()
return true
}

@mkurapov mkurapov requested a review from njlie December 13, 2024 16:02
Copy link
Contributor

@njlie njlie left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants