Support custom ACME providers - #620
Merged
dmercuriali merged 4 commits intoAug 5, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughThe change adds configurable ACME providers with directory URLs and optional External Account Binding credentials. Certificate APIs and forms accept provider selection and expose provider data. Configuration stores maintain separate ACME account keys per provider. Dynamic certificate renewal routes orders and challenges to the selected provider. The certificate UI selects and displays providers. Assessment against linked issues:
Suggested reviewers: Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
NiccoMlt
force-pushed
the
536-acme-certificate-support-custom-acme-providers
branch
from
July 23, 2026 13:27
8d2198e to
a696ea2
Compare
NiccoMlt
force-pushed
the
536-acme-certificate-support-custom-acme-providers
branch
from
August 3, 2026 08:26
a696ea2 to
7bf8548
Compare
dmercuriali
self-requested a review
August 4, 2026 14:31
Any RFC 8555 CA via acme.<n>.* config (EAB kid/hmac supported, validated at parse time), per-certificate provider selection, per-provider account keys. Let's Encrypt stays default with the legacy account key. Closes diennea#536
NiccoMlt
force-pushed
the
536-acme-certificate-support-custom-acme-providers
branch
from
August 4, 2026 14:40
7bf8548 to
4970f11
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
carapace-server/src/test/java/org/carapaceproxy/server/certificates/AcmeFailureClassifierTest.java (1)
51-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a lazy-wrapped transient failure case.
isTransientunwrapsAcmeLazyLoadingExceptionbefore it classifies the cause. Line 45 tests that unwrapping only for a non-transient cause. No test proves that a transient cause survives the unwrapping. A change that drops the unwrap would still pass every current assertion in this class, because the non-transient path returnsfalseeither way.The lazily fetched order is exactly the path
testPendingOrderPollFailureexercises inDynamicCertificatesManagerTest, so the case is worth pinning here.💚 Proposed additional assertions
// a 5xx without a problem document, e.g., from a load balancer assertTrue(AcmeFailureClassifier.isTransient(new AcmeException("HTTP 503"))); + // a bound resource defers the round-trip and wraps the transient failure + assertTrue(AcmeFailureClassifier.isTransient(new AcmeLazyLoadingException( + Order.class, + URI.create("https://localhost/order").toURL(), + new AcmeNetworkException(new IOException("connection reset"))))); + // a message-less failure must not blow up the message match + assertFalse(AcmeFailureClassifier.isTransient(new AcmeException(null, null))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/test/java/org/carapaceproxy/server/certificates/AcmeFailureClassifierTest.java` around lines 51 - 63, Add a lazy-wrapped transient case to testTransientFailures using AcmeLazyLoadingException around a transient cause such as AcmeNetworkException, then assert AcmeFailureClassifier.isTransient returns true. This should specifically verify unwrapping preserves transient classification, complementing the existing non-transient lazy-loading test.carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java (4)
224-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the client injection into a helper.
These two lines repeat at four sites, including the
"acmeClients"field name as a string literal. A rename of the field would break all four silently, becauseWhitebox.setInternalStateresolves the name at runtime.A small private helper centralizes the field name and the ordering comment:
♻️ Proposed helper
// must run after reloadConfiguration, which rebuilds the client map; // by-name, because there are other map fields private static void injectAcmeClients(DynamicCertificatesManager man, Map<String, ACMEClient> clients) { Whitebox.setInternalState(man, "acmeClients", clients); }Each site then becomes:
man.reloadConfiguration(conf); - // after the reload, which rebuilds the client map; by-name, because there are other map fields - Whitebox.setInternalState(man, "acmeClients", Map.of(DEFAULT_PROVIDER_NAME, ac)); + injectAcmeClients(man, Map.of(DEFAULT_PROVIDER_NAME, ac));Also applies to: 403-404, 544-545, 656-657
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java` around lines 224 - 225, Extract the repeated Whitebox injection into a private static injectAcmeClients helper accepting DynamicCertificatesManager and Map<String, ACMEClient>, centralizing the "acmeClients" field name and ordering comment. Replace all four direct Whitebox.setInternalState calls in DynamicCertificatesManagerTest with this helper, preserving the existing client maps and call ordering after reloadConfiguration.
776-785: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the transient branch persists nothing.
The transient case checks that the state stays
ORDERINGand the attempts count stays0. It does not check that the manager skipped the write. Add a direct assertion, because "do not touch the persisted state" is the contract under test:💚 Proposed additional assertion
} else { // state untouched, will be retried at the next cycle assertCertificateState(domain, ORDERING, 0, man); + verify(store, never()).saveCertificate(any()); }For the stale branch, consider asserting the recorded message as well, so a regression in the
ex.getMessage()fallback onDynamicCertificatesManagerline 400 is caught:assertEquals("unknown order", man.getCertificateDataForDomain(domain).getMessage());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java` around lines 776 - 785, Extend the transient branch of the failure-case test around assertCertificateState to verify that no certificate data was persisted, using the manager’s existing persisted-state accessor. In the stale branch, also assert that getCertificateDataForDomain(domain).getMessage() records “unknown order” after the fallback path.
728-731: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that neither client received the other provider's order.
verify(letsencryptClient, times(1)).createOrderForDomain(eq(leNames))counts only the calls that matchleNames. It still passes ifletsencryptClientalso receivedcustom.local. The same holds forcustomClient. The test therefore proves each client got its own order, but not that routing is exclusive, which is the property the test is named for.💚 Proposed additional assertions
verify(letsencryptClient, times(1)) .createOrderForDomain(eq(new CertificateData("le.local", null, WAITING).getNames())); verify(customClient, times(1)) .createOrderForDomain(eq(new CertificateData("custom.local", null, WAITING).getNames())); + // routing is exclusive: neither client sees the other provider's certificate + verify(letsencryptClient, never()) + .createOrderForDomain(eq(new CertificateData("custom.local", null, WAITING).getNames())); + verify(customClient, never()) + .createOrderForDomain(eq(new CertificateData("le.local", null, WAITING).getNames()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java` around lines 728 - 731, Strengthen the routing assertions in DynamicCertificatesManagerTest around the letsencryptClient and customClient createOrderForDomain verifications by also verifying that each client never received the other provider’s domain names. Keep the existing positive exactly-once assertions and add negative verifications for the cross-provider orders.
92-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate DNS challenge limit property sets.
DynamicCertificatesManager.DNS_CHALLENGE_REACHABILITY_CHECKS_LIMITis initialized once when the class loads, so the laterSystem.setProperty(...)calls intestWildcardCertificateStateManagementandtestSanCertificateStateManagementdo not change the tested limit. Remove those calls to avoid relying on stale test setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java` around lines 92 - 98, Remove the later System.setProperty calls for carapace.acme.dnschallengereachabilitycheck.limit from testWildcardCertificateStateManagement and testSanCertificateStateManagement; retain the class-level setup executed before DynamicCertificatesManager loads so both tests use the intended limit.carapace-server/src/main/java/org/carapaceproxy/server/certificates/AcmeFailureClassifier.java (1)
61-72: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffConsider bounding retries after
AcmeRateLimitedException.
isTransientreturnstrueforAcmeRateLimitedException. The caller then leaves the certificate state untouched and does not increment the attempts counter. The next manager cycle therefore repeats the same request against a CA that already rejected it for rate limiting, with no cap and no backoff. Let's Encrypt rate limits can last hours, so this loop can extend the limit window.Two options:
- Keep the rate-limited failure transient, but record
AcmeRateLimitedException.getRetryAfter()and skip the certificate until that instant passes.- Count the rate-limited failure like other failures, so the configured
maxattemptscaps it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/main/java/org/carapaceproxy/server/certificates/AcmeFailureClassifier.java` around lines 61 - 72, Update the handling of AcmeRateLimitedException in isTransient and its caller so rate-limited certificates cannot retry indefinitely without backoff or attempt limits. Either persist getRetryAfter() and skip requests until that time, or process the exception as a counted failure governed by maxattempts; preserve transient handling for other retryable ACME failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@carapace-server/src/main/java/org/carapaceproxy/api/CertificatesResource.java`:
- Around line 271-285: At the anchor location (lines 271-285), ensure that the
call to updateDynamicCertificateForDomain occurs within the same configuration
lock transaction where provider ownership and domain conflicts are validated, OR
implement rollback to restore the previous certificate state if the
configuration apply fails after the call to updateDynamicCertificateForDomain.
At the sibling location (lines 388-411), apply the same atomic mutation pattern
to the certificate upload flow so that failed configuration applications do not
leave persisted certificate data orphaned.
In
`@carapace-server/src/main/java/org/carapaceproxy/configstore/HerdDBConfigurationStore.java`:
- Around line 343-345: Update saveAcmeUserKey and its saveKeyPair interaction so
false is returned only when the key already exists; propagate connection,
storage, and other non-duplicate SQLException failures according to the
ConfigurationStore.saveAcmeUserKey contract. Preserve the existing success
behavior and identify duplicate-key errors explicitly rather than treating every
SQLException as a duplicate.
In
`@carapace-server/src/main/java/org/carapaceproxy/core/RuntimeServerConfiguration.java`:
- Around line 368-389: The configuration save flow must prevent raw
acme.<n>.hmac values from appearing in logs or persisted configuration. Update
HerdDBConfigurationStore.commitConfiguration() to redact or mask each HMAC
property before its value is logged or stored, while preserving other properties
unchanged, and add a regression test asserting the logged output does not
contain the raw MAC key.
In
`@carapace-server/src/main/java/org/carapaceproxy/server/certificates/AcmeFailureClassifier.java`:
- Around line 61-72: Update AcmeFailureClassifier.isTransient to preserve
AcmeRateLimitedException.getRetryAfter() for the caller instead of reducing it
to a boolean; in DynamicCertificatesManager’s transient-failure path, track
consecutive failures per domain, delay retries with backoff or the rate-limit
retry time, and record the failure on the certificate after a threshold so
maxattempts and the UI observe it. Apply changes in
carapace-server/src/main/java/org/carapaceproxy/server/certificates/AcmeFailureClassifier.java
lines 61-72 and
carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java
lines 395-399.
In `@carapace-ui/src/main/webapp/src/components/certificates/CertificateForm.vue`:
- Around line 67-82: Update the CertificateForm submit flow around handleOk()
and handleSubmit() to reject ACME submissions when form.provider is empty,
preventing the incomplete form from being sent to /api/certificates/. Also keep
the Create action disabled until a valid provider selection is available, while
preserving submission behavior for non-ACME certificate types.
---
Nitpick comments:
In
`@carapace-server/src/main/java/org/carapaceproxy/server/certificates/AcmeFailureClassifier.java`:
- Around line 61-72: Update the handling of AcmeRateLimitedException in
isTransient and its caller so rate-limited certificates cannot retry
indefinitely without backoff or attempt limits. Either persist getRetryAfter()
and skip requests until that time, or process the exception as a counted failure
governed by maxattempts; preserve transient handling for other retryable ACME
failures.
In
`@carapace-server/src/test/java/org/carapaceproxy/server/certificates/AcmeFailureClassifierTest.java`:
- Around line 51-63: Add a lazy-wrapped transient case to testTransientFailures
using AcmeLazyLoadingException around a transient cause such as
AcmeNetworkException, then assert AcmeFailureClassifier.isTransient returns
true. This should specifically verify unwrapping preserves transient
classification, complementing the existing non-transient lazy-loading test.
In
`@carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java`:
- Around line 224-225: Extract the repeated Whitebox injection into a private
static injectAcmeClients helper accepting DynamicCertificatesManager and
Map<String, ACMEClient>, centralizing the "acmeClients" field name and ordering
comment. Replace all four direct Whitebox.setInternalState calls in
DynamicCertificatesManagerTest with this helper, preserving the existing client
maps and call ordering after reloadConfiguration.
- Around line 776-785: Extend the transient branch of the failure-case test
around assertCertificateState to verify that no certificate data was persisted,
using the manager’s existing persisted-state accessor. In the stale branch, also
assert that getCertificateDataForDomain(domain).getMessage() records “unknown
order” after the fallback path.
- Around line 728-731: Strengthen the routing assertions in
DynamicCertificatesManagerTest around the letsencryptClient and customClient
createOrderForDomain verifications by also verifying that each client never
received the other provider’s domain names. Keep the existing positive
exactly-once assertions and add negative verifications for the cross-provider
orders.
- Around line 92-98: Remove the later System.setProperty calls for
carapace.acme.dnschallengereachabilitycheck.limit from
testWildcardCertificateStateManagement and testSanCertificateStateManagement;
retain the class-level setup executed before DynamicCertificatesManager loads so
both tests use the intended limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cf060c0-03e9-4d32-a638-35af274aa951
📒 Files selected for processing (22)
carapace-server/src/main/java/org/carapaceproxy/api/CertificatesResource.javacarapace-server/src/main/java/org/carapaceproxy/configstore/CertificateData.javacarapace-server/src/main/java/org/carapaceproxy/configstore/ConfigurationStore.javacarapace-server/src/main/java/org/carapaceproxy/configstore/HerdDBConfigurationStore.javacarapace-server/src/main/java/org/carapaceproxy/configstore/PropertiesConfigurationStore.javacarapace-server/src/main/java/org/carapaceproxy/core/HttpProxyServer.javacarapace-server/src/main/java/org/carapaceproxy/core/RuntimeServerConfiguration.javacarapace-server/src/main/java/org/carapaceproxy/server/certificates/ACMEClient.javacarapace-server/src/main/java/org/carapaceproxy/server/certificates/AcmeFailureClassifier.javacarapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.javacarapace-server/src/main/java/org/carapaceproxy/server/config/AcmeProviderConfiguration.javacarapace-server/src/main/java/org/carapaceproxy/server/config/SSLCertificateConfiguration.javacarapace-server/src/main/resources/conf/server.dynamic.propertiescarapace-server/src/test/java/org/carapaceproxy/configstore/ConfigurationStoreTest.javacarapace-server/src/test/java/org/carapaceproxy/core/RuntimeServerConfigurationTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/ACMEClientTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/AcmeFailureClassifierTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/CertificatesTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.javacarapace-ui/src/main/webapp/src/components/certificates/Certificate.vuecarapace-ui/src/main/webapp/src/components/certificates/CertificateForm.vuecarapace-ui/src/main/webapp/src/components/certificates/Certificates.vue
CA rejections (stale orders, malformed responses) and unexpected local failures bump the cert attempts and fall back to a fresh order; network errors, rate limits and CA outages retry in the same state. Order polls fetch once per cycle. Policy lives in AcmeFailureClassifier with fast pure unit tests.
NiccoMlt
force-pushed
the
536-acme-certificate-support-custom-acme-providers
branch
from
August 4, 2026 22:03
eb29216 to
d57bbc2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java`:
- Around line 174-183: Update the reload flow around
DynamicCertificatesManager’s ACME client construction and loadCertificates() so
clients and the certificate cache are built in local variables first. Publish
both together only after certificate loading completes successfully, preserving
the previous runtime state unchanged when reload fails and keeping
acmeClientFor() consistent with the active certificate cache.
- Around line 395-408: Update the exception handling around the dynamic
certificate operation in DynamicCertificatesManager so
store.saveCertificate(cert) failures are handled independently from ACME failure
classification. After ACME state or remote side effects change, retry saving the
exact post-operation certificate state; do not invoke cert.error() or otherwise
route persistence failures through certificate failure handling, and preserve
the renewal task from being cancelled.
In
`@carapace-server/src/test/java/org/carapaceproxy/server/certificates/CertificatesTest.java`:
- Around line 492-494: Update the restoration assertions in the certificate test
to also compare previous.getProvider() with restored.getProvider(), ensuring a
failed update does not persist the changed provider value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dca8182-ead7-4eb2-bf26-2b53ad683089
📒 Files selected for processing (22)
carapace-server/src/main/java/org/carapaceproxy/api/CertificatesResource.javacarapace-server/src/main/java/org/carapaceproxy/configstore/CertificateData.javacarapace-server/src/main/java/org/carapaceproxy/configstore/ConfigurationStore.javacarapace-server/src/main/java/org/carapaceproxy/configstore/HerdDBConfigurationStore.javacarapace-server/src/main/java/org/carapaceproxy/configstore/PropertiesConfigurationStore.javacarapace-server/src/main/java/org/carapaceproxy/core/HttpProxyServer.javacarapace-server/src/main/java/org/carapaceproxy/core/RuntimeServerConfiguration.javacarapace-server/src/main/java/org/carapaceproxy/server/certificates/ACMEClient.javacarapace-server/src/main/java/org/carapaceproxy/server/certificates/AcmeFailureClassifier.javacarapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.javacarapace-server/src/main/java/org/carapaceproxy/server/config/AcmeProviderConfiguration.javacarapace-server/src/main/java/org/carapaceproxy/server/config/SSLCertificateConfiguration.javacarapace-server/src/main/resources/conf/server.dynamic.propertiescarapace-server/src/test/java/org/carapaceproxy/configstore/ConfigurationStoreTest.javacarapace-server/src/test/java/org/carapaceproxy/core/RuntimeServerConfigurationTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/ACMEClientTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/AcmeFailureClassifierTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/CertificatesTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.javacarapace-ui/src/main/webapp/src/components/certificates/Certificate.vuecarapace-ui/src/main/webapp/src/components/certificates/CertificateForm.vuecarapace-ui/src/main/webapp/src/components/certificates/Certificates.vue
🚧 Files skipped from review as they are similar to previous changes (17)
- carapace-ui/src/main/webapp/src/components/certificates/Certificate.vue
- carapace-server/src/main/java/org/carapaceproxy/configstore/ConfigurationStore.java
- carapace-server/src/main/java/org/carapaceproxy/configstore/CertificateData.java
- carapace-server/src/test/java/org/carapaceproxy/server/certificates/AcmeFailureClassifierTest.java
- carapace-server/src/test/java/org/carapaceproxy/server/certificates/ACMEClientTest.java
- carapace-server/src/main/java/org/carapaceproxy/server/config/SSLCertificateConfiguration.java
- carapace-server/src/main/resources/conf/server.dynamic.properties
- carapace-ui/src/main/webapp/src/components/certificates/CertificateForm.vue
- carapace-server/src/main/java/org/carapaceproxy/server/config/AcmeProviderConfiguration.java
- carapace-ui/src/main/webapp/src/components/certificates/Certificates.vue
- carapace-server/src/main/java/org/carapaceproxy/configstore/PropertiesConfigurationStore.java
- carapace-server/src/main/java/org/carapaceproxy/core/HttpProxyServer.java
- carapace-server/src/test/java/org/carapaceproxy/configstore/ConfigurationStoreTest.java
- carapace-server/src/main/java/org/carapaceproxy/core/RuntimeServerConfiguration.java
- carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java
- carapace-server/src/main/java/org/carapaceproxy/api/CertificatesResource.java
- carapace-server/src/main/java/org/carapaceproxy/configstore/HerdDBConfigurationStore.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java (2)
175-188: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPublish all reload-dependent state only after loading succeeds.
keyPairsSizechanges at Line 174 anddomainsCheckerIPAddresseschanges at Line 184 beforeloadCertificates(...)runs at Line 185. If loading fails, Lines 187-188 do not run, but those fields retain the new values. The next lifecycle can process the old certificate and client state with new reload settings. Stage all reload-dependent values and publish them together after loading succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java` around lines 175 - 188, Stage keyPairsSize and domainsCheckerIPAddresses in local reload variables alongside the rebuilt ACME clients and certificates, without mutating manager fields before loadCertificates succeeds. After all reload-dependent values load successfully, publish keyPairsSize, domainsCheckerIPAddresses, acmeClients, and certificates together in reloadConfiguration; preserve the existing state unchanged when loading fails.
270-273: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReset in-flight ACME state when the provider changes.
loadOrCreateDynamicCertificateForDomainreloads the persisted order and challenge state, then overwrites the provider at Line 272. If a certificate changes provider while it isDNS_CHALLENGE_WAIT,VERIFYING,VERIFIED, orORDERING, Lines 345-359, 551, and 572 use the new provider with the previous provider's order URL or challenge data. Reset and clean up the old ACME state before switching providers, or reject the change until no order or challenge is pending.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java` around lines 270 - 273, Update loadOrCreateDynamicCertificateForDomain so changing the provider does not retain in-flight ACME state from the previous provider. Before cert.setProvider(provider), detect a provider change and either clear the persisted order, challenge, and related ACME state with the required cleanup, or reject the change while an operation is pending; preserve normal renewal behavior when the provider is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java`:
- Around line 187-188: Update DynamicCertificatesManager’s reloadConfiguration,
certificatesLifecycle, and reloadCertificatesFromDBInternal flows so acmeClients
and certificates are published atomically as one immutable runtime-state
snapshot, or consistently protected by the same mutex. Ensure readers cannot
observe values from different reloads or a later database reload.
---
Outside diff comments:
In
`@carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.java`:
- Around line 175-188: Stage keyPairsSize and domainsCheckerIPAddresses in local
reload variables alongside the rebuilt ACME clients and certificates, without
mutating manager fields before loadCertificates succeeds. After all
reload-dependent values load successfully, publish keyPairsSize,
domainsCheckerIPAddresses, acmeClients, and certificates together in
reloadConfiguration; preserve the existing state unchanged when loading fails.
- Around line 270-273: Update loadOrCreateDynamicCertificateForDomain so
changing the provider does not retain in-flight ACME state from the previous
provider. Before cert.setProvider(provider), detect a provider change and either
clear the persisted order, challenge, and related ACME state with the required
cleanup, or reject the change while an operation is pending; preserve normal
renewal behavior when the provider is unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 54247e35-d020-4f29-aadf-b4f0fa761945
📒 Files selected for processing (3)
carapace-server/src/main/java/org/carapaceproxy/server/certificates/DynamicCertificatesManager.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/CertificatesTest.javacarapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- carapace-server/src/test/java/org/carapaceproxy/server/certificates/CertificatesTest.java
- carapace-server/src/test/java/org/carapaceproxy/server/certificates/DynamicCertificatesManagerTest.java
HerdDB duplicate keys are typed, so real database failures now propagate instead of masking as duplicates; secrets are masked in configuration save logs. A failed configuration apply restores the previously stored certificate row, and the UI blocks submits until the provider list is available.
All reload-dependent state (key size, checker IPs, clients, certificate cache) publishes together only after loading succeeds. Store failures in the renewal loop are logged and retried next cycle instead of burning attempts as CA rejections.
NiccoMlt
force-pushed
the
536-acme-certificate-support-custom-acme-providers
branch
from
August 5, 2026 08:22
229d1cb to
bd5f456
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
dmercuriali
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementation
Certificates can now be issued by any RFC 8555 compliant CA, not just Let's Encrypt.
Closes #536.
Note
Stacked on #619 so merge that first; this PR should get retargeted to
masterafterwards automatically.Caution
CI mocks the ACME boundary, so external account binding and custom directory URLs are not exercised end-to-end; QA will verify against a real provider. A local run against Pebble) is a meaningful smoke check.
Configuration
Providers are configured with a new configuration family and referenced per certificate:
If no provider is specified, Let's Encrypt is used, as before. Therefore, the
letsencryptname is reserved for the built-in provider.kidandhmacare optional, but both-or-neither; those allow logging in with EAB (External Account Binding) credentials, if required by the provider.Built-in
letsencryptprovider still stores the account key under the legacy_acmeuserkeyentry, while custom providers use_acmeuserkey_<name>, e.g._acmeuserkey_digicert.Important
carapace.acme.testmodeJVM system property keeps affecting only the built-in Let's Encrypt provider (staging vs production); custom providers always use their configured URL.REST APIs
POST /api/certificatesacceptsprovidertoo.POST /api/certificates/{domain}/uploadaccepts provider query param too (ACME type only);GET /api/certificatesexposes each certificate'sprovideracmeProviders, that feeds the UI select.Web UI