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

Ensure consistent JWK generation. #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thedodd
Copy link

@thedodd thedodd commented Feb 14, 2019

I'm opening this PR because I was running into issues where the HTTP-01 workflow was failing due to invalid signatures and such. I investigated the code here after thoroughly reviewing my own code in the service which is using this crate. The issue has been resolved after using BTreeMap instead of standard HashMap.

It was one of those tough bugs to resolve where the issue was presenting itself intermittently. Sometimes the workflow would succeed, other times it would fail for no apparently reason or code change on my side.

After making this update, the issue has not presented itself. For reasoning straight from the docs, see the first few paragraphs under this heading: https://doc.rust-lang.org/std/collections/index.html#iterators

Added a few debugging logs as well.
@thedodd
Copy link
Author

thedodd commented Feb 14, 2019

@onur looks like some of the tests on Linux are having issues:

  • 1.34-nightly: account_registration & authorization tests are failing.
  • 1.32-stable: only authorization test is failing.

It looks like the same exact test failures took place on the last two commits on master (also only for Linux), so the failures were not introduced by this PR. How would you like to proceed with this?

@onur
Copy link
Owner

onur commented Feb 15, 2019

Hi @thedodd. Thanks for the PR.

I am not sure why tests started failing on travis. We also have same problem on #44. I'd like to merge that one first.

@thedodd
Copy link
Author

thedodd commented Mar 11, 2019

Hey @onur. That sounds fine to me. That's great too, because then it would be possible to easily implement an async version of this using reqwest's async module. I'm currently using it in such a way that blocking network calls are wrapped in a blocking section to delegate them to background threads.

Keep me posted. I'm happy to rebase or make any other updates as needed.

@axos88
Copy link

axos88 commented Apr 18, 2019

Awww man, just debugged the same issue, and wanted to submit basically the same PR. Can we get this merged?

@thedodd
Copy link
Author

thedodd commented Apr 18, 2019

@axos88 yea, this was a pesky issue to debug.

@dacut
Copy link

dacut commented Feb 12, 2021

I am attempting to upgrade this client to ACMEv02 (in a fork, since this appears to be abandoned).
I've merged this in over in my repo to the main branch in this commit: dacut@53a9feb

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.

4 participants