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

Add wolfjsse.autoSNI Security property #249

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

gasbytes
Copy link
Contributor

@gasbytes gasbytes commented Jan 7, 2025

Introduce wolfjsse.autoSNI property to make SNI behavior configurable. When
set to true, enables legacy automatic SNI based on hostname/peer. By default,
SNI is only set through explicit SSLParameters configuration.

  • Fixes SSLSocketExplorerWithSrvSNI test failures
  • Maintains compatibility for existing applications
  • Adds test coverage in WolfSSLSocketTest

@gasbytes gasbytes requested a review from cconlon January 7, 2025 16:55
@gasbytes gasbytes marked this pull request as draft January 7, 2025 17:16
@gasbytes gasbytes marked this pull request as ready for review January 7, 2025 21:19
@cconlon
Copy link
Member

cconlon commented Jan 10, 2025

I'm a little hesitant about this change, since I want to say we have had customers with failing connections until this behavior was added. Are we sure that SunJSSE only sets the SNI extension in the ClientHello when set in the SSLParameters, and doesn't pick it up if the SSLSocket is created using certain inputs?

Also, did we decide that this did indeed fix the SSLSocketExplorerWithSrvSNI test, or was that one that was broken by something else?

Thanks,
Chris

@cconlon cconlon assigned gasbytes and unassigned wolfSSL-Bot Jan 10, 2025
@gasbytes
Copy link
Contributor Author

Hello Chris,

I've been thinking about your concerns regarding the SNI behavior change. You raise a valid point about potential customer issues. To address this while still fixing the test failure, I propose adding a new system property com.wolfssl.provider.jsse.autoSNI that would let customers opt into the legacy automatic SNI behavior if needed.

By default, we would only set SNI when explicitly configured through SSLParameters - fixing the test case and better aligning with standard JSSE behavior.
This way, we can maintain compatibility for customers who depend on the automatic SNI while also follow the JSSE behavior.

Would this approach address your concerns?

Thanks,
Reda Chouk

@cconlon
Copy link
Member

cconlon commented Jan 31, 2025

@gasbytes Yes, that sounds like a great plan. I suggest adding it as a Security property (instead of System) for consistency with our other wolfJSSE configuration properties. I also suggest following existing Security property naming schemes and call it wolfjsse.autoSNI, which would be set from java.security like so:

wolfjsse.autoSNI=true

Please also update the README.md section on System Property Support once added, and add in a JUnit test that tests setting the property (Security.setProperty("wolfjsse.autoSNI", "true") if possible.

Introduce wolfjsse.autoSNI property to make SNI behavior configurable. When
set to true, enables legacy automatic SNI based on hostname/peer. By default,
SNI is only set through explicit SSLParameters configuration.

- Fixes SSLSocketExplorerWithSrvSNI test failures
- Maintains compatibility for existing applications
- Adds test coverage in WolfSSLSocketTest
@gasbytes gasbytes force-pushed the SSLSocketExplorerWithSrvSNI-patch branch from a164fd9 to 9657eee Compare January 31, 2025 23:28
@gasbytes gasbytes changed the title Remove automatic SNI extension fallback in WolfSSLEngineHelper Add wolfjsse.autoSNI Security property Jan 31, 2025
@gasbytes gasbytes assigned cconlon and unassigned gasbytes Jan 31, 2025
@cconlon cconlon merged commit 1e530a9 into wolfSSL:master Jan 31, 2025
47 checks passed
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

Successfully merging this pull request may close these issues.

3 participants