feat(auth): Make sig validation not processed in dev mode

[mankins]

trafficstars

Changes proposed in this pull request

• Disable signature validation for incoming requests on the AS to make development easier with Postman
• New config setting bypassSignatureValidation to bypass signature validation

Context

Attempts to fix part of #664

Checklist

• [x] Related issues linked using fixes #number
• [x] Tests added/updated
• [ ] Documentation added
• [ ] Make sure that all checks pass

Discussion

This is a WIP / Draft PR that attempt some of the work discussed in #664. I thought a bit about the use case and thought against keying off the Config.env === 'development' because it may be common to want signature validation in development but sometimes disable validation. As such I propose bypassSignatureValidation to be set from an environmental variable BYPASS_SIGNATURE_VALIDATION to disable signature checks.

While there are tests and code, this current draft has not been tested because I haven't been able to get my local dev environment up for various reasons including #682

I'm submitting this Draft PR in case it helps others and to start a discussion about the approach taken and to help contribute to #hacktoberfest ... apologies in advance if I missed documentation of contributor guidelines about draft prs.

[mankins]

Bump: Can you run the tests again @wilsonianb ? My local still isn't working.

[wilsonianb]

Re-ran the tests. You can also try opening a pull request within your fork in order to run the checks without waiting for someone to push that button for you.

[mkurapov]

Maybe we could also add this flag to validateHttpSigHeaders, or directly as one of the first checks in grantContinueHttpsigMiddleware ? otherwise we will still need to provide the http header signature and signature-input properly formatted, if I'm not mistaken.

Thoughts @mankins @wilsonianb ?

[wilsonianb]

:thinking: yeah.... Now I'm wondering if this could have been handled entirely in app.ts route middleware setup. Assuming it's safe to bypass the entire *HttpsigMiddleware, we could have routes like:

this.publicRouter.post(
  '/',
  dummySigHeaderMiddleware, // adds headers if `bypassSignatureValidation` to satisfy validator middleware
  createValidatorMiddleware(openApi, {
    path: '/',
    method: HttpMethod.POST
  }),
  this.config.bypassSignatureValidation
    ? (ctx, next) => next()
    : grantInitiationHttpsigMiddleware,
  grantRoutes.create
)

[mkurapov]

@wilsonianb I like that idea, keeps it pretty tidy. I wonder if there is a way to add a flag to createValidatorMiddleware to skip parts of validation, instead of needing an additional dummySigHeaderMiddleware. Maybe somehow setting default values incase this flag is true? https://github.com/kogosoftwarellc/open-api/tree/master/packages/openapi-default-setter

[wilsonianb]

Maybe somehow setting default values in case this flag is true? https://github.com/kogosoftwarellc/open-api/tree/master/packages/openapi-default-setter

Interesting idea. Maybe our openapi's createRequestValidator could accept an optional set of default params to incorporate into its already existing default setter https://github.com/interledger/rafiki/blob/889807585763faaf4fc68d9a5d49140776d12501/packages/openapi/src/index.ts#L78-L92

[wilsonianb]

So... should we revert the commit from this pr? :+1: or :-1:

[mankins]

So... should we revert the commit from this pr? 👍 or 👎

I'm fine either way but wanted to comment:

The change to the mechanism that decides to validate signature or not is a small part of this pr:

https://github.com/interledger/rafiki/pull/683/files#diff-649ee9ffe9608a0c6296afe23fa744654797ec47dd45e41c5b4bd99f4195b536R25-R28

Most of it deals with testing that change.

As such you could keep this PR / tests / behavior, and follow on with a modification for how it works with a better implementation, including some ideas by @mkurapov about making it obvious these things can be toggled.

[wilsonianb]

I guess it depends on if we choose to put the check in app.ts middleware setup, in which case the added tests wouldn't be relevant because the signature middlewares wouldn't be called at all with bypassSignatureValidation.

[mkurapov]

I think no need to revert for now especially if it's functional, but I'd like to explore the idea of having this be bypass handled in app.ts and default parameters as @wilsonianb mentioned