Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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.

Sign up for GitHub

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

Jump to bottom

Custom ssl engine parameters #639

Closed
wants to merge 2 commits into from
Closed

Custom ssl engine parameters #639

wants to merge 2 commits into from

Conversation

fmarco76
Copy link

Deploying tomcat with a custom ssl engine could require to perform some extra configuration before its usage. As an example, tomcatjss project needs to provide the actual IPs of client and server for audit purpose.
To provide the IPs we are working on a patch which require to copy several tomcat methods from NIO endpoint and channel because the current implementation does not allow to access the ssl engine before it is used (the method to be replaced are in this PR).

Changes in this PR allow to extend tomcat NIO classes and modify the ssl engine without reimplementing the existing methods.

@fmarco76
Make possible to create different channel in subclasses
ff08303 
Extending the Nio and Nio2 enpoints was not possible to associate
custom channels unless the method setSocketOption is not modified.
However, in some context could be required to use custom Nio?Channels
without changes in the endpoint.

A new method, createChannel(), is introduced to generate the channel. Extensions
of Nio endpoints have to reimplement only this method to associate a
different channel.
@ChristopherSchultz
Copy link
Contributor

This looks good to me, but I'd appreciate someone with more in-depth knowledge of the protocol handlers to weigh-in.

The only thing that caught my eye was that the signature of AbstractNetworkChannelEndpoint.createChannel has changed by widening the return value from NetworkChannel to Channel. This change is not backward compatible and we might want to introduce a new method instead of changing the signature of the current one.

This pull request is against main, so this change is okay, but back-porting to other stable releases I think is not okay for this reason.

@fmarco76
Copy link
Author

The only thing that caught my eye was that the signature of AbstractNetworkChannelEndpoint.createChannel has changed by widening the return value from NetworkChannel to Channel. This change is not backward compatible and we might want to introduce a new method instead of changing the signature of the current one.

Hi @ChristopherSchultz, the AbstractNetworkChannelEndpoint.createChannel is introduced by this PR so I am not sure this is widening the return value. Currently, the channel is created inside the method NioEndpoint.setSocketOptions and does not invokes other methods. I have introduced the createChannel so I can override it in a subclass and associate a different channel keeping the endpoint logic.

The only problem with back-porting I can think of it is that classes extending the AbstractNetworkChannelEndpoint will need to implement this method. To avoid this I can convert to a concrete method which throw an exception when invoked. In this way classes not using it will still work and the other have to override it.
Would this change make it good for back-porting or should I look at different approaches. Thanks!

@fmarco76
Make SSL engine modifieable by subclasses
b21f5da 
Extensions of the SecureNioChannel and SecureNio2Channel could require
to modify the sslengine before it get used for SNI and/or handshake.

The creation has been moved on a separate method which can be override.

This is useful if the ssl engine would require parameters from the
underlying connection for audit or debugging.
@jfclere jfclere changed the title Cusotm ssl engine parameters Custom ssl engine parameters Jul 19, 2023
@rmaucher
Copy link
Contributor

This proposed change looks good to me.

@rmaucher
Copy link
Contributor

I merged this change to Tomcat main branch (11). Backporting is not straightforward however as this is an API change stacked on top of a previous API change for Tomcat 11, so the code that uses it would be tied to 11 anyway.

@rmaucher rmaucher closed this Jul 20, 2023
@fmarco76
Copy link
Author

@rmaucher thanks for the merge. I have tested cherry-picking to tomcat 9 and it required a couple of minor fixes to work. Is there something I can do to facilitate the backport?

@rmaucher
Copy link
Contributor

@rmaucher thanks for the merge. I have tested cherry-picking to tomcat 9 and it required a couple of minor fixes to work. Is there something I can do to facilitate the backport?

This seems difficult, AbstractJsseEndpoint would be the spot (same for 10.1), but it would be an incompatible API change (so probably not ok), and code written for 11 would not be compatible anyway.

@fmarco76
Copy link
Author

but it would be an incompatible API change

Yes, the API would be slightly different between version 9 and 11.

@fmarco76 fmarco76 deleted the CusotmSSLEngine branch July 20, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fmarco76 @ChristopherSchultz @rmaucher