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(auth): secure admin api with hmac signatures #2709

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

njlie
Copy link
Contributor

@njlie njlie commented May 7, 2024

Changes proposed in this pull request

  • Added signature header verification to Auth Server Admin API

Context

Closes #2704.

Checklist

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

@github-actions github-actions bot added type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels May 7, 2024
Copy link

netlify bot commented May 7, 2024

Deploy Preview for brilliant-pasca-3e80ec ready!

Name Link
🔨 Latest commit dd78364
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/664e5dd24653ce000923310a
😎 Deploy Preview https://deploy-preview-2709--brilliant-pasca-3e80ec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

bruno/collections/Rafiki/scripts.js Outdated Show resolved Hide resolved
bruno/collections/Rafiki/scripts.js Outdated Show resolved Hide resolved
packages/auth/src/shared/utils.ts Outdated Show resolved Hide resolved
packages/auth/src/config/app.ts Outdated Show resolved Hide resolved
@njlie njlie force-pushed the nl/2704/auth-admin-api-security branch from f75f811 to cc173b6 Compare May 22, 2024 21:03

async function canApiSignatureBeProcessed(
signature: string,
ctx: Context,
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be good to type this Context such that we get the types for the services like redis and logger.

const key = `signature:${signature}`
const op = redis.multi()
op.set(key, signature)
op.expire(key, signature)
Copy link
Contributor

Choose a reason for hiding this comment

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

should the expiry take in a num as the second argument?

},
'time differential'
)
if (currentTime - signatureTime > ttlMilliseconds) return false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a test for this as well

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

Successfully merging this pull request may close these issues.

Secure Auth Server Admin API with signatures
4 participants