Make server-ca optional in Cloud Logging credentials by JannikBrand · Pull Request #432 · SAP/cf-java-logging-support

JannikBrand · 2026-09-10T07:33:30Z

What

Make server-ca optional in the auto-configured OTLP exporters, and when it is present, add it to the default Node.js trust store (rather than replacing the trust store with it).

Why

SAP Cloud Logging's OTLP ingest endpoint presents a server certificate that chains to a public root (Let's Encrypt ISRG Root X1). Java's (e.g. Sapmachine) cacerts may already trusts ISRG X1 and X2. The server-ca field in the binding response is a convenience for runtimes that don't already trust public roots.

Under the previous "replace" semantics:

The server-ca value from the binding became the sole trust anchor. If the endpoint's certificate ever chained to a different public root than the one pinned in the binding it would fail.
The system trust store was effectively dead code inside our SDK, which contradicts the guidance in the paired docs update (SAP-docs/btp-cloud-logging#61) that "the runtime's system trust store is sufficient in the common case".

We add server-ca rather than replace the system trust store with it because bindings today always ship server-ca. So a replacing it would never exercise the system trust store and would break on CA rotations.

Testing

mvn -pl cf-java-logging-support-opentelemetry-agent-extension test — 274 tests pass, 0 failures (SapMachine 17.0.20.1).
Not yet exercised end-to-end against a real Cloud Logging tenant with the Java agent — happy to run through a testing checklist or coordinate with maintainers on a canary tenant.
No changes to public API.

Companion changes

SAP-docs/btp-cloud-logging#61 (docs)
SAP/opentelemetry-exporter-for-sap-cloud-logging-for-nodejs#47 (same treatment for the Node helper)
Internal reference: PERFX-10387.

Signed-off-by: Jannik Brand <jannik.brand@sap.com>

cla-assistant · 2026-09-10T07:35:07Z

All committers have signed the CLA.

KarstenSchnitter

Please comment whether you tested this with a real application and the Java agent. Up to now, the agent would not create a connection without an explicit server certificate to trust. The agent would just not connect and create errors. This change can only work if the implementation of the agent changed.
Note, that other exporters circumvent this restriction by downloading the server certificate if it was not provided.

KarstenSchnitter · 2026-09-10T08:11:33Z

+
+        byte[] serverCert = credentials.getServerCert();
+        if (serverCert != null && serverCert.length > 0) {
+            builder.setTrustedCertificates(serverCert);


Doing this conditionally is known to break the Java agent. It would not establish the connection. You may want to change the TLS setup using builder.setSslContext() instead. This would also be a way to exchange client key and certificate without restart in future.

Signed-off-by: Jannik Brand <jannik.brand@sap.com>

KarstenSchnitter

The original issue is unresolved. When no server-ca is given this implementation will crash.

KarstenSchnitter · 2026-09-10T12:10:50Z

Quite honestly, I would like to see a new interface being introduced based on Supplier<X509Certificate>. That could be implemented by the server-ca from the credentials, the ServerCertificateDownloader or this approach using the system certificates. Using a new mapper / joiner class, this should generate the input to setTrustedCertificates.

KarstenSchnitter

Please look for other places in this extension, where the PemEncoder could be used.

KarstenSchnitter · 2026-09-10T14:06:30Z

               .setClientTls(credentials.getClientKey(), credentials.getClientCert())
-               .setTrustedCertificates(credentials.getServerCert()).setRetryPolicy(RetryPolicy.getDefault());
+               .setTrustedCertificates(TrustedCertificatesJoiner.toPemBytes(
+                       new SystemTrustAnchorSource(),


I would actually expect, that an explicit certificate takes precedence over the system trust store. Furthermore, if the server-ca is provided explicitly, why would you still trust the default certs?

- Inject Function<CloudLoggingCredentials, byte[]> into all three exporter providers so the joiner can be replaced in tests. - Rename X509CertificateSource#get() to stream(). - SystemTrustAnchorSource: merge accepted issuers from every X509TrustManager returned by the default TrustManagerFactory. - PemEncoder: use UTF-8 for the base64 line separator. - Tests: share the loaded certificate as a constant in PemEncoderTest; use Assumptions.assumeThat for the resource-not-null precondition in the tls tests; fold the join-order assertion into the concatenation test.

KarstenSchnitter

There is still some code duplication. But the major question is why to use the system trust store when an explicit server-ca was provided.

KarstenSchnitter · 2026-09-11T13:14:23Z

+             credentials -> TrustedCertificatesJoiner.toPemBytes(new SystemTrustAnchorSource(),
+                                                                  new BindingServerCertificateSource(credentials.getServerCert())));


I would actually expect, that an explicit certificate takes precedence over the system trust store. Furthermore, if the server-ca is provided explicitly, why would you still trust the default certs?

KarstenSchnitter · 2026-09-11T13:17:57Z

+        try {
+            TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+            tmf.init((KeyStore) null);
+            return Arrays.stream(tmf.getTrustManagers())


Can tmf.getTrustManagers be null?

KarstenSchnitter · 2026-09-11T13:18:28Z

+            return Arrays.stream(tmf.getTrustManagers())
+                         .filter(X509TrustManager.class::isInstance)
+                         .map(X509TrustManager.class::cast)
+                         .flatMap(tm -> Arrays.stream(tm.getAcceptedIssuers()));


Can tm.get.getAcceptedIssuers be null?

KarstenSchnitter · 2026-09-11T13:23:46Z

+        try {
+            CertificateFactory factory = CertificateFactory.getInstance("X.509");
+            X509Certificate certificate = (X509Certificate) factory
+                    .generateCertificate(new ByteArrayInputStream(pem.getBytes(StandardCharsets.UTF_8)));
+            return Stream.of(certificate);
+        } catch (CertificateException e) {
+            LOG.log(Level.WARNING, e, () -> "Failed to parse server certificate downloaded from " + endpointUrl
+                    + "; it will be omitted from the trust anchors.");
+            return Stream.empty();
+        }


This is largely duplicate code to the BindingServerCertificateSource.

KarstenSchnitter · 2026-09-11T14:24:41Z

+        }
+        try {
+            CertificateFactory factory = CertificateFactory.getInstance("X.509");
+            X509Certificate certificate = (X509Certificate) factory.generateCertificate(new ByteArrayInputStream(pemBytes));


Please use generateCertificates (in plural) to get the full certificate chain that might be provided within the pemBytes.

JannikBrand · 2026-09-11T14:29:36Z

There is still some code duplication. But the major question is why to use the system trust store when an explicit server-ca was provided.

Optimally, I would agree and wanted a replacing instead of additive mechanism here.
This is what other OTEL SKDs are doing, but I also found other examples (e.g. Loggregator) where the behavior would be additive.
However, a best practice in general would be to rely on the system trust store whenever it is possible and only pin the ca when the system trust store does not include it. The server-ca in the binding is only a snapshot so it might be outdated eventually.
Since Cloud Logging bindings always return the server-ca field we would always pin it, which is why I don't want to leave out the system trust store.

(This is similar to the contribution in opentelemetry-exporter-for-sap-cloud-logging-for-nodejs)

We add server-ca rather than replace the system trust store with it because bindings today always ship server-ca. So a replacing it would never exercise the system trust store and would break on CA rotations.

Happy to get more feedback on this.
We could also expose a setting where users could configure additive or replacing behavior, but I intended to keep things simple for now, since this PR is already including quite some changes

Make server-ca optional in Cloud Logging credentials

25a2205

Signed-off-by: Jannik Brand <jannik.brand@sap.com>

This was referenced Sep 10, 2026

Clarify that server-ca is optional for mTLS ingest SAP-docs/btp-cloud-logging#61

Open

Make server-ca optional and use it as additional trust anchor SAP/opentelemetry-exporter-for-sap-cloud-logging-for-nodejs#47

Open

KarstenSchnitter requested changes Sep 10, 2026

View reviewed changes

KarstenSchnitter reviewed Sep 10, 2026

View reviewed changes

JannikBrand marked this pull request as draft September 10, 2026 08:36

Append server-ca to JVM default trust store instead of replacing it

afd87de

Signed-off-by: Jannik Brand <jannik.brand@sap.com>

KarstenSchnitter reviewed Sep 10, 2026

View reviewed changes

Refactor server-ca handling into TLS source abstraction

d264cbb

KarstenSchnitter requested changes Sep 10, 2026

View reviewed changes

KarstenSchnitter reviewed Sep 11, 2026

View reviewed changes

		credentials -> TrustedCertificatesJoiner.toPemBytes(new SystemTrustAnchorSource(),
		new BindingServerCertificateSource(credentials.getServerCert())));

Uh oh!

Conversation

JannikBrand commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Testing

Companion changes

Uh oh!

cla-assistant Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KarstenSchnitter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KarstenSchnitter commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KarstenSchnitter Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

KarstenSchnitter Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

JannikBrand commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JannikBrand commented Sep 10, 2026 •

edited

Loading

cla-assistant Bot commented Sep 10, 2026 •

edited

Loading

KarstenSchnitter left a comment •

edited

Loading

KarstenSchnitter commented Sep 10, 2026 •

edited

Loading

JannikBrand commented Sep 11, 2026 •

edited

Loading