fix: cache items only when coming from trusted proxy by achmelo · Pull Request #4651 · zowe/api-layer

achmelo · 2026-05-28T13:29:00Z

Description

Caching service allows to store items under the arbitrary value in header if the request is authenticated with client certificate. This change makes the validation more strict and allow to items only when the request is coming from a trusted proxy. It also drops support for custom header and keeps only standard Client-Cert header.

Linked to # (issue)
Part of the # (epic)

Type of change

Please delete options that are not relevant.

fix: Bug fix (non-breaking change which fixes an issue)
feat: New feature (non-breaking change which adds functionality)
docs: Change in a documentation
refactor: Refactor the code
chore: Chore, repository cleanup, updates the dependencies.
BREAKING CHANGE or !: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the style guidelines of this project
PR title conforms to commit message guideline ## Commit Message Structure Guideline
I have commented my code, particularly in hard-to-understand areas. In JS I did provide JSDoc
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The java tests in the area I was working on leverage @nested annotations
Any dependent changes have been merged and published in downstream modules

For more details about how should the code look like read the Contributing guideline

Signed-off-by: ac892247 <a.chmelo@gmail.com>

…in config Signed-off-by: ac892247 <a.chmelo@gmail.com>

Signed-off-by: ac892247 <a.chmelo@gmail.com>

# Conflicts: # caching-service-package/src/main/resources/bin/start.sh # caching-service/src/test/java/org/zowe/apiml/caching/config/SecurityConfigTest.java

Signed-off-by: ac892247 <a.chmelo@gmail.com>

…lient certificate was provided Signed-off-by: ac892247 <a.chmelo@gmail.com>

Signed-off-by: ac892247 <a.chmelo@gmail.com>

pavel-jares-bcm · 2026-05-29T07:22:17Z

+                    log.debug("DEBUG: isForwardingEnabled = {}", certificateValidator.isForwardingEnabled());
+                    log.debug("DEBUG: hasGatewayChain = {}", certificateValidator.hasGatewayChain(certsFromTls));


These messge don't make sense here. They should be before the if (here it is clear what is the value) and also evaluate the same code is not needed.

pavel-jares-bcm · 2026-05-29T07:27:06Z

+                    );

-                logIgnoredCertificates(new X509Certificate[]{clientCertFromHeader.get()}, clientAuthCerts);
+                    log.debug("DEBUG: clientAuthCerts.length = {}", clientAuthCerts.length);


I would say for debugging an issue it would be helpfull to write some information about certificates. Not necessary the whole key, but serial number for example.

pavel-jares-bcm · 2026-05-29T07:31:58Z

            caching-service:
                image: ghcr.io/balhar-jakub/caching-service:${{ github.run_id }}-${{ github.run_number }}
+                env:
+                    APIML_SECURITY_X509_ACCEPTFORWARDEDCERT: true


This value is in application.yaml. It is not needed to set again. Btw. I guess the URL of cert should be detected by CS automatically.

pavel-jares-bcm · 2026-05-29T08:25:13Z

    private static final String VALID_BASIC_AUTH = "Basic " + Base64.getEncoder().encodeToString((USER + ":" + PASSWORD).getBytes());
    private static final String INVALID_BASIC_AUTH = "Basic " + Base64.getEncoder().encodeToString((USER + ":invalidPassword").getBytes());
-    private static final String X_CS_SERVICE_ID = "X-CS-Service-ID";
+    private static final String X_CS_SERVICE_ID = "Client-Cert";


field X_CS_SERVICE_ID is unclear

pavel-jares-bcm · 2026-05-29T08:26:58Z

  -Dapiml.service.ssl.trust-store="${client_truststore_location}" \
  -Dapiml.service.ssl.trust-store-password="${client_truststore_pass}" \
  -Dapiml.service.ssl.trust-store-type="${client_truststore_type}" \
+  -Dapiml.security.x509.acceptForwardedCert=${ZWE_configs_apiml_security_x509_enabled:-${ZWE_components_gateway_apiml_security_x509_enabled:-true}} \


Regarding these changes does it make sense have certificate forwarding configurable

Signed-off-by: ac892247 <a.chmelo@gmail.com>

sonarqubecloud · 2026-06-02T06:52:19Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

pavel-jares-bcm

Please take a look also on Sonar issues.

pavel-jares-bcm · 2026-06-02T06:47:28Z

        hostname: caching-service
    banner: console
-
+    security:


All configuration should be updated, including local config

pavel-jares-bcm · 2026-06-02T06:49:17Z

+            VPx2
+            """.replaceAll("\\s+", "");
+
+        @org.junit.jupiter.api.BeforeEach


Suggested change

@org.junit.jupiter.api.BeforeEach

@BeforeEach

import org.junit.jupiter.api.BeforeEach

pavel-jares-bcm · 2026-06-02T06:51:36Z

    enabled: true
+    security:
+            x509:
+                certificatesUrl: https://localhost:10010/gateway/certificates


Shouldn't we prefer the full URL (https://localhost:10010/gateway/api/v1/certificates). Both of them are supported, but I would say the i'm should be use the one with API version.

this is only for local run, in start.sh it uses the same URL as zaas service

pavel-jares-bcm · 2026-06-02T06:59:38Z

+                certificateValidator.hasGatewayChain(certsFromTls)) {
+                Optional<X509Certificate> clientCertFromHeader = getClientCertFromHeader(exchange.getRequest());
+                log.debug("clientCertFromHeader.isPresent = {}", clientCertFromHeader.isPresent());
+                if (clientCertFromHeader.isPresent()) {


The logic was changed, when the request is signed by Zowe certificate and there is no client cert header certificates are not logged and no attibute is set. I would say, the original implementation is better. We should process that in the else statement. It has probably no impact to caching service, but the behaviour could be different for internal API. Basically, I guess it just could make debugging of an issue more complicated.

the only thing that changed is a log message, what exactly is wrong about it?

now I got, it's moved back to original condition

pavel-jares-bcm · 2026-06-02T07:02:22Z

            void whenNoBasicAuth_thenReturnForbidden() {
                given()
-                    .header(new Header(X_CS_SERVICE_ID, "apimtst"))
+                    .header(new Header(CLIENT_CERT_HEADER_NAME, MOCK_FORWARDED_CERT))


Do we have somewhere test with clientCert and service prefix?

InMemoryFunctionalTest

Signed-off-by: ac892247 <a.chmelo@gmail.com>

achmelo added 21 commits May 27, 2026 08:59

reuse secure certificate forwarding in caching service

4274393

Signed-off-by: ac892247 <a.chmelo@gmail.com>

get cert from header only if forward is enabled

ff9bec0

Signed-off-by: ac892247 <a.chmelo@gmail.com>

add messages from security common, do not set fallback property in ma…

7652720

…in config Signed-off-by: ac892247 <a.chmelo@gmail.com>

enable cert forward for infinispan tests

1b22d4a

Signed-off-by: ac892247 <a.chmelo@gmail.com>

enable client cert forwarding

63e7a75

Signed-off-by: ac892247 <a.chmelo@gmail.com>

enable client cert forwarding and use sslinfo only for modulith

6584f3f

Signed-off-by: ac892247 <a.chmelo@gmail.com>

fix syntax

4d90f2c

Signed-off-by: ac892247 <a.chmelo@gmail.com>

enable forwarding by default, print whether service enables forwarding

a24b801

Signed-off-by: ac892247 <a.chmelo@gmail.com>

accept all certs for client auth in caching service

22b281b

Signed-off-by: ac892247 <a.chmelo@gmail.com>

disable test that is probably not valid anymore

7faf3b6

Signed-off-by: ac892247 <a.chmelo@gmail.com>

move cert URL to common script

c104d0b

Signed-off-by: ac892247 <a.chmelo@gmail.com>

Merge remote-tracking branch 'origin/v3.x.x' into reboot/cache-key

19a9d3f

# Conflicts: # caching-service-package/src/main/resources/bin/start.sh # caching-service/src/test/java/org/zowe/apiml/caching/config/SecurityConfigTest.java

try to skip else branch for caching service

a1adf2c

Signed-off-by: ac892247 <a.chmelo@gmail.com>

set cert url, log failed test

102a617

Signed-off-by: ac892247 <a.chmelo@gmail.com>

log request, do not send dn header

056c0c3

Signed-off-by: ac892247 <a.chmelo@gmail.com>

remove test for disabled ssl validation, store cache item only when c…

ed02ff7

…lient certificate was provided Signed-off-by: ac892247 <a.chmelo@gmail.com>

format

93061dd

Signed-off-by: ac892247 <a.chmelo@gmail.com>

send certificate in the header

0986f7c

Signed-off-by: ac892247 <a.chmelo@gmail.com>

send header in every request

8d40777

Signed-off-by: ac892247 <a.chmelo@gmail.com>

improve doc

b4ea3eb

Signed-off-by: ac892247 <a.chmelo@gmail.com>

return test and send proper header

7b72b69

Signed-off-by: ac892247 <a.chmelo@gmail.com>

github-project-automation Bot added this to API Mediation Layer Backlog Management May 28, 2026

github-project-automation Bot moved this to New in API Mediation Layer Backlog Management May 28, 2026

pull-request-size Bot added the size/XL label May 28, 2026

github-actions Bot added the Sensitive Sensitive change that requires peer review label May 28, 2026

achmelo and others added 5 commits May 28, 2026 15:36

fix styles

2e61016

Signed-off-by: ac892247 <a.chmelo@gmail.com>

store encoded cert only when exists

6e756e7

Signed-off-by: ac892247 <a.chmelo@gmail.com>

Merge remote-tracking branch 'origin/v3.x.x' into reboot/cache-key

606dbb3

Merge branch 'v3.x.x' into reboot/cache-key

a1d24a1

remove unnecessary stubbing

019db5a

Signed-off-by: ac892247 <a.chmelo@gmail.com>

pavel-jares-bcm reviewed May 29, 2026

View reviewed changes

achmelo added 12 commits June 1, 2026 10:32

Merge remote-tracking branch 'origin/v3.x.x' into reboot/cache-key

0bb79a5

move certificate url to common config

b5ffeca

Signed-off-by: ac892247 <a.chmelo@gmail.com>

use client cert from sslinfo as key

a4e6c81

Signed-off-by: ac892247 <a.chmelo@gmail.com>

cert url for redis setup

1a5dac9

Signed-off-by: ac892247 <a.chmelo@gmail.com>

remove the option to disable forwarding from start.sh

48a54a7

Signed-off-by: ac892247 <a.chmelo@gmail.com>

Merge remote-tracking branch 'origin/v3.x.x' into reboot/cache-key

368edab

remove flag from constructor

51d9a27

Signed-off-by: ac892247 <a.chmelo@gmail.com>

cleanup

2b44eab

Signed-off-by: ac892247 <a.chmelo@gmail.com>

do not use autowired

8062a1d

Signed-off-by: ac892247 <a.chmelo@gmail.com>

Merge remote-tracking branch 'origin/v3.x.x' into reboot/cache-key

0baf429

log certificate's serial number

3172996

Signed-off-by: ac892247 <a.chmelo@gmail.com>

cleanup

cb18796

Signed-off-by: ac892247 <a.chmelo@gmail.com>

pavel-jares-bcm reviewed Jun 2, 2026

View reviewed changes

achmelo and others added 4 commits June 2, 2026 09:13

code review

4534e17

Signed-off-by: ac892247 <a.chmelo@gmail.com>

Merge remote-tracking branch 'origin/v3.x.x' into reboot/cache-key

7b819b9

code review

207e586

Signed-off-by: ac892247 <a.chmelo@gmail.com>

Merge branch 'v3.x.x' into reboot/cache-key

f3bdf4c

		log.debug("DEBUG: isForwardingEnabled = {}", certificateValidator.isForwardingEnabled());
		log.debug("DEBUG: hasGatewayChain = {}", certificateValidator.hasGatewayChain(certsFromTls));

Conversation

achmelo commented May 28, 2026

Description

Type of change

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jun 2, 2026

Quality Gate passed

Uh oh!

pavel-jares-bcm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants