-
Notifications
You must be signed in to change notification settings - Fork 90
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(http-signature-utils): move http signature related code to this package #797
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to add the signing endpoint to the localenv?
|
||
export function validateHttpSigHeaders(request: RequestLike): boolean { | ||
const sig = request.headers['signature'] | ||
const sigInput = request.headers['signature-input'] as string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const sigInput = request.headers['signature-input'] as string | |
const sigInput = request.headers['signature-input'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSigInputComponents(sigInput ?? '')
requires a string and as is, sigInput
is of type string | {toString(): string;} | string[]
headers: HttpSigHeaders | ||
} | ||
|
||
function validateHttpSigHeaders(ctx: Context): ctx is HttpSigContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep this but just as a wrapper for the imported validateHttpSigHeaders
in order to keep the HttpSigContext
type guard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would avoid having to change HttpSigContext
to AppContext
in verifySigFromClient
/verifySigFromClient
.
Not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is already being checked by the OpenAPI validation middleware
rafiki/packages/auth/src/app.ts
Lines 198 to 204 in 30096d1
createValidatorMiddleware(openApi.authServerSpec, { | |
path: '/', | |
method: HttpMethod.POST | |
}), | |
this.config.bypassSignatureValidation | |
? (ctx, next) => next() | |
: grantInitiationHttpsigMiddleware, |
Maybe the types used in the OpenAPI type guards should also extend
HttpSigContext
which can be used throughout these signature middlewares
Maybe that gives us enough sanity checking to stop calling validateHttpSigHeaders
early?
#797 (comment)
request, | ||
privateKey, | ||
keyId | ||
}: SignOptions): Promise<Headers> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could generic SignOptions
and Headers
types be used to enforce that a request with body
will return headers with content headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's worth it
type NoBodyRequest = SignOptions['request'] & {
body?: never
}
type NoBodyOptions = Omit<SignOptions, 'request'> & {
request: NoBodyRequest
}
type NoBodyHeaders = SignatureHeaders
type BodyRequest = SignOptions['request'] & {
body: string | Buffer
}
type BodyOptions = Omit<SignOptions, 'request'> & {
request: BodyRequest
}
type BodyHeaders = SignatureHeaders & ContentHeaders
export const createHeaders:
| ((options: NoBodyOptions) => Promise<NoBodyHeaders>)
| ((options: BodyOptions) => Promise<BodyHeaders>) = async ({
request,
privateKey,
keyId
}) => {
import { importJWK } from 'jose' | ||
import { JWK } from './jwk' | ||
|
||
export function validateHttpSigHeaders(request: RequestLike): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should either just export a verifyHttpSigHeaders
function or minimize overlap between that and validateHttpSigHeaders
?
As is, I like how we're able to perform basic validation checks on expected headers in auth
's signature middleware before fetching the client key for actual signature verification. But I don't like how we call validateHttpSigHeaders
again internally (with getSigInputComponents
+validateSigInputComponents
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know how we could improve on that here. Like you said, we want to validate the headers before fetching the key. When we validate the signature, however, we do need the components again. So the only option we have, I think, is to remove component validation from validating the signature because we always validate the headers first, which already includes validating the components.
(!!request.headers['content-digest'] && | ||
request.body && | ||
Object.keys(request.body).length > 0 && | ||
sigInputComponents.includes('content-digest') && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we require that content-length
and content-type
are also included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the headers, in the Signature-Input
components, in both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both unless they're somehow not required by GNAP/httbis?
I think content-type
is being enforced in the headers by the OpenAPI spec's:
requestBody:
content:
application/json:
@@ -14,7 +14,7 @@ services: | |||
NODE_ENV: development | |||
AUTH_DATABASE_URL: postgresql://auth:auth@database/auth | |||
INTROSPECTION_HTTPSIG: "false" | |||
BYPASS_SIGNATURE_VALIDATION: "true" | |||
BYPASS_SIGNATURE_VALIDATION: "false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
packages/http-signature-utils/src/postman-scripts/preRequestSignatures.js
Outdated
Show resolved
Hide resolved
@@ -27,7 +27,6 @@ | |||
"@types/tmp": "^0.2.3", | |||
"@types/uuid": "^8.3.4", | |||
"apollo-server": "^3.10.1", | |||
"auth": "workspace:../auth", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, especially that the Open API schema gets defined in a single place. I'm guessing this package will also have http-signature-utils
as a dependency? Are we planning on signing these AS <> RS introspection requests?
Changes proposed in this pull request
Context
http-signature-utils
#818Checklist
fixes #number