Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce app-wide property for quarkus.cxf.tls-configuration-name #1680

Open
dcheng1248 opened this issue Jan 13, 2025 · 6 comments
Open

Introduce app-wide property for quarkus.cxf.tls-configuration-name #1680

dcheng1248 opened this issue Jan 13, 2025 · 6 comments
Assignees

Comments

@dcheng1248
Copy link
Contributor

Currently, to set the TLS truststore for a client, the quarkus property quarkus.cxf.client."client-name".tls-configuration-name must be set. In our use case, we have all the certificates gathered into one truststore but have several clients, and it can feel cumbersome to have to set the property for every client. It would help avoid repeated code if an app-wide property is introduced.

I would be happy to see if I can help implement this if you think it is worth doing but not high priority.

@dcheng1248
Copy link
Contributor Author

@ppalaga would this be something to consider? I have holidays starting from next week, so I'd be happy to work towards it during that time, if you are in agreement.

@ppalaga
Copy link
Contributor

ppalaga commented Jan 21, 2025

Thanks for for the reminder, @dcheng1248 and apologizes for not reacting promptly.

I am still pondering whether we should use the "nameless" default Quarkus trust store defined by properties quarkus.tls.key-store.*, quarkus.tls.trust-store.*. etc. as a default trust store for CXF clients.

That's how they do it in some Quarkus extensions

GRPC client

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/runtime/config/GrpcClientConfiguration.java#L69-L82

OpenTelemetry

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/config/runtime/exporter/OtlpExporterConfig.java#L100-L112

REST Client

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java#L258-L268

But on the other hand, there are extensions not taking quarkus.tls.* as a default:

Keycloak admin

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/keycloak-admin-client-common/runtime/src/main/java/io/quarkus/keycloak/admin/client/common/KeycloakAdminClientConfig.java#L76-L83

Mailer

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MailerRuntimeConfig.java#L73-L84

OIDC Client

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/config/OidcCommonConfig.java#L94-L102

Redis Client

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/redis-client/runtime/src/main/java/io/quarkus/redis/runtime/client/config/RedisClientConfig.java#L223-L234

WebSocket client

https://github.com/quarkusio/quarkus/blob/5534ed16ac3140034f938e2a21a9d753958e6bc8/extensions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/config/WebSocketsClientRuntimeConfig.java#L55-L63

There seem to be all kinds of flavors to handle the defaults.

I think in our case, a CXF specific client-only property (let's call it quarkus.cxf.client.tls-configuration-name) would be useful, so that following use cases are possible:

(We need tests for all the above scenarios.)

We have to decide what would the default of quarkus.cxf.client.tls-configuration-name. There are three options:

  • default pointing to quarkus.tls.*
  • java.net.ssl
  • none

Any opinion on that?

We have to implement and document some clean precedence rules for various levels of named and default TLS configs as well as for the old (and now deprecated) quarkus.cxf.client."client-name".key-store* and quarkus.cxf.client."client-name".trust-store family of options.

I can gladly assign this to you if you like @dcheng1248 .

@dcheng1248
Copy link
Contributor Author

@ppalaga many thanks for the detailed response and thoughts you put into this! I'm still new to large codebases so your examples provide great help for me to orient myself and think about the issue and implementations.

To your question, here are my thoughts:

  • default pointing to quarkus.tls.*:
    This would introduce an additional layer of dependency on the TLS registry, although making it align with GRPC and REST could be beneficial. But would this lead to potential (security) issues? I don't understand this completely but remember seeing this in the Zulip chat between you and Clement.

  • java.net.ssl:
    I believe this provides the most drop-in compatibility with URLConnectionHTTPConduit, as it doesn't require any TLS to be set and works out of the box. However, if quarkus.tls.* is set by some users, it could be confusing that the CXF default points to somewhere else entirely. Additionally, it seems that none of the existing quarkus extensions use this by default, so it would be a large deviation from existing practices.

  • none
    I personally don't see much benefit to this option. Since the quarkus.cxf.client."client-name".trust-store etc. are deprecated, a named TLS configuration needs to be set anyway, and having some sort of default is potentially less work/repeated code for users than having none?

I'm tempted towards the second option just for the compatibility, but think the first could also be a good choice. Do you think it would be practical to throw an error (that asks the user to set the tls configurations) if the user sets a client-endpoint-url to https but do not have a tls configured? This would prevent going down the rabbit hole of weird server responses (like I did) due to the lack of TLS. If this is possible, I would be inclined to say the first option along with the error warning.

For the implementation, I would love to contribute. How urgent do you think this issue is? I will have a lot more time starting next week, as I will be on holiday from work. This is, however, an admittedly non-trivial implementation for me (due to unfamiliarity with the codebase, needing to think about the precedence rules and my general inexperience) so it might take me a bit longer. If you prefer a faster implementation, I would love to help however I can.

@ppalaga
Copy link
Contributor

ppalaga commented Jan 21, 2025

I think quarkus.tls.* sounds like the best fit for storing the key for the HTTP server whereas java.net.ssl would generally make the best out of the box experience for clients that do not need mTLS. Compatibility with URLConnectionHTTPConduit is another strong argument for taking java.net.ssl as a default for quarkus.cxf.client.tls-configuration-name. So let's do it in that way.

Do you think it would be practical to throw an error (that asks the user to set the tls configurations) if the user sets a client-endpoint-url to https but do not have a tls configured?

Would we need it if we make java.net.ssl the default?

For the implementation, I would love to contribute. How urgent do you think this issue is?

Not urgent from my PoV, but folks following #1665 might be happy if it could reach
3.19.0 that we'll release around Feb. 19th .

Let me assign it to you for now. Feel free to let me know if you stop looking at it.

@dcheng1248
Copy link
Contributor Author

Would we need it if we make java.net.ssl the default?

Probably not. But if java.net.ssl is the default, should we still introduce a reserved configuration name for the quarkus.tls.* properties (like "quarkus-tls") to make it simple if users want to use that instead, or would this cause more confusion?

Not urgent from my PoV, but folks following #1665 might be happy if it could reach
3.19.0 that we'll release around Feb. 19th .

Great! I will do my best and be in touch about any problems. Thank you!

@ppalaga
Copy link
Contributor

ppalaga commented Jan 23, 2025

Would we need it if we make java.net.ssl the default?

Probably not. But if java.net.ssl is the default, should we still introduce a reserved configuration name for the quarkus.tls.* properties (like "quarkus-tls") to make it simple if users want to use that instead

We do not need to introduce that name, because Quarkus defines it already:

https://github.com/quarkusio/quarkus/blob/f2c37e65a0f2a6eff59716b0ea73a475287cf879/extensions/tls-registry/runtime/src/main/java/io/quarkus/tls/runtime/config/TlsConfig.java#L16

https://github.com/quarkusio/quarkus/blob/f2c37e65a0f2a6eff59716b0ea73a475287cf879/extensions/tls-registry/runtime/src/main/java/io/quarkus/tls/runtime/CertificateRecorder.java#L156-L158

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants