PoC: allow renewing factory root CA #51

vkhoroz · 2024-06-12T19:22:10Z

This is currently in the PoC stage i.e. fully untested.

However, it clearly shows the primary idea, and is ready for an early feedback.

The easiest way to grasp this is commit-by-commit.

mike-sul · 2024-06-17T12:49:46Z

internal/rotate_est.go

 	if err != nil {
 		return err
 	}
+	if len(estCerts) > 1 {
+		return fmt.Errorf("Unexpected more than one certificate in response: %d", len(estCerts))


Nitpick. Maybe "Unexpectedly received more than one certificate in response" or "Expected one ..., but received %d"?

Thanks, your second suggestion looks good to me... I'll take it.

internal/rotate_est.go

mike-sul · 2024-06-17T13:08:19Z

internal/renew_root.go

+	}
+
+	var content bytes.Buffer
+	for _, c := range certs {


Do we expect a certain number of certs? If so, then maybe it makes sense to check the expectation (e.g. len(certs) == 2). Or we actually don't know, since customers may put any number of certs into it?

I expect any number of certs here.
Our API will only ever send 1 or 3, but the custom EST server may send a different amount.

That said, I was thinking about some hard limit to protect the device.
Maybe smth like a maximum of 10 CAs sounds reasonable.
At the final call, an EST server should normally wind up sending just one CA.

mike-sul · 2024-06-17T13:11:28Z

main.go

+
+	log.Printf("Performing root certificate update")
+	if err = handler.Update(); err == nil {
+		log.Print("Root certificate update sequence complete")


nitpick: "is completed" or "completed"?

mike-sul · 2024-06-17T13:13:18Z

main.go

@@ -184,6 +206,14 @@ func main() {
 					},
 				},
 			},
+			{
+				Name:     "renew-root",
+				HelpName: "renew-root <EST Server> [<rotation-id>]",


It looks like inconcistency between the cli parameter name rotation-id and the state field handler.State.CorrelationId. I suppose that's because it comes from the base class.

I wanted this to be correlation-id, but it must be rotation-id for backward compatibility (required for renew-cert).
So, in this PR I ended up renaming the runtime field name, but leaving the state file property on disk the old way.

That said, I tend to agree with you that the command-line argument needs to be renamed too, as it is a positional argument, especially for this new command.

mike-sul · 2024-06-17T13:27:10Z

contrib/renew-root-cert

+	exit 0
+}
+
+# We'll execute under two conditions:


Are those conditions prevent from updating root CA while there is ongoing root CA update?

These conditions were copied from rotate-cert, and IIUC they are here to prevent an accidental CA re-renewal unrelated config changes.

Maybe @doanac has a better explaination, as an original author of this (borrowed) idea.

doanac

I need to re-read the the cert verification logic another 5 times and then look at the unit tests a couple times but this is how I was hoping this would pan out.

doanac · 2024-06-17T13:41:06Z

internal/renew_root.go

+func (s fetchRootStep) Execute(h *rootRenewalContext) error {
+	caFile := h.app.sota.GetOrDie("import.tls_cacert_path")
+	url := h.State.EstServer + "/cacerts"
+	res, err := h.client.Get(url)


I think you can use these helpers which will help with retry and backoff- https://github.com/foundriesio/fioconfig/blob/main/internal/http.go#L84

I guess you are right... I just didn't pay enough attention as to why those helpers are not used for the cert rotation.
Maybe, in that case a "plain retry" would be dangerous because of the POST which is not applicable for a simple fetch of new CAs?

it was probably just me forgetting to use my own helper functions

doanac · 2024-06-17T13:49:04Z

internal/renew_root.go

+	}
+	for len(data) > 0 {
+		if block, data = pem.Decode(data); block == nil {
+			return nil, fmt.Errorf("Malformed PEM block at %s", data[:100])


maybe put some ... in the error message to indicate the string has been truncated for display purposes?

Makes sense.

internal/renew_root.go

main.go

mike-sul · 2024-06-17T14:07:49Z

LGTM, will need to go through it for the second time to understand better.

I actually needed to change the decodeEstResponse method signature, so that I could reuse it for the root CA update. This turned out to be a separate tiny improvement. Signed-off-by: Volodymyr Khoroz <[email protected]>

There is only one step in the state machine. But, a machine itself is still useful for robustness, retries, notifications, and restarting services. Signed-off-by: Volodymyr Khoroz <[email protected]>

It uses a command added in the previous commit. Signed-off-by: Volodymyr Khoroz <[email protected]>

Each new certificate must pass all of the below checks: 1. It is a valid certificate authority. 2. Its subject is exactly the same as a subject of one of the current CAs. 3. One of the following conditions is met: 3.1. It is signed by one of the current CAs. 3.2. It has the same public key as one of the current CAs. 3.3. It has the same public key as any certificate satisfying 3.1. Signed-off-by: Volodymyr Khoroz <[email protected]>

Some customers would want to allow unconditional root CA renewal. This can also be used as a way to recover from the root CA key loss. Signed-off-by: Volodymyr Khoroz <[email protected]>

Signed-off-by: Volodymyr Khoroz <[email protected]>

I need to reuse the EST server mock for the root CA renewal test. For that, I need to customize its handler func. While doing that I noticed a few interesting things: 1. An EST server mock client is not used. 2. An cert rotation EST step test does not verify what it received from the server. So, I also improved that test in scope of my work. Signed-off-by: Volodymyr Khoroz <[email protected]>

Signed-off-by: Volodymyr Khoroz <[email protected]>

vkhoroz · 2024-06-19T19:15:48Z

Just rebased on main

StealthyCoder · 2024-09-13T12:15:42Z

contrib/renew-root-cert

+
+[ -z "$CONFIG_FILE" ] && (echo "No CONFIG_FILE specified"; exit 1)
+[ -f "$CONFIG_FILE" ] || (echo "$CONFIG_FILE does not exist"; exit 1)
+[ -z "$SOTA_DIR" ] && (echo "No SOTA_DIR specified"; exit 1)


This variable does not seem to be used anymore, or is it supposed to be STORAGE_DIR that is references later on ? 👍

Then do we need to check if it is actually a directory with [ -d $SOTA_DIR ] || ?

This is a copy paste from the other script: https://github.com/foundriesio/fioconfig/blob/main/contrib/renew-client-cert#L8

I think you are right and this code needs to check for the STORAGE_DIR instead.
It needs to be fixed in both the original and the copy.

It is a minor fix, and things just work in old versions (with this mistake); so no need to backport.

Good catch! 🚀

contrib/renew-root-cert

doanac · 2024-10-15T18:13:33Z

internal/renew_root.go

+	}
+	caCerts, err := loadCertsFromPem(caCertBuf)
+	if err != nil {
+		log.Fatal("Failed to parse root CA file", err)


this should return an error and not log.Fatal, right?

A root renewal script is not able to execute if it cannot parse CA certs.
So, it's a fatal error.

vkhoroz requested review from doanac and mike-sul June 12, 2024 19:22

vkhoroz self-assigned this Jun 12, 2024

mike-sul reviewed Jun 17, 2024

View reviewed changes

internal/rotate_est.go Show resolved Hide resolved

mike-sul reviewed Jun 17, 2024

View reviewed changes

doanac reviewed Jun 17, 2024

View reviewed changes

vkhoroz added 11 commits June 19, 2024 20:40

Feature: add a command to renew/update root CA

9a2edf2

There is only one step in the state machine. But, a machine itself is still useful for robustness, retries, notifications, and restarting services. Signed-off-by: Volodymyr Khoroz <[email protected]>

Feature: a contrib handler to renew/update root CA

ee3bf9c

It uses a command added in the previous commit. Signed-off-by: Volodymyr Khoroz <[email protected]>

Feature: ability to turn off signature check during root CA renewal

05eef23

Some customers would want to allow unconditional root CA renewal. This can also be used as a way to recover from the root CA key loss. Signed-off-by: Volodymyr Khoroz <[email protected]>

Feature: attempt resuming incomplete root CA renewal on restart

acf486b

Signed-off-by: Volodymyr Khoroz <[email protected]>

fixup! wrong slice initialization in validateCerts

1eaf19a

Signed-off-by: Volodymyr Khoroz <[email protected]>

Test: add unit tests for root CA renewal

d7fcbb5

Signed-off-by: Volodymyr Khoroz <[email protected]>

Test: verify that connection succeeds after cert rotation

7117573

Signed-off-by: Volodymyr Khoroz <[email protected]>

fixup! Feature: a contrib handler to renew/update root CA

8b44b0a

Signed-off-by: Volodymyr Khoroz <[email protected]>

vkhoroz force-pushed the vkhoroz-renew-root-ca branch from 4b1ab2a to 8b44b0a Compare June 19, 2024 19:15

StealthyCoder reviewed Sep 13, 2024

View reviewed changes

vkhoroz mentioned this pull request Oct 9, 2024

WIP: rotate root CA foundriesio/fioctl#433

Open

doanac reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC: allow renewing factory root CA #51

PoC: allow renewing factory root CA #51

vkhoroz commented Jun 12, 2024

mike-sul Jun 17, 2024

vkhoroz Jun 17, 2024

mike-sul Jun 17, 2024

vkhoroz Jun 17, 2024

mike-sul Jun 17, 2024

mike-sul Jun 17, 2024

vkhoroz Jun 17, 2024

mike-sul Jun 17, 2024

vkhoroz Jun 17, 2024

doanac left a comment

doanac Jun 17, 2024

vkhoroz Jun 17, 2024

doanac Jun 17, 2024

doanac Jun 17, 2024

vkhoroz Jun 17, 2024

mike-sul commented Jun 17, 2024

vkhoroz commented Jun 19, 2024

StealthyCoder Sep 13, 2024

StealthyCoder Sep 13, 2024

vkhoroz Sep 14, 2024

vkhoroz Sep 14, 2024

doanac Oct 15, 2024

vkhoroz Oct 22, 2024

PoC: allow renewing factory root CA #51

Are you sure you want to change the base?

PoC: allow renewing factory root CA #51

Conversation

vkhoroz commented Jun 12, 2024

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

Choose a reason for hiding this comment

doanac 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

mike-sul commented Jun 17, 2024

vkhoroz commented Jun 19, 2024

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