-
Notifications
You must be signed in to change notification settings - Fork 0
message signature hello-server testing #27
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
base: main
Are you sure you want to change the base?
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.
This is certainly going in the right direction but there is still work to do. So far, I have concentrated on the server code not on the docs. Brendan may be able to help more.
except jwt.InvalidTokenError as e: | ||
_fail("Token check", f"Approov token invalid ({e})", 401) | ||
|
||
def verifyApproovTokenBinding(claims: dict, test_name: str): |
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.
This function should change to include a new parameter - a list of headers to extract from a request. It should then iterate accross the header values and generate the string to be fed into the hash. When it is called, the provided list will normally just have "Authorization" in it, but it could also have both that and CUSTOM_BINDING_HEADER. Once this is changed, you can remove the next function verifyApproovTokenBindingCustom and instead call this function with different header lists.
_pass("Token binding") | ||
return {"authorization": auth, "computed_pay": pay_from_auth, "token_pay": claims["pay"]} | ||
|
||
def verifyApproovTokenBindingCustom(claims: dict, test_name: str): |
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.
Thus should be removed once the above function is fixed.
if not ipk_b64url: | ||
_fail(test_name, "Approov token missing 'ipk' claim required for message signature", 401) | ||
|
||
sig_header = request.headers.get("Signature") |
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.
sig_header and sig_input are both SFV dictionary headers. You need to process these SFV dictionaries using a proper SFV parser and then extract the "install" key from both and use that for the message signature check.
pubkey_der = _b64url_to_bytes(ipk_b64url) | ||
pubkey = load_der_public_key(pubkey_der) | ||
|
||
canonical = f"@method: {request.method.upper()}\n@target-uri: {request.path}".encode("utf-8") |
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.
This method really needs to handle general message signatures as provided by and decoded from the signature-input header. It cannot reconstruct and test against a canonical header. To do this properly, you must use a full message signatures implementation such as: https://github.com/pyauth/http-message-signatures, https://github.com/pyauth/requests-http-signature or https://pypi.org/project/requests-http-message-signatures. I think one of the first two look better.
return {"has_ipk": True} | ||
|
||
# -------- mode dispatcher -------- | ||
def run_mode(mode: str): |
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.
Instead of run mode, this function should be changed to accept parameters that indicate if a token is expected, the list of headers to check and whether message signing should be performed and then call the associated functions to perform the different checks. Doing it like that enables an engineer to just copy the associated code from here to their project without making many changes and it will just work. In the current setup it is restricted to just work if code has the exact requirements of this project.
No description provided.