Skip to content

Conversation

jvz
Copy link
Member

@jvz jvz commented Oct 10, 2025

As discovered in issue #3947, an NPE can be thrown when an SslConfiguration returns null for its keystore, truststore, or SSLContext, in particular, in SslSocketManager. Other fixes are noted by IntelliJ's nullability analysis on some files I checked that use SslConfiguration instances.

This fixes #3947.

Checklist

Before we can review and merge your changes, please go through the checklist below. If you're still working on some items, feel free to submit your pull request as a draft—our CI will help guide you through the remaining steps.

✅ Required checks

  • License: I confirm that my changes are submitted under the Apache License, Version 2.0.

  • Commit signatures: All commits are signed and verifiable. (See GitHub Docs on Commit Signature Verification).

  • Code formatting: The code is formatted according to the project’s style guide.

    How to check and fix formatting
    • To check formatting: ./mvnw spotless:check
    • To fix formatting: ./mvnw spotless:apply

    See the build instructions for details.

  • Build & Test: I verified that the project builds and all unit tests pass.

    How to build the project

    Run: ./mvnw verify

    See the build instructions for details.

🧪 Tests (select one)

  • I have added or updated tests to cover my changes.
  • No additional tests are needed for this change.

📝 Changelog (select one)

  • I added a changelog entry in src/changelog/.2.x.x. (See Changelog Entry File Guide).
  • This is a trivial change and does not require a changelog entry.

As discovered in issue #3947, an NPE can be thrown when an `SslConfiguration` returns `null` for its keystore, truststore, or `SSLContext`, in particular, in `SslSocketManager`.
@vy
Copy link
Member

vy commented Oct 11, 2025

@jvz, thanks so much for the prompt and clean work. Would you mind adding a test reproducing the issue reported in #3947, please?

Co-authored-by: Volkan Yazıcı <volkan@yazi.ci>
@jvz
Copy link
Member Author

jvz commented Oct 13, 2025

@jvz, thanks so much for the prompt and clean work. Would you mind adding a test reproducing the issue reported in #3947, please?

Would you prefer this in a unit test fashion or as a functional test? I can see the value in either. The unit tests would be useful around ensuring things work with null values, but a functional test could check that you can use a trust store without a key store for syslog appenders.

@vy
Copy link
Member

vy commented Oct 13, 2025

Would you prefer this in a unit test fashion or as a functional test? I can see the value in either. The unit tests would be useful around ensuring things work with null values, but a functional test could check that you can use a trust store without a key store for syslog appenders.

I'd appreciate a unit test reproducing the failure reported in #3947 – it doesn't necessarily need to be exactly the same, but should dial effectively same eventual knobs to trigger it. To aid with the implementation, feel free to mock stuff that is unrelated for the test subject. You can read this as, "Please no Docker ITs, etc. Just simple JUnit tests I can run using ./mvnw test -Dtest=FooTest." While reviewing, what I have in mind is to remove your fix, and confirm that the test fails in the same fashion as reported. I'm assuming test passes with your fix. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

Sending Syslog via TLS not possible with only encryted TLS Channel without CERT AUTH

2 participants