Skip to content

Conversation

@philippta
Copy link

@philippta philippta commented Nov 26, 2025

How certmagic currently stores certs

In certmagic, a certificate consists of three different entities:

  • Certificate (.crt)
  • Private key (.key)
  • Meta data (issuer and renewal details) (.json)

These entities are stored and retrieved individually using the .Load() and .Store() functions, when obtaining or renewing a cert.

Problem

The current implementation has two important problems:

  • Storing and loading certificates and private keys are non-atomic operations, causing this issue:
  • Failing to store one entitiy can cause other entiries to be deleted.
    • certmagic/storage.go

      Lines 193 to 205 in 20b57b0

      // storeTx stores all the values or none at all.
      func storeTx(ctx context.Context, s Storage, all []keyValue) error {
      for i, kv := range all {
      err := s.Store(ctx, kv.key, kv.value)
      if err != nil {
      for j := i - 1; j >= 0; j-- {
      s.Delete(ctx, all[j].key)
      }
      return err
      }
      }
      return nil
      }

Solution

This change adresses the issue by storing all three entities as a single certificate bundle (.bundle).

As we're operating on a database with live certificates, this change also introduces the concept of a storage mode, that allows for seemless migration from the old to the new storage format:

const (
	// Storage mode controls the format in which certificates are stored in `Storage`.
	//
	// Formats:
	// - legacy: Store cert, privkey and meta as three separate storage items (.cert, .key, .json).
	// - bundle: Store cert, privkey and meta as a single, bundled storage item (.bundle).
	//
	// Modes:
	// - legacy:     Store and load certificates in legacy format.
	// - transition: Store and load certificates in legacy and bundle format.
	// - bundle:     Store and load certificates in bundle format.
	//
	// In the transition mode, failures around reads and writes of the bundle are soft.
	// They should only log errors and try to work with the legacy format as fallback.
	// Operations on the legacy format are hard-failures, implying that errors should be propagated up.
	//
	// The storage mode is controlled via the CERTMAGIC_STORAGE_MODE environment variable
	StorageModeEnv = "CERTMAGIC_STORAGE_MODE"

	StorageModeLegacy     = "legacy"
	StorageModeTransition = "transition"
	StorageModeBundle     = "bundle"
)

REVIEW DISCLAIMER

I did not try to be smart about the alternate storage implementation:

  • Most of the functions that interacted with Storage have been duplicated and updated.
  • For all those function, a feature flag switch was added.
  • I refrained from abstracting the storage logic further.
  • With that we can still integrate any upstream changes from certmagic easily.
  • This leaves also us the easy option of removing all ...Legacy functions after a full switch.

This is pretty much the calling sequence for all functions that do storage things:

// ---------------------------
// BEFORE
// ---------------------------
func storeCertResource(...) {
    // store crt, key, meta
}
// ---------------------------
// AFTER
// ---------------------------
func storeCertResource(...) { // keeps the original function signature
    switch storageMode {
    case "legacy": 
        storeCertResourceLegacy(...)

    case "transition":
        storeCertResourceLegacy(...)
        storeCertResourceBundle(...)

    case "bundle":
        storeCertResourceBundle(...)
    }
}

func storeCertResourceLegacy(...) { // the original unmodified function
    // store crt, key, meta
}

func storeCertResourceLegacy(...) { // the original function but with bundle format
    // store bundle
}

@philippta philippta force-pushed the feature/cert-storage branch from aadbadb to 85ab379 Compare December 2, 2025 12:06
@philippta philippta changed the title Implement CertificateResourceStorage as an atomic certificate storage Store CertificateResource as one unit Dec 2, 2025
Copy link

@ErikBooijFR ErikBooijFR left a comment

Choose a reason for hiding this comment

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

I love it, I have very high hopes for this change. Some nits mostly

philippta and others added 3 commits December 9, 2025 12:38
@philippta philippta marked this pull request as ready for review December 9, 2025 13:23
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Comment @cursor review or bugbot run to trigger another review on this PR

@philippta
Copy link
Author

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Comment @cursor review or bugbot run to trigger another review on this PR

@philippta
Copy link
Author

@cursor review

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.

3 participants