Multi-validation & true async issuance #17

cpu · 2017-04-05T18:18:00Z

Updates the README
5491a24 updates the README to include Chisel usage instructions
& reference the current-most draft.

Adds multi-validation and true async issuance.
7d615b5 splits the operation of the CA and VA across multiple
go-routines instead of doing all work on the WFE's handler routine.

The VA now performs multiple validations, sleeping a random amount of
time between each. This forces the ACME client to poll the
authorizations to learn when they have switched from pending to invalid
or valid. All validation attempts must succeed for the authorization to
be valid.

The WFE no longer communicates directly with the CA, instead when the VA
updates the last authorization on an order to valid status it invokes
the CA's CompleteOrder function in a separate goroutine. This will
complete the order & update it with a certificate pointer. The WFE uses
this pointer to construct the certificate URL as required.

This commit splits the operation of the CA and VA across multiple go-routines instead of doing all work on the WFE's handler routine. The VA now performs multiple validations, sleeping a random amount of time between each. This forces the ACME client to poll the authorizations to learn when they have switched from pending to invalid or valid. All validation attempts must succeed for the authorization to be valid. The WFE no longer communicates directly with the CA, instead when the VA updates the last authorization on an order to valid status it invoke's the CA's CompleteOrder function in a separate goroutine. This will complete the order & update it with a certificate URL.

cpu · 2017-04-05T18:18:31Z

Note: CI is going to fail until Chisel is updated to handle multiple validations & async issuance.

jsha

I mention locking for orders in the comments below. I think we also need locking for Authzs and Challenges. I noticed you flagged a race condition with a TODO, but I think it's worth handling the race conditions now, since they're hard to debug when they actually happen. Better to add the concurrency safety while we're thinking about it.

jsha · 2017-04-05T22:52:39Z

ca/ca.go

+	}
+
+	if order.Status != acme.StatusPending {
+		ca.log.Printf("Error: Asked to complete orrder %s is not status pending, was status %s",


*order %s, which is not

jsha · 2017-04-05T22:53:54Z

ca/ca.go

+
+	ca.log.Printf("Order %s is fully authorized. Ready to issue", order.ID)
+	// Update the order to reflect that we're now processing it
+	order.Status = acme.StatusProcessing


I think we need to hold a lock on the order.

jsha · 2017-04-05T22:54:59Z

ca/ca.go

+	order.Status = acme.StatusProcessing
+
+	csr := order.ParsedCSR
+	domains := make([]string, len(csr.DNSNames))


Why do we need to make a copy?

jsha · 2017-04-05T22:56:50Z

core/types.go

@@ -19,6 +19,7 @@ type Order struct {
 	ParsedCSR            *x509.CertificateRequest
 	ExpiresDate          time.Time
 	AuthorizationObjects []*Authorization
+	CertPathPrefix       string


Rather than embedding a public-facing path (WFE's responsibility) in the internal Order object, let's embed a certificate ID, and make WFE responsible for rendering that into a certificate path.

jsha · 2017-04-05T23:19:41Z

va/va.go

+		va.log.Printf("Pulled a task from the Tasks queue: %#v", task)
+		va.log.Printf("Starting %d validations.", concurrentValidations)
+
+		var wg sync.WaitGroup


This effectively serializes the tasks; If one authorization is validating, another authorization can't be validating at the same time. I think that might potentially mask some client bugs. Also, this loop seems a little long, and I think the goto may not be necessary.

What if instead, this was a very short loop:

for task := range va.tasks { go process(task) }

And the bulk of this function could be moved into process(..).

jsha · 2017-04-05T23:22:26Z

va/va.go

+		// concurrent validations into a slice for processing.
+		for i := 0; i < concurrentValidations; i++ {
+			va.log.Printf("Reading a result from task.Results")
+			result := <-task.Results


Since we wait on the WaitGroup above, task.Results is guaranteed to be exactly filled, and there is guaranteed to be no concurrent access. So there's no reason to make it a channel. It can just be a slice, and then you can range over it. Alternately you could get rid of the WaitGroup and use just the channel, which would allow you to surface errors without waiting for all validations to come in. Basically, we need just one of these concurrency primitives here.

Also, I'd recommend factoring this out into a firstError function or similar, that returns either an error or nil. That way you can call that function and have an if/else that very clearly handles both the error and non-error cases in one place.

jsha · 2017-04-05T23:30:07Z

va/va.go

 	if err != nil {
-		return err
+		result.Error = err


I think switching this function from one that returns an error, to one that mutates its input to set an error, is a step backwards in clarity. We had similar code in Boulder, but moved towards the "return an error" approach.

Stepping back a bit, this is related to the choice to have a vaTask contain both input and output. What if we defined vaTask to be only the input to a task, and the output from a task to be a ValidationRecord? Or in other words, treat vaTask as immutable. This might be an argument to use a chan *ValidationRecord in place of a WaitGroup. The wg argument to performValidation could be replaced with a chan, and the switch statement could do ch <- va.validateHTTP01(task).

Based on review feedback this commit updates the CA to stash a certificate ID in the internal order object instead of a path prefix. The WFE then constructs a certificate URL for the certificate based on the ID in the Order() handler. This keeps public facing paths as a WFE responsibility.

This commit updates the core internal types to embed RWMutex's to protect against concurrent reads/updates. The relevant wfe, db, va, and ca functions were updated to acquire read/write locks as appropriate. This commit also reworks parts of the CA and VA to have shorter, easier to read functions. The VA is updated to not serialize the validation work as it reads vaTasks off of a channel. The WaitGroup was removed and results from validation attempts are processed as they occur.

cpu · 2017-04-12T19:20:00Z

@jsha I believe all of your feedback is addressed, can you give this PR another pass when you have a chance? Thanks!

jsha · 2017-04-13T05:05:46Z

ca/ca.go

+		authz.RUnlock()
+	}
+
+	order.RLock()


You need to lock earlier, before you fetch order.AuthorixationObjects, Also I think a full lock would be fine; we're not worried about performance here, and there's no deadlock possibility, right?

Good catch on locking too late here. Thanks.

jsha · 2017-04-13T05:15:47Z

ca/ca.go

+	// Update the order to valid status and store a cert ID for the wfe to use to
+	// render the certificate URL for the order
+	order.Status = acme.StatusValid
+	order.CertID = cert.ID


Why do you need to store a copy of cert.ID? Why not use it directly?

"Why not use it directly" -> You mean a pointer to the certificate?

Yeah; I wrote this comment before the earlier one. Is the certificate stored in memory separately from the order? If so, then yes a pointer to the certificate would be good. Otherwise, DER should work.

Aha, should have read the latest diffs before replying. This looks great.

jsha · 2017-04-13T05:22:55Z

ca/ca.go

 	order.Status = acme.StatusValid
-	order.Certificate = order.CertPathPrefix + cert.ID
-	ca.log.Printf("Order %s has Certificate %s", order.ID, order.Certificate)
+	order.CertID = cert.ID


Since this field is only used to check whether a certificate is done, maybe it would be better to create a field that contains the cert DER, since we'll actually need that later. Otherwise, it's a bit confusing why there are two "ID" fields.

Per review feedback its strange to have two IDs when one is just used to indicate that an order is complete. Instead the internal Order type should hold a pointer to the internal Certificate type. The ID and DER encoding can be read from that as required for delivering the order.

Per review feedback the `order.RLock()` on L38 was happening too late, after the loop over the `order.AuthorizationObjects`. This commit moves the lock earlier & simplifies by acquiring one write lock for the duration of the function vs a read lock and a subsequent write lock.

cpu · 2017-04-13T15:34:09Z

@jsha Another round of feedback applied - thanks!

…etsencrypt#17)

Daniel added 2 commits March 23, 2017 10:49

Update README.

5491a24

cpu requested a review from jsha April 5, 2017 18:18

jsha reviewed Apr 5, 2017

View reviewed changes

Daniel added 3 commits April 10, 2017 13:29

Fixes order printf typo

6ea9faa

jsha requested changes Apr 13, 2017

View reviewed changes

Daniel added 2 commits April 13, 2017 11:27

cpu mentioned this pull request Apr 13, 2017

Add TLS-SNI-02 Challenge Support #18

Merged

jsha merged commit f471a5b into master Apr 13, 2017

jsha deleted the cpu-go-go-gadget-goroutine branch April 13, 2017 22:44

mcpherrinm mentioned this pull request May 24, 2024

Merge challtestsrv package into Pebble #466

Draft

mcpherrinm pushed a commit to mcpherrinm/pebble that referenced this pull request May 24, 2024

README: remove word "command" since this is a library only package (l…

c35d6f4

…etsencrypt#17)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-validation & true async issuance #17

Multi-validation & true async issuance #17

cpu commented Apr 5, 2017 •

edited

Loading

cpu commented Apr 5, 2017

jsha left a comment

jsha Apr 5, 2017

jsha Apr 5, 2017

jsha Apr 5, 2017

jsha Apr 5, 2017

jsha Apr 5, 2017

jsha Apr 5, 2017

jsha Apr 5, 2017

cpu commented Apr 12, 2017

jsha Apr 13, 2017

cpu Apr 13, 2017

jsha Apr 13, 2017

cpu Apr 13, 2017

jsha Apr 13, 2017

jsha Apr 13, 2017

jsha Apr 13, 2017

cpu commented Apr 13, 2017

Multi-validation & true async issuance #17

Multi-validation & true async issuance #17

Conversation

cpu commented Apr 5, 2017 • edited Loading

cpu commented Apr 5, 2017

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Apr 13, 2017

cpu commented Apr 5, 2017 •

edited

Loading