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

feat: verify ACME challenges and complete order #164

Merged
4 commits merged into from Feb 10, 2023
Merged

feat: verify ACME challenges and complete order #164

4 commits merged into from Feb 10, 2023

Conversation

ghost
Copy link

@ghost ghost commented Feb 9, 2023

This PR:

  • Alters the code to use the Pebble docker container's mock let's encrypt server in development and testing
  • Completely turns off ssl verification when node-acme-client communicates with the development pebble server (no need to inject extra CA into node)
  • Alters the challenge bundle to return other challenge attributes, like state
  • Implements verifyChallenges instance method that asks the ACME provider to complete each challenge
  • implements completeOrder instance method that finalizes the order and retreives the certificate after all challenges have been completed

With these additional features, I successfully completed an order and obtained a multi-part certificate from the mock server

Closes #21

@ghost ghost requested a review from humphd February 9, 2023 15:53
@shawnyu5
Copy link
Contributor

shawnyu5 commented Feb 9, 2023

Can you change the title of the pr to something more descriptive?

@ghost ghost changed the title Issue 21 feat: verify ACME challenges and complete order Feb 9, 2023
@ghost
Copy link
Author

ghost commented Feb 9, 2023

Sure, sorry, my bad

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Your PR description makes it sound like you're missing files in this change?

One question about a line in the docs I want clarified.

app/lib/lets-encrypt.server.ts Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Feb 9, 2023

@humphd you mean this?

Completely turns off ssl verification when node-acme-client communicates with the development pebble server (no need to inject extra CA into node)

that part is here:
https:/Seneca-CDOT/starchart/pull/164/files#diff-7e3d8f4b0066f749d73746330c16a2fcfc75298640f9eb1a39e9e81863371ba6R198

I'm reaching into the acme's default client (an instantiated axios), and overwriting it's https client with the one that does not require SSL signature to match.

This is officially supported by both node-acme-client and axios for this very reason. This way we don't need to make system wide changes to the CA list, only acme client is affected, and only in a non-production environment

@ghost ghost requested a review from humphd February 9, 2023 20:07
@humphd
Copy link
Contributor

humphd commented Feb 9, 2023

It was you saying "Alters the code to use the Pebble docker container's mock let's encrypt server in development and testing" which made me think you had altered other files. While I'm thinking of this, do you want to delete the CA .pem file I added before, since we won't need it now?

@ghost
Copy link
Author

ghost commented Feb 9, 2023

sure!

@ghost ghost requested a review from humphd February 9, 2023 22:08
app/lib/lets-encrypt.server.ts Show resolved Hide resolved
app/lib/lets-encrypt.server.ts Show resolved Hide resolved
Copy link
Contributor

@sfrunza13 sfrunza13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have spent a while looking at this and at the node-acme and rfc docs so that I could better understand it because I haven't really worked with this before. That being said the ideas in this code were evident even before that because it is well commented and written already, I went and read up again on the whole flow however because I am a noob.

So in this PR you did exactly what you set out to do. You added to the challengeBundle so you could use properties from AcmeDnsChallenge (and further the ChallengeAbstract interface), you used those attributes to check that all challenges pass and if not (I am not clear here but I think) you begin async retry on pending and invalid ones, and once order status is ready the completeOrder method will finalize the order and get the certificate.

You have also changed to a diff axios client that doesnt require ssl for ease of testing. David already mentioned the directoryUrl and I'm sure as he mentioned you already know.

I really have barely read up on this stuff (maybe 1hr and change) and can kind of follow along with the entirety of this lets-encrypt server you have written so I have nothing more to add, it's really good.

@ghost
Copy link
Author

ghost commented Feb 10, 2023

Hi @sfrunza13, Thanks for the kind words

It is usually a good practice to build stuff layer to layer, so this class only focuses on communicating with Let's Encrypt.
The next piece will be to integrate this with the queue system. To do this, we need to have four stages, and three general outcome of each stage:

Stages:

  • Create an order and add the challenges to DNS
  • Wait for DNS to propagate (wait as needed. This is a static method, we don't communicate with the LE api to save on the rate limit)
  • Request challenge verification from let's encrypt, wait for LE result and wait / retry as needed This step has to be able to fail and re-run
  • Order completion, DNS cleanup

Outcomes:

  • Success (self explanatory, next stage can commence)
  • Defer (the current stage has soft-failed, I.e., DNS has not yet been propagated), so let's wait a bit and retry what we have to (some challenges may fail and some may complete for the same cert, only retry the failed ones)
  • Failure (This is a hard fault, we cannot go ahead, have to completely abort the process)

So I made this code with the above in mind for example, if step 3 finds it that a challenge is still pending, it will simply return false, indicating a Defer, but if the order is in an incorrect state (the whole order failed), there is no way to continue, so it will throw an exception.

We will be able to handle / catch these in the queue worker, and depending of the above three outcomes, we can decide on what to do

This way the actual certificate generation logic, and the proccess management of it is completely separate, helps to separate the responsibilities and create a cleaner, more readable code.

@ghost ghost merged commit d284cc6 into DevelopingSpace:main Feb 10, 2023
@humphd
Copy link
Contributor

humphd commented Feb 10, 2023

Hi @sfrunza13, Thanks for the kind words

It is usually a good practice to build stuff layer to layer, so this class only focuses on communicating with Let's Encrypt. The next piece will be to integrate this with the queue system. To do this, we need to have four stages, and three general outcome of each stage:

Stages:

  • Create an order and add the challenges to DNS
  • Wait for DNS to propagate (wait as needed. This is a static method, we don't communicate with the LE api to save on the rate limit)
  • Request challenge verification from let's encrypt, wait for LE result and wait / retry as needed This step has to be able to fail and re-run
  • Order completion, DNS cleanup

Outcomes:

  • Success (self explanatory, next stage can commence)
  • Defer (the current stage has soft-failed, I.e., DNS has not yet been propagated), so let's wait a bit and retry what we have to (some challenges may fail and some may complete for the same cert, only retry the failed ones)
  • Failure (This is a hard fault, we cannot go ahead, have to completely abort the process)

So I made this code with the above in mind for example, if step 3 finds it that a challenge is still pending, it will simply return false, indicating a Defer, but if the order is in an incorrect state (the whole order failed), there is no way to continue, so it will throw an exception.

We will be able to handle / catch these in the queue worker, and depending of the above three outcomes, we can decide on what to do

This way the actual certificate generation logic, and the proccess management of it is completely separate, helps to separate the responsibilities and create a cleaner, more readable code.

@dadolhay seems like the start of a great blog post <hint /> :)

@ghost
Copy link
Author

ghost commented Feb 10, 2023

@humphd
I was already in the process of copying XD

@ghost ghost deleted the issue-21 branch February 10, 2023 16:07
This pull request was closed.
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.

Implement code that finishes the certificate generation process
4 participants