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

Smart nonces #144

Closed
wants to merge 6 commits into from
Closed

Smart nonces #144

wants to merge 6 commits into from

Conversation

mmaney
Copy link
Collaborator

@mmaney mmaney commented Feb 9, 2020

Fixes #143 by adding a simple fresh nonce cache that is refilled opportunistically from Replay-Nonce header included in most server responses.

NB: this builds on top of wrap-requests and requires that PR.

[to clarify: this branch will bring along the "wrap requests" work, because that's how git works. I called it out separately when it was first submitted because the "wrap requests" code had been an earlier PR, and the nonce harvesting wasn't the original goal.]

@kylejohnson
Copy link
Collaborator

@mmaney Could you link the PR number that this relies on on?

@mmaney
Copy link
Collaborator Author

mmaney commented Mar 10, 2020

Sorry - when I submitted this hoping to get some feedback, it was an offshoot of "wrap requests" which had some other motivation (using pebble for testing, maybe?). The branch includes its prerequisite (the two "wrap" commits) because that's how git works. I'll edit above to make this less unclear (I hope).

[later] I've closed the wrap-requests PR as it's subsumed by this PR. That may help the confusion, too.

@komuw
Copy link
Owner

komuw commented Mar 14, 2020

@mmaney tests are failing, could you have a look?

@komuw komuw mentioned this pull request Mar 14, 2020
@mmaney
Copy link
Collaborator Author

mmaney commented Mar 15, 2020

I don't understand what that CI system is trying to say, but from that first error it looks like an error with the aliyundns access key, which I can't do anything about?

Oh, the traceback (and some useful clues) is hidden 'way below. Yuck. Is this trying to talk to LE's staging? that's known not to work since they switched that to reject the older draft v.2 protocol, so until the rfc8555 updates are in, the CI is useless. If I understand what's going on...

@komuw
Copy link
Owner

komuw commented Mar 16, 2020

Oh, the traceback (and some useful clues) is hidden 'way below. Yuck. Is this trying to talk to LE's staging? that's known not to work since....

@mmaney As far as I know, sewer does not talk to LE or carry out any other network IO during tests. Any network IO is mocked out.

As an example one of the tests that is failing is:

def test_certificate_is_issued_for_renewal(self):
with mock.patch("requests.post") as mock_requests_post, mock.patch(
"requests.get"
) as mock_requests_get:
mock_requests_post.return_value = test_utils.MockResponse()
mock_requests_get.return_value = test_utils.MockResponse()
for i in ["-----BEGIN CERTIFICATE-----", "-----END CERTIFICATE-----"]:
self.assertIn(i, self.client.renew())

as you can see, that test begins with;

with mock.patch("requests.post") as mock_requests_post ...

so that any network IO is mocked.

The problem is that this PR has changed, instead of having requests.get and requests.post in the code; this PR has changed that to self.GET and self.POST.
So this PR should correspondingly change the testcases so that the mock is like

with mock.patch("sewer.Client.POST") as mock_requests_post ....:

etc

@mmaney
Copy link
Collaborator Author

mmaney commented Mar 17, 2020

One damn thing after another, isn't it? I'm going to drop back ten and punt.

@mmaney mmaney closed this Mar 17, 2020
@mmaney mmaney deleted the smart-nonces branch March 17, 2020 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sewer ignores fresh nonces in most responses, makes get-nonce requests too often
3 participants