mohawk icon indicating copy to clipboard operation
mohawk copied to clipboard

Add option to `Receiver` class to allow deferring content hash check

Open rneilson opened this issue 8 years ago • 5 comments

From the Hawk spec:

However, if the payload is not available at authentication time (e.g. too large to fit in memory, streamed elsewhere, or processed at a different stage in the application), the server may choose to defer payload validation for later by retaining the hash value provided by the client after validating the MAC.

This is currently not an option with Mohawk, as if a content hash is provided by the client, Receiver will attempt to validate it during instantiation. I propose a new keyword argument to Receiver(), defer_content_hash, which will only validate the MAC, timestamp, and nonce of the request, but not generate the content hash.

This would also require an additional method of Resource, perhaps .check_hash(content, content_type) which would then generate the hash and compare it to the value from Resource.parsed_header. This new method would also be called from inside Resource._authorize() (see here).

rneilson avatar Apr 07 '17 00:04 rneilson

Yes, Receiver(..., defer_content_hash=True) sounds good.

As for check_hash(), this should probably be designed with large payloads in mind, right? As I recall, that's the main use case here. My Python is rusty but could we offer a context manager for this? Maybe like:

receiver = Receiver(..., defer_content_hash=True)
with receiver.verify_content() as verifier:
    verifier.set_content_type(content_type)
    for chunk in get_buffered_content():
        verifier.add_content(chunk)

Or would that interface be too annoying?

kumar303 avatar Apr 17 '17 21:04 kumar303

Not sure if a context manager would be best -- there are a lot of different ways to handle large payloads, some of which involve offloading, threads, or multipart forms (where a context manager might not work very well, if at all). What might work is letting the receiver itself be a context manager (I'll have to look up how best to do that) if desired, but have something akin to set_content_type(), add_content(), and verify_content() as methods directly on Receiver, allowing incremental updates of the hash by whatever means the user likes.

rneilson avatar Apr 17 '17 22:04 rneilson

I think a low level interface with set_content_type(), add_content(), and verify_content() directly on Receiver might be a bit awkward because you have to remember to call verify_content() at the end. My idea with the context manager was to do the final verification in __exit__() so that there is no possible way to skip it.

kumar303 avatar Apr 17 '17 22:04 kumar303

This is why I'm thinking a dual approach -- a context manager would be nice to have, yes, for that very reason. But if you're offloading writing to a file (or something) to another thread, or dealing with a multipart form (which gets tricky), you may not be able to use a context manager. Or if you want to catch a failed verification and delete the uploaded file before responding, for example.

If the low-level interface is there, it can be called wherever and however, which would obviously be the user's responsibility (the "we're all adults here" principle), and the context manager could just wrap the beginning and end of the process with __enter__() and __exit__().

rneilson avatar Apr 17 '17 22:04 rneilson

I think it would be good to start with a context manager and see where we end up. I am having trouble imagining a situation where you could not use the context manager for buffered content verification, even when offloading to threads. The underlying API would always be there (i.e. __exit__ could call self._finish_verification() or something) but if they don't need it, I'd rather not document it.

kumar303 avatar Apr 18 '17 15:04 kumar303