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

Add Threadsafety for x-bunq-server-signature #7

Closed
PJUllrich opened this issue Aug 6, 2017 · 14 comments
Closed

Add Threadsafety for x-bunq-server-signature #7

PJUllrich opened this issue Aug 6, 2017 · 14 comments
Assignees
Labels

Comments

@PJUllrich
Copy link
Contributor

PJUllrich commented Aug 6, 2017

Hello! It's me again!

I encountered a problem when making calls with the SDK in a concurrent manner. Especially, I got the following error when making calls to the production API (no errors occurred in Sandbox)
Here's the stacktrace:

# Making a call with a ApiContext to the User object. I deleted calls in my own code.
    users = generated.User.list(api.Client.ctx())
  File "/usr/lib/python3.6/site-packages/bunq/sdk/model/generated/endpoint.py", line 9480, in list
    response = api_client.get(endpoint_url, custom_headers)
  File "/usr/lib/python3.6/site-packages/bunq/sdk/client.py", line 246, in get
    custom_headers
  File "/usr/lib/python3.6/site-packages/bunq/sdk/client.py", line 109, in _request
    response.headers
  File "/usr/lib/python3.6/site-packages/bunq/sdk/security.py", line 250, in validate_response
    signer.verify(digest, base64.b64decode(headers[_HEADER_SERVER_SIGNATURE]))
  File "/usr/lib/python3.6/site-packages/requests/structures.py", line 54, in __getitem__
    return self._store[key.lower()][1]
KeyError: 'x-bunq-server-signature'

Do you have an idea what could cause the problem that out of a sudden the server signature is not found anymore when making concurrent calls and no problem occurs when making sequential calls?

Update: I experienced this issue whenever I create 2 Threads which create and 'get' Payments to the production API. So basically, I make 2 independent Payments from e.g. account A to C and account B to C and then there seems to be some kind of issue with the verification function of the bunq response

@OGKevin OGKevin added the bug label Aug 6, 2017
@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@PJUllrich The most common reason of x-bunq-server-signature is when an error is returned by API and somehow missed by the self._assert_response_success(response) method.

Could you please add this:

print(response.content.decode())

to line 103 of our client.py and send the output here?

@PJUllrich
Copy link
Contributor Author

PJUllrich commented Aug 7, 2017

I added the requested print statement in client.py and added another one in security.py between lines 249 - 250 (just before the Exception occurs). The print statement read:

print(f'From security.py / validate_response - {headers}')

I added the outputs for

  1. A response from the server that lead to the Exception and
  2. A response for a payment made at the 'same' time that didn't lead to an Exception:

Since the output was quite long, I added it to PasteBin and redacted sensitive information:

  1. https://pastebin.com/qu6qKWTJ
  2. https://pastebin.com/hWtMmNVx

As you can see, in the headers of the 1. (exception-causing) response, the x-bunq-server-signature is indeed missing. In the 2. response, said signature is available.

@OGKevin OGKevin assigned dnl-blkv and unassigned PJUllrich Aug 7, 2017
@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@PJUllrich I am interested in the response body of the request where validation fails... These outputs include everything but that body :(

@PJUllrich
Copy link
Contributor Author

That one was not printed since the verification failed I figured. Let me check once again.

@OGKevin OGKevin assigned PJUllrich and unassigned dnl-blkv Aug 7, 2017
@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@PJUllrich that's why I asked adding the print statement before the verification! :)

@PJUllrich
Copy link
Contributor Author

PJUllrich commented Aug 7, 2017

I added it there, but the output was humongous! I thought I copied everything necessary, but apparently I missed that one response body. I'll give it another try.

Also, please post the lines of code before which you want to have the print statement. You said line 103 and I put it there, but now that I inspect the code that I have, it's actually behind the verification. I will now put the print statement before this code snippet:

if self._api_context.installation_context is not None:
    security.validate_response(
        self._api_context.installation_context.public_key_server,
        response.status_code,
        response.content,
        response.headers
    )

@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@PJUllrich in the newest develop version of sdk_python, the lines go as:

    response = requests.request(
        method,
        self._get_uri_full(uri_relative),
        data=request_bytes,
        headers=all_headers
    )
    # line 103
    self._assert_response_success(response)

    if self._api_context.installation_context is not None:
        security.validate_response(
            self._api_context.installation_context.public_key_server,
            response.status_code,
            response.content,
            response.headers
        )

@OGKevin
Copy link
Contributor

OGKevin commented Aug 7, 2017

Aha, @dnl-blkv he is using the SDK published on PyPi. The sdk uploaded on PyPi is not up to date with the latest version of development...

@PJUllrich could you checkout the dev branch and place the sdk in your source code ? It is not possible to upload dev branch to PyPi as of I need to supply a newer version number, according to my knowledge may need to double check this.

@PJUllrich
Copy link
Contributor Author

PJUllrich commented Aug 7, 2017

D'oh! Look at this:

{
	"Error": [{
		"error_description": "Too many requests. You can do a maximum of 3 GET call per 3 second to this endpoint.",
		"error_description_translated": "Too many requests. You can do a maximum of 3 GET call per 3 second to this endpoint."
	}]
} {
	'Date': 'Mon, 07 Aug 2017 11:42:00 GMT',
	'Server': 'Apache',
	'X-Bunq-Client-Response-Id': 'b8b95ad5-170b-4657-9ca9-41cbe06f195f',
	'X-Bunq-Client-Request-Id': '',
	'X-Frame-Options': 'SAMEORIGIN',
	'Transfer-Encoding': 'chunked',
	'Content-Type': 'application/json',
	'Strict-Transport-Security': 'max-age=31536000;'
}

Multi-threading is too fast! 🤣

Ok, I guess I have to revisit my code in order to minimise the GET requests or live with the slightly slower performance of my script. Either way, the error should probably be thrown before the verification is done, or the server should give a signature for the error response as well.

@OGKevin
Copy link
Contributor

OGKevin commented Aug 7, 2017

@PJUllrich xD. @dnl-blkv might be an idea to support multi threads for python ?

@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@PJUllrich here we go! I wonder though why it goes through the assertion... Ahh wait! You've got the old version where validation is done before asserting for errors :). Hold on, we'll do a proper release and you'll be able to include the newest version where this nastiness is fixed.

@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@OGKevin probably yes, but a bit later

@PJUllrich
Copy link
Contributor Author

Well thanks anyway guys! I guess this topic can be closed now.

@OGKevin OGKevin closed this as completed Aug 7, 2017
@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 7, 2017

@PJUllrich However, our rate limiting remains non-friendly to concurrence. For now, you can workaround it by creating a shared registry storing, for every endpoint, times of last 3 GET calls, last 5 POST calls and last 2 PUT calls and checking against those if the call can already be made (or sleeping otherwise). Here's the page where the limits are specified: https://doc.bunq.com/api/1/page/errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants