Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions contrib/renew-root-cert
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#!bin/sh -e

# This is an OnChanged handler that can handle requests to renew
# the factory root CA certificate used to verify the device gateway TLS certificate.

[ -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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch! 🚀


fioconfig="$FIOCONFIG_BIN"
if [ -z "$fioconfig" ] ; then
fioconfig=$(which fioconfig)
fi

correlation_id=$(cat $CONFIG_FILE | grep CORRELATIONID | cut -d= -f2)
[ -z "$correlation_id" ] && (echo "No CORRELATIONID found in config file"; exit 1)
echo "Correlation ID for root CA update is $correlation_id"

est_server=$(cat $CONFIG_FILE | grep ESTSERVER | cut -d= -f2)
[ -z "$est_server" ] && (echo "No ESTSERVER found in config file"; exit 1)
echo "EST server for update is $est_server"

update() {
if ! $fioconfig renew-root $est_server $correlation_id ; then
exit 123
fi
exit 0
}

# We'll execute under two conditions:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

# 1) A root update has never taken place (root-update.state.completed does not exist)
# 2) We need to do a new root update ($correlation_id != root-update.state.completed's CorrelationId)

active_file="${STORAGE_DIR}/root-ca-update.state"
Comment thread
StealthyCoder marked this conversation as resolved.
completed_file="${active_file}.completed"

if [ ! -f "$completed_file" ] ; then
echo "Updating root CA: root-ca-update.state.completed does not exist"
update
fi

idmatch=$(grep -Po '"RotationId":.*?[^\\]",' $completed_file || true)
completed_id=$(echo $idmatch | cut -d\" -f4)
if [ "$completed_id" != "$correlation_id" ] ; then
echo "Updating root CA: Correlation ID has changed from $completed_id -> $correlation_id"
update
fi

echo "Root CA update not needed. Current correlation ID is $correlation_id"
14 changes: 9 additions & 5 deletions internal/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ type App struct {
EncryptedConfig string
SecretsDir string

configUrl string
configPaths []string
unsafeHandlers bool
sota *AppConfig
configUrl string
configPaths []string
unsafeHandlers bool
unsafeCaRenewal bool
sota *AppConfig

exitFunc func(int)
}
Expand Down Expand Up @@ -164,7 +165,9 @@ func createClient(sota *AppConfig) (*http.Client, CryptoHandler) {
return createClientPkcs11(sota)
}

func NewApp(configPaths []string, secrets_dir string, unsafeHandlers, testing bool) (*App, error) {
func NewApp(
configPaths []string, secrets_dir string, unsafeHandlers, unsafeCaRenewal, testing bool,
) (*App, error) {
if len(configPaths) == 0 {
configPaths = DEF_CONFIG_ORDER
}
Expand Down Expand Up @@ -193,6 +196,7 @@ func NewApp(configPaths []string, secrets_dir string, unsafeHandlers, testing bo
configPaths: configPaths,
sota: sota,
unsafeHandlers: unsafeHandlers,
unsafeCaRenewal: unsafeCaRenewal,
exitFunc: os.Exit,
}

Expand Down
9 changes: 8 additions & 1 deletion internal/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,13 @@ func testWrapper(t *testing.T, doGet http.HandlerFunc, testFunc func(app *App, c
t.Fatal(err)
}
certOut.Close()
// The root.key is used by the TestRenewRoot
if bytes, err := x509.MarshalPKCS8PrivateKey(ts.TLS.Certificates[0].PrivateKey); err != nil {
t.Fatal(err)
} else if err = os.WriteFile(filepath.Join(dir, "root.key"), bytes, 0644); err != nil {
t.Fatal(err)
}

if err := os.WriteFile(filepath.Join(dir, "pkey.pem"), []byte(pkey_pem), 0644); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -139,7 +146,7 @@ path = "%s"
if config["bar"].Value != "bar file value" {
t.Fatal("Encryption of bar should not have occurred")
}
app, err := NewApp([]string{dir}, dir, true, true)
app, err := NewApp([]string{dir}, dir, true, false, true)
app.configUrl = ts.URL
if err != nil {
t.Fatal(err)
Expand Down
215 changes: 215 additions & 0 deletions internal/renew_root.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
package internal

import (
"bytes"
"crypto"
"crypto/x509"
"encoding/pem"
"errors"
"fmt"
"io"
"log"
"os"
)

type RootRenewalState struct {
BaseState
EstServer string
}

type rootRenewalContext = stateContext[*RootRenewalState]
type rootRenewalStep = stateStep[*RootRenewalState]

// Not type RootRenewalHandler stateHandler[*RootRenewalState].
// We want methods from a parent to be inherited, thus use struct composition.
type RootRenewalHandler struct {
stateHandler[*RootRenewalState]
}

func NewRootRenewalHandler(app *App, stateFile, estServer string) *RootRenewalHandler {
state := &RootRenewalState{EstServer: estServer}
return &RootRenewalHandler{
stateHandler[*RootRenewalState]{
stateContext: newStateContext[*RootRenewalState](app, stateFile, state),
steps: []rootRenewalStep{
fetchRootStep{},
},
},
}
}

func RestoreRootRenewalHandler(app *App, stateFile string) *RootRenewalHandler {
handler := NewRootRenewalHandler(app, stateFile, "")
if ok := handler.Restore(); !ok {
handler = nil
}
return handler
}

func (h *RootRenewalHandler) Update() error {
return h.execute("RootCaUpdateStarted", "RootCaUpdateCompleted", true)
}

func (h *RootRenewalHandler) Resume(online bool) error {
if !online {
log.Print("Incomplete root CA renewal state found.")
return nil
}
log.Print("Incomplete root CA renewal state found. Will attempt to complete")
return h.Update()
}

type fetchRootStep struct{}

func (s fetchRootStep) Name() string {
return "Fetch new root"
}

func (s fetchRootStep) Execute(h *rootRenewalContext) error {
caFile := h.app.sota.GetOrDie("import.tls_cacert_path")
caCertBuf, err := os.ReadFile(caFile)
if err != nil {
log.Fatal("Failed to read root CA file", err)
}
caCerts, err := loadCertsFromPem(caCertBuf)
if err != nil {
log.Fatal("Failed to parse root CA file", err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

}

url := h.State.EstServer + "/cacerts"
res, err := h.client.Get(url)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

if err != nil {
return fmt.Errorf("Unable to submit root certificate request: %w", err)
}
buf, err := io.ReadAll(res.Body)
if err != nil {
return fmt.Errorf("Unable to read root certificate response body: HTTP_%d - %w", res.StatusCode, err)
}
if res.StatusCode != 200 {
return fmt.Errorf("Unable to obtain root certificate: HTTP_%d - %s", res.StatusCode, string(buf))
}
ct := res.Header.Get("content-type")
if ct != "application/pkcs7-mime" {
return fmt.Errorf("Unexpected content-type return in root certificate response: %s", ct)
}
certs, err := decodeEstResponse(string(buf))
if err != nil {
return err
}

if err = validateRootCerts(caCerts, certs, h.app.unsafeCaRenewal); err != nil {
return fmt.Errorf("Error validating root certificates: %w", err)
}

var content bytes.Buffer
for _, c := range certs {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

content.Write(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: c.Raw}))
}
if err = safeWrite(caFile, content.Bytes()); err != nil {
return fmt.Errorf("Error updating root certificates file: %w", err)
}
return nil
}

func loadCertsFromPem(data []byte) (certs []*x509.Certificate, err error) {
var block *pem.Block
if len(data) == 0 {
return nil, errors.New("Unexpected empty PEM block")
}
for len(data) > 0 {
if block, data = pem.Decode(data); block == nil {
return nil, fmt.Errorf("Malformed PEM block at %s", data[:100])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Makes sense.

} else if block.Type != "CERTIFICATE" {
return nil, fmt.Errorf("Invalid PEM block type: %s", block.Type)
} else if c, err := x509.ParseCertificate(block.Bytes); err != nil {
return nil, fmt.Errorf("Invalid X.509 certificate: %w", err)
} else {
certs = append(certs, c)
}
}
return
}

// As per crypto.PublicKey documentation, all public keys implement this interface.
type publicKey interface {
Equal(x crypto.PublicKey) bool
}

func validateRootCerts(curCerts, newCerts []*x509.Certificate, skipSignatureCheck bool) error {
// Each new certificate must pass all of the below checks:
// 1. It is a valid certificate authority.
Comment thread
doanac marked this conversation as resolved.
// 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.
// Requirement 3 allows safely and securely rotating existing root CA in a 2-phase process:
// - At first phase, the EST server returns 3 CAs:
// A. Current CA (self-signed or signed by a higher order CA);
// B. A new CA (self-signed or signed by a higher order CA);
// B1. A new CA with the same public key as the CA `B`, signed by a current CA.
// - At second phase, the EST server returns only the CA `B` from the above.
subj := curCerts[0].Subject.String() // All certs must have the same subject
signedKeys := make([]publicKey, 0, 1)
skipKeyCheck := make([]bool, len(newCerts)) // pre-initialized to false
for idx, cert := range newCerts {
serial := cert.SerialNumber.String()
if cert.Subject.String() != subj {
return fmt.Errorf(
"Unexpected subject '%s' in certificate with serial %s, must be '%s'",
cert.Subject.String(), serial, subj,
)
} else if !cert.IsCA {
return fmt.Errorf("Certificate with serial %s is not a certificate authority", serial)
} else if !cert.BasicConstraintsValid {
return fmt.Errorf("Certificate with serial %s failed basic constraints validation", serial)
}
// First loop identifies certificates matching condition 3.2.
if skipSignatureCheck {
skipKeyCheck[idx] = true
continue
}
for _, ca := range curCerts {
if cert.Equal(ca) {
skipKeyCheck[idx] = true
break
} else if err := cert.CheckSignatureFrom(ca); err == nil {
if pub, ok := cert.PublicKey.(publicKey); ok {
signedKeys = append(signedKeys, pub)
} else {
// According to Golang docs this should be unreachable... keep here just for sanity.
return fmt.Errorf(
"Certificate with serial %s has invalid public key type: %T", serial, cert.PublicKey)
}
skipKeyCheck[idx] = true
break
}
}
}
// Second loop identifies certificates matching condition 3.3.
for idx, cert := range newCerts {
if skipKeyCheck[idx] {
continue
}
serial := cert.SerialNumber.String()
if pub, ok := cert.PublicKey.(publicKey); ok {
var isSigned bool
for _, sigPub := range signedKeys {
if isSigned = sigPub.Equal(pub); isSigned {
break
}
}
if !isSigned {
return fmt.Errorf(
"Certificate with serial %s is neither (1) signed by one of current CAs "+
"nor (2) has the same public key as another certificate which is signed by one of current CAs",
serial)
}
} else {
// According to Golang docs this should be unreachable... keep here just for sanity.
return fmt.Errorf(
"Certificate with serial %s has invalid public key type: %T", serial, cert.PublicKey)
}
}
return nil
}
13 changes: 10 additions & 3 deletions internal/rotate_est.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,14 @@ func (s estStep) Execute(handler *certRotationContext) error {
if ct != "application/pkcs7-mime" {
return fmt.Errorf("Unexpected content-type return in certificate response: %s", ct)
}
estCert, err := decodeEstResponse(string(buf))
estCerts, err := decodeEstResponse(string(buf))
if err != nil {
return err
}
if len(estCerts) > 1 {
return fmt.Errorf("Unexpected more than one certificate in response: %d", len(estCerts))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

}
estCert := estCerts[0]

// Do minimal sanity checking on the new cert
if err = verifyNewCert(cert, estCert); err != nil {
Expand Down Expand Up @@ -180,7 +184,7 @@ func createB64CsrDer(key crypto.Signer, cert *x509.Certificate) ([]byte, error)
return []byte(base64.StdEncoding.EncodeToString(csrBytes)), nil
}

func decodeEstResponse(estResponse string) (*x509.Certificate, error) {
func decodeEstResponse(estResponse string) ([]*x509.Certificate, error) {
bytes, err := base64.StdEncoding.DecodeString(estResponse)
if err != nil {
return nil, fmt.Errorf("Unable to base64 decode EST response: %w", err)
Expand All @@ -189,7 +193,10 @@ func decodeEstResponse(estResponse string) (*x509.Certificate, error) {
if err != nil {
return nil, fmt.Errorf("Invalid pkcs7 data in EST response: %w", err)
}
return p7.Certificates[0], nil
if len(p7.Certificates) < 1 {
return nil, errors.New("Invalid pkcs7 data in EST response: no certificates")
Comment thread
doanac marked this conversation as resolved.
}
return p7.Certificates, nil
}

func verifyNewCert(curCert, newCert *x509.Certificate) error {
Expand Down
Loading