sslProxy, TLS Tutorials

[predic8]

Summary by CodeRabbit

• New Features

  • Added YAML tutorials for TLS termination and centralized SSL configuration to simplify HTTPS setup and testing.
  • Introduced a configurable proxy target model enabling dynamic destination resolution, SSL options, and host/port configuration.
  • Enhanced SSL handling to support aggregated/multi-certificate contexts and explicit connection configuration.

• Chores

  • Updated proxy-related tests and internal wiring to align with the new target and SSL handling model.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Extracted the nested Target type from AbstractServiceProxy into a top-level com.predic8.membrane.core.proxies.Target class, updated usages and imports across proxies and tests, adjusted SSLProxy annotations and accessors, refactored SSLContext/collection handling and certificate warning logic, and added YAML tutorial files for TLS/SSL configurations. Changes are mainly API migration, SSL configuration management, and tests/docs updates.

Changes

Cohort / File(s)	Summary
Core: Target extraction core/src/main/java/com/predic8/membrane/core/proxies/Target.java, core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java	Added a top-level Target class and removed the public static inner Target from AbstractServiceProxy; call sites and imports updated to use the new Target.
Core: SSL proxy & connection config core/src/main/java/com/predic8/membrane/core/proxies/SSLProxy.java	Adjusted @MCElement annotation parameters, updated target accessor signatures to reference SSLProxy.Target, added ConnectionConfiguration getter/setter, and minor formatting changes.
Core: SSL context aggregation & collection core/src/main/java/com/predic8/membrane/core/proxies/RuleManager.java, core/src/main/java/com/predic8/membrane/core/transport/ssl/SSLContextCollection.java	Introduced builder aggregation per IpPort, added useCollection flag and builder handling to return a collection when needed. Added helper getOrCreateBuilder(...).
Core: Static SSL certificate handling core/src/main/java/com/predic8/membrane/core/transport/ssl/StaticSSLContext.java	Centralized certificate warning via new warnOfOldCertificate(...), adjusted getCertificates() to parse certs and emit warnings, method now declares additional exceptions.
Core: KeyStore digest API core/src/main/java/com/predic8/membrane/core/security/KeyStoreUtil.java, core/src/test/java/.../KeyStoreUtilTest.java	Changed getDigest(...) signature to accept a Certificate instead of (KeyStore, alias); updated tests to pass certificate.
Tests & call sites core/src/test/java/com/predic8/membrane/..., distribution/src/test/java/com/predic8/membrane/...	Replaced references to AbstractServiceProxy.Target with top-level Target, updated imports (some to wildcards). No logic changes beyond type/import adjustments.
Docs / Tutorials distribution/tutorials/security/10-TLS-Termination.yaml, distribution/tutorials/security/20-Central-SSL-Config.yaml	Added two YAML tutorial configurations demonstrating TLS termination and centralized SSL config examples.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Implement expression support in Target #1863: Related changes to Target expression-based URL compilation and init/compileUrl behavior.
• JsonSchemaGenerator: Refinements for YAML completion #2240: Refactor moving nested Target to a top-level class and updating call sites.
• XML Namespace Declarations for XPath Expressions #2253: Overlapping changes to Target extraction and XML/config accessors for Target.

Suggested labels

7.x

Suggested reviewers

• predic8

Poem

🐰 A little Target hopped outside its shell,
Freed to roam where dynamic URLs dwell.
SSL flags gather, contexts align,
Tutorials bloom, tests continue to shine—
Hooray, the proxy's ready—sip some carrot tea! 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title mentions 'sslProxy' and 'TLS Tutorials' but is vague about what was actually changed; it does not clearly describe whether this is a refactoring, feature addition, or configuration update.	Consider making the title more descriptive, such as 'Refactor Target class extraction and add TLS termination tutorials' or 'Extract Target from AbstractServiceProxy and add TLS configuration examples'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[membrane-ci-server]

This pull request needs "/ok-to-test" from an authorized committer.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/src/main/java/com/predic8/membrane/core/transport/ssl/SSLContextCollection.java (1)

55-60: Potential NoSuchElementException when building with empty contexts.

If useCollection is set to true but no SSLContext instances are added, build() creates an SSLContextCollection with an empty list. Subsequent calls (e.g., wrapAcceptedSocket at line 145 calling sslContexts.getFirst()) will throw NoSuchElementException.

Consider adding validation:

🛡️ Proposed fix to add validation

 public SSLProvider build() {
+    if (sslContexts.isEmpty()) {
+        throw new IllegalStateException("Cannot build SSLProvider: no SSLContext was added");
+    }
     if (sslContexts.size() > 1 || useCollection) {
         return new SSLContextCollection(sslContexts, dnsNames);
     } else
         return sslContexts.getFirst();
 }

🧹 Nitpick comments (2)

core/src/main/java/com/predic8/membrane/core/security/KeyStoreUtil.java (1)

58-68: Drop obsolete KeyStoreException and guard cert nulls.

getDigest no longer accesses a KeyStore, so the checked KeyStoreException (and its Javadoc) is misleading. Also consider a fail-fast null check for clearer errors.

♻️ Proposed update

-import java.util.Optional;
+import java.util.Objects;
+import java.util.Optional;
@@
-     * `@throws` KeyStoreException           If there's an error accessing the KeyStore.
@@
-    public static `@org.jetbrains.annotations.NotNull` String getDigest(Certificate cert) throws CertificateEncodingException, KeyStoreException, NoSuchAlgorithmException {
-        byte[] pkeEnc = cert.getEncoded();
+    public static `@NotNull` String getDigest(`@NotNull` Certificate cert) throws CertificateEncodingException, NoSuchAlgorithmException {
+        Objects.requireNonNull(cert, "cert");
+        byte[] pkeEnc = cert.getEncoded();

core/src/main/java/com/predic8/membrane/core/transport/ssl/StaticSSLContext.java (1)

319-329: Consider making the warning mechanism more resilient.

Two observations:

1. Thread safety: defaultCertificateWarned is accessed without synchronization. Multiple threads could simultaneously pass the check and log duplicate warnings. Consider using AtomicBoolean:

2. Exception propagation: A warning utility should not fail the main operation. If getDigest() throws, the entire SSL context creation fails. Consider catching exceptions internally.

♻️ Proposed resilient implementation

+import java.util.concurrent.atomic.AtomicBoolean;

-    private static boolean defaultCertificateWarned = false;
+    private static final AtomicBoolean defaultCertificateWarned = new AtomicBoolean(false);

-    private static void warnOfOldCertificate(Certificate cert) throws CertificateEncodingException, KeyStoreException, NoSuchAlgorithmException {
-        if (!defaultCertificateWarned) {
+    private static void warnOfOldCertificate(Certificate cert) {
+        if (defaultCertificateWarned.get()) {
+            return;
+        }
+        try {
             String digest = getDigest(cert);
             if (digest.equals(DEFAULT_CERTIFICATE_SHA256_OLD) ||
                     digest.equals(DEFAULT_CERTIFICATE_SHA256)) {
-                log.warn("Using Membrane with the default certificate. This is highly discouraged! "
-                        + "Please run the generate-ssl-keys script in the conf directory.");
-                defaultCertificateWarned = true;
+                if (defaultCertificateWarned.compareAndSet(false, true)) {
+                    log.warn("Using Membrane with the default certificate. This is highly discouraged! "
+                            + "Please run the generate-ssl-keys script in the conf directory.");
+                }
             }
+        } catch (CertificateEncodingException | KeyStoreException | NoSuchAlgorithmException e) {
+            log.debug("Could not check for default certificate: {}", e.getMessage());
         }
     }

Note: If you adopt this change, you'll also need to remove the exception declarations from getCertificates (line 230) if they're no longer thrown elsewhere in that method.