S2S-3829 Enable Spire in Rest-Util #574

carlhejiayu · 2025-05-20T17:55:45Z

Description
Enable Server using Spire to do S2S authentication.

Detailed Changes

Introduce new settings ssl.spire.enabled, by default it's false and everything function exactly the same as before; it will only try to do s2s through spire if this is turned on.
Modify application and server constructor to take in X509Source. This argument is optional for non-spire setting. If it's configured to use spire, this argument became Mandatory.
HelloWorldSpireApplication is the sample application with spire setting. We can take this as an example for all library users to follow.

Testing
Most of the testing are done through unit test where we mock X509Source to provide certain bundle and test against the server behaviour with its certain configuration. All the tests are written in SpireTest.java

Server Onboarding Spire Doc
Please look at the following doc to see how existing rest-util server can make config changes to onboard spire
https://confluentinc.atlassian.net/wiki/spaces/SECENG/pages/4470801894/Rest-Util+Server+onboarding+to+Spire

DEMO Video
Here is a demo video on how we can turn on certain setting and make our application doing spire S2S in our local dev machine
https://drive.google.com/file/d/1BXudIXuEIy0ck1QckdoapAMHcox9jU2D/view?usp=sharing

TODO: we can setup the container testing strategy where can spin up spire server/agent in the CI do the testing. But this is also not super trival, wont' be included in the scope of this PR.

…tation

core/pom.xml

…ing closed

…structor building

carlhejiayu · 2025-05-27T17:09:46Z

core/src/main/java/io/confluent/rest/SslFactory.java

+    SpiffeSslContextFactory.SslContextOptions options = SpiffeSslContextFactory.SslContextOptions
+        .builder()
+        .x509Source(x509Source)
+        .acceptAnySpiffeId()


Right now, I configured to acceptAnySpiffeId . The authorization design and implementation is not part of this PR scope.
We should have new design session on how we can reject/allow certain spiffee ID to hit certain endpoint.

Can we not configure a regex pattern here

If you look at the native library here, they don't natively support regex. If we want to have more complex pattern, we have to define one more layer abstraction to do this functionality, which is why it won't be this PR scope.

carlhejiayu · 2025-05-27T18:03:08Z

core/src/main/java/io/confluent/rest/ApplicationServer.java

@@ -116,8 +124,18 @@ public ApplicationServer(T config, ThreadPool threadPool) {

    listeners = config.getListeners();

+    if (x509Source == null) {
+      if (config.getBoolean(RestConfig.SSL_IS_SPIRE_ENABLED_CONFIG)) {
+        throw new RuntimeException("X509Source must be provided when SPIRE SSL is enabled");


We want to allow callers to pass in X509Source. The main reason is that we want to allow multiple application in the same server to be able to share the same X509Source to reduce the connection to spire agents. This is very particular important for performance reasons.

Here, I choose to throw error when we see X509Source is NULL. There are some reasons why I make X509Source mandatory instead of to initializing one if it is NULL.

initializing X509Source require lots of custom params, like socketPath , initTimeOut. We should not put these setting in the application config. Instead, responsibility of how to create it should be controlled by caller.

It is very difficult to determine the lifecycle of this object from the server standpoint. If the server decided to
initialize on behalf of caller, then it should have logic to close the connection when the server is down; However, the server need to differentiate the case when X509Source is passed v.s created.

Overall, we can see helping client to initializing X509Source is violation of SRP (single responsibility principle).

To conclude, we have let caller to pass in X509Source if they want to use s2s-spire because the caller have the absolute control on how it got created and how long it should last. [These responsibilities do not belong to server]

carlhejiayu · 2025-05-27T22:58:59Z

core/src/main/java/io/confluent/rest/SslFactory.java

-      setSecurityStoreProps(sslConfig, sslContextFactory, true, false);
-      sslContextFactory.setKeyManagerPassword(sslConfig.getKeyManagerPassword());
-
-      if (!sslConfig.getKeyManagerFactoryAlgorithm().isEmpty()) {
-        sslContextFactory.setKeyManagerFactoryAlgorithm(sslConfig.getKeyManagerFactoryAlgorithm());
-      }
-
-      if (sslConfig.getReloadOnKeyStoreChange()) {
-        Path watchLocation = Paths.get(sslConfig.getReloadOnKeyStoreChangePath());
-        try {
-          FileWatcher.onFileChange(watchLocation,
-              onFileChangeCallback(sslConfig, sslContextFactory));
-          log.info("Enabled SSL cert auto reload for: " + watchLocation);
-        } catch (java.io.IOException e) {
-          log.error("Cannot enable SSL cert auto reload", e);
-        }
-      }


Refactor this part because it failed the style check; if no refactor done, it will complain with this error
NPathComplexity

Just note: this refactor has not made any no functional changes ; it just just refactor into separate function to make the style check passing

moe-omar · 2025-05-29T19:14:41Z

core/src/main/java/io/confluent/rest/ApplicationServer.java

  }

-  public ApplicationServer(T config, ThreadPool threadPool) {
+  public ApplicationServer(T config, ThreadPool threadPool, X509Source x509Source) {


Instead of changing this constructor we can add a new constructor which calls your new one with null for the x509Source, that way you don't introduce a breaking change

public ApplicationServer(T config, ThreadPool threadPool) { this(config, threadPool, null); }

If a service needs S2S then they call

public ApplicationServer(T config, ThreadPool threadPool, X509Source x509Source)

everything else calls or continues to use the same existing constructor

Yes added more constructor for callers to be more flexible

silvanli-confluent · 2025-06-05T18:04:10Z

core/src/main/java/io/confluent/rest/ApplicationServer.java

+    this(config, createThreadPool(config), x509Source);
+  }
+
+  public ApplicationServer(T config,ThreadPool threadPool) {


nit: whitespace

thx. I made this commit to fix the white space

akhileshm1 · 2025-06-09T16:10:40Z

@carlhejiayu, is it possible to make this PR against 7.9.x? we can pint-merge the changes to master. the reason being SR is currently on 7.9.x in CC.

carlhejiayu · 2025-06-09T20:40:00Z

@carlhejiayu, is it possible to make this PR against 7.9.x? we can pint-merge the changes to master. the reason being SR is currently on 7.9.x in CC.

Yes @akhileshm1 , i created a brand new PR that is based in 7.9.X. see here: #583

dragonmushu

Thanks added some comments and also is there a way we can log mTLS failures

dragonmushu · 2025-06-11T15:19:41Z

core/src/main/java/io/confluent/rest/RestConfig.java

@@ -206,6 +206,11 @@ public class RestConfig extends AbstractConfig {
      + " runtime request tags in global stats.";
  protected static final boolean METRICS_GLOBAL_STATS_REQUEST_TAGS_ENABLE_DEFAULT = false;

+  public static final String SSL_IS_SPIRE_ENABLED_CONFIG = "ssl.spire.enabled";


Would named listeners be supported here. Basically we need to also be able to open up multiple ssl ports within same application. One for spire and another maybe that is just tls.

If you look at rest-util ssl config, i think configuration applied to all the ports that doing SSL. This is by the design, and i don't think they have option to support custom SSL setting per port; to do that, that seemed much bigger refactor also i think it's might be slight anti-pattern.

@dragonmushu , I would suggest that you initialize 2 application instance . For example, the first instance could be configured like this SSL.spire.enabled=true & port = 8080, and the second application could be configured like this SSL.spire.enabled=false& port = 9090 . i believe this should achieve this same purpose

This is by the design, and i don't think they have option to support custom SSL setting per port; to do that, that seemed much bigger refactor also i think it's might be slight anti-pattern.

This is not true. Currently we have named listeners that allows us to set to different tls settings for each listener in rest-utils. We shouldn't break that functionality.

Hi @dragonmushu , can you put up the github link of what you have already done so that i can see? I may not be familiar with it

https://github.com/confluentinc/cc-security-decisions/pull/1402/files

Hey @dragonmushu , in the example link you provided, it looks you have 2 port , 1 is TLS, 1 is unsecured. I am not seeing that you set 2 ports for both TLS

Thats my first PR. In the second PR I also added another listener for mtls. https://github.com/confluentinc/cc-security-decisions/pull/1442/files

dragonmushu · 2025-06-11T15:26:33Z

core/src/main/java/io/confluent/rest/SslFactory.java

-          log.error("Cannot enable SSL cert auto reload", e);
-        }
-      }
+      configureKeyStore(sslContextFactory, sslConfig);


Both shouldn't be configured right?

if we have spire enabled, you are right that they won't be needed to be configured; However, even configuring won't do much anything because internally the server will go to talk spire agent to grab all the trust bundle and stuff into cache; it will never have a need to reach the KeyStore even Keystore was configured. I already provided the comment here

dragonmushu · 2025-06-11T15:33:54Z

core/src/main/java/io/confluent/rest/SslFactory.java

+    SpiffeSslContextFactory.SslContextOptions options = SpiffeSslContextFactory.SslContextOptions
+        .builder()
+        .x509Source(x509Source)
+        .acceptAnySpiffeId()


Can we not configure a regex pattern here

dragonmushu

@carlhejiayu , @moe-omar do we not want to interface with the SRJ standalone module here: https://confluentinc.atlassian.net/wiki/x/HAId6w instead of the raw java library

Instead of passing in an X509 source can we pass in the SpireX509Client.

dragonmushu · 2025-06-12T18:58:17Z

Also @carlhejiayu one thing I called out in the requirements was observability around the

@carlhejiayu , @moe-omar do we not want to interface with the SRJ standalone module here: https://confluentinc.atlassian.net/wiki/x/HAId6w instead of the raw java library

Instead of passing in an X509 source can we pass in the SpireX509Client.

Sorry for these late reviews. It would be good to unify these two libraries because clients are recommended to use the SRJ library to configure client SDKs and so if we can somehow just wrap the SRJ module here for the server side it would be nice. Also we would only try to have one connection to the agent per service.

carlhejiayu · 2025-06-17T01:13:10Z

@carlhejiayu , @moe-omar do we not want to interface with the SRJ standalone module here: https://confluentinc.atlassian.net/wiki/x/HAId6w instead of the raw java library

Instead of passing in an X509 source can we pass in the SpireX509Client.

Hi @dragonmushu , I would not recommend do this for the following the 2 reason:

SpireX509Client is an SRJ concept (SRJ is confluent internal repo), we should not import confluent dependency into this public repo rest-util; in fact, it is not allowed.
I believe SRJ is on process of using rest-util to do their implementation. I believe that X509Source should be the way to go unless you have a strong reason we should keep SpireX509Client

carlhejiayu · 2025-06-17T01:13:39Z

Also @carlhejiayu one thing I called out in the requirements was observability around the

@carlhejiayu , @moe-omar do we not want to interface with the SRJ standalone module here: https://confluentinc.atlassian.net/wiki/x/HAId6w instead of the raw java library
Instead of passing in an X509 source can we pass in the SpireX509Client.

Sorry for these late reviews. It would be good to unify these two libraries because clients are recommended to use the SRJ library to configure client SDKs and so if we can somehow just wrap the SRJ module here for the server side it would be nice. Also we would only try to have one connection to the agent per service.

I believe SRJ would start to rely on rest-util

carlhejiayu added 4 commits May 14, 2025 11:37

modify server to make it connecting to spire agent

f183a7f

dependency pom xml

bc0364e

make HelloWorldApplication using SSL

e422164

add spire client test to do MTLS

5572864

carlhejiayu changed the title ~~Carl/s2 s 3829~~ S2S-3829 Enable Spire in Rest-Util May 20, 2025

carlhejiayu added 10 commits May 20, 2025 17:41

remove SpiffeTrustManager

659666b

sslFactory utilize SpiffeSslContextFactory instead of custom implemen…

fe397ea

…tation

mdofiy applicationserver to to take X509 source outside

9e43a3e

modify application to take or initialize X509

152111b

modify HelloWorld Application to pass X509Source when init application

478a430

modify spireTest to use spiffeFactoryContext

2024708

add config for spire related setting

228ef7a

refactor x509source initilization to sslFactory

1a431e7

add spire setting for helloWorldApplication

d3a02fd

put x509 passing into the server

9f31513

carlhejiayu commented May 21, 2025

View reviewed changes

core/pom.xml Show resolved Hide resolved

carlhejiayu added 14 commits May 22, 2025 15:50

move X509source object to be in application server to prevent it gett…

6c8f783

…ing closed

add 2 ways of application starting

0a3e187

add null check

606a70f

style fixing

8994624

style check for sslfactory

d7770ce

implement mock class in spire Test

4442176

nit changes

3d69a5d

fix TestCustomizeThreadPool unit testing failure due to incorrect con…

0e9e5cb

…structor building

add option to be MTLS v.s TLS

54af447

add more tests cases for spire Test

d1d1033

revert all changes made to HelloWorldApplication

d32474a

create HelloWorldApplication with s2s based on spire

f3538cc

Spire Test Refactor 1

141f8df

Spire Test Refactor 2

dd160ff

carlhejiayu added 3 commits May 27, 2025 12:14

remove socketPath variable from config

9c426ed

remove socket path

d9e04ea

fix HelloWorldSpireApplication style issue

3e302a7

carlhejiayu commented May 27, 2025

View reviewed changes

carlhejiayu added 3 commits May 27, 2025 20:42

revert extra variable for MTLS

ed5a9a0

make all the test to use SSL_CLIENT_AUTHENTICATION_CONFIG instead

8dfdb65

add more comments for the sample HelloWorldSpireApplication

d579989

carlhejiayu marked this pull request as ready for review May 28, 2025 20:16

carlhejiayu requested review from a team as code owners May 28, 2025 20:16

style fix

acd87b8

trnguyencflt added the knet label May 29, 2025

moe-omar reviewed May 29, 2025

View reviewed changes

carlhejiayu added 2 commits June 3, 2025 11:57

add new consturctor to application server

b8f41f6

add more constructor

e1d461e

silvanli-confluent reviewed Jun 5, 2025

View reviewed changes

white space nit

6e48e15

dragonmushu reviewed Jun 11, 2025

View reviewed changes

dragonmushu reviewed Jun 12, 2025

View reviewed changes

carlhejiayu mentioned this pull request Jun 16, 2025

S2S-3829 Enable Spire in Rest-Util -base 7.9.x #583

Merged

tyffani assigned carlhejiayu Jun 16, 2025

carlhejiayu closed this Jun 19, 2025

S2S-3829 Enable Spire in Rest-Util #574

S2S-3829 Enable Spire in Rest-Util #574

Uh oh!

Conversation

carlhejiayu commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

carlhejiayu May 27, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlhejiayu May 27, 2025 • 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!

carlhejiayu May 27, 2025 • 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!

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!

akhileshm1 commented Jun 9, 2025

Uh oh!

carlhejiayu commented Jun 9, 2025

Uh oh!

dragonmushu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlhejiayu Jun 11, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

carlhejiayu Jun 12, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

carlhejiayu Jun 17, 2025 • 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!

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!

dragonmushu left a comment

Choose a reason for hiding this comment

Uh oh!

dragonmushu commented Jun 12, 2025

Uh oh!

carlhejiayu commented Jun 17, 2025

Uh oh!

carlhejiayu commented Jun 17, 2025

Uh oh!

Uh oh!

carlhejiayu commented May 20, 2025 •

edited

Loading

carlhejiayu May 27, 2025 •

edited

Loading

carlhejiayu May 27, 2025 •

edited

Loading

carlhejiayu May 27, 2025 •

edited

Loading

carlhejiayu Jun 11, 2025 •

edited

Loading

carlhejiayu Jun 12, 2025 •

edited

Loading

carlhejiayu Jun 17, 2025 •

edited

Loading