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

Push IPs address to SSLEngine session #73

Closed
wants to merge 1 commit into from

Conversation

fmarco76
Copy link
Member

@fmarco76 fmarco76 commented Jun 23, 2023

SSLEngine is by design unaware of the underlying communication channel. In tomcat the communication channel is started by the classes NioEndpoint and it is maintained in SecureNioChannel which will create the buffer used with the SSLEngine in order to wrap and unwrap the messages.

To allow the audit of TLS messages to include IP addresses of the client and server, the above classed have been extended in order to store the IPs in the SSLEngine session after its creation.

This require the JSS PR 972

@fmarco76
Copy link
Member Author

The build fails because require the modified JSS.

@@ -0,0 +1,77 @@
package org.dogtagpki.tomcat;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods from this and the following class come from tomcat. Should we put a note to indicate the origin?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the copyright/licensing issue, but is it possible to make some changes in Tomcat so that the code that we need to copy is minimized? For now we just need to make the changes in downstream Tomcat, but later we can try to merge it upstream too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of copyright/licensing issue, I'd suggest to document the origin of the code if it resembles an existing code somewhere else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmarco76 I'm not familiar with Tomcat enough to make any assessment, but in general, you are right to be concerned about copying the code. I'm with @edewata there. Is there a way to talk to the Tomcat team to make an adjustment so that the class will become extensible in some way? Also, if you are allowed to copy the code, I'd suggest that you add a comment in there detailing where the code came from, and if you had made changes to the copied code, create a block (use comments to state begin and end of changed code) so the readers know exactly what has been changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments about the origin of the code and what has been modified

socketProperties.getAppWriteBufSize(),
socketProperties.getDirectBuffer());
if (isSSLEnabled()) {
channel = new JSSSecureNioChannel(bufhandler, this);
Copy link
Contributor

@edewata edewata Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the original SecureNioChannel have the correct local & remote addresses? I was wondering if we can do something like this:

SecureNioChannel secureNioChannel = new SecureNioChannel(bufhandler, this);
SSLEngine sslEngine = secureNioChannel.getSslEngine();
JSSSession jssSession = (JSSSession) sslEngine.getSession();
jssSession.setLocalAddr(secureNioChannel.getLocalAddr());
jssSession.setRemoteAddr(secureNioChannel.getRemoteAddr());
channel = secureNioChannel;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SSLEngine is not created with the SecureNioChannel but when the handshake is performed. Actually, when the handshake is requested, among the initial operations there is the SNI process and the SSLEngine is created there:

https://github.com/apache/tomcat/blob/3e287ac4d7dbc04a364f10ede42b3518866d4ed1/java/org/apache/tomcat/util/net/SecureNioChannel.java#L304

Therefore, you have to perform this operation after the method performSNI() and before to complete the method handshake() because after the method the audit is written.

**SSLEngine** is by design unaware of the underlying communication channel.
In tomcat the communication channel is started by the classes
`NioEndpoint` and it is maintained in `SecureNioChannel` which will
create the buffer used with the SSLEngine in order to wrap and unwrap
the messages.

To allow the audit of TLS messages to include IP addresses of the client
and server, the above classed have been extended in order to store the
IPs in the SSLEngine session after its creation.
@sonarcloud
Copy link

sonarcloud bot commented Jul 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@fmarco76
Copy link
Member Author

This PR is closed because the code for future versions of tomcat-jss has migrated to the JSS project. The proposed changes will be submitted to that JSS project.

@fmarco76 fmarco76 closed this Aug 28, 2023
fmarco76 added a commit to fmarco76/jss that referenced this pull request Aug 28, 2023
**SSLEngine** is by design unaware of the underlying communication channel.
In tomcat the communication channel is started by the classes
`NioEndpoint` and it is maintained in `SecureNioChannel` which will
create the buffer used with the SSLEngine in order to wrap and unwrap
the messages.

To allow the audit of TLS messages to include IP addresses of the client
and server, the above classed have been extended in order to store the
IPs in the SSLEngine session after its creation.

Replace the tomcatJSS PR#73
(dogtagpki/tomcatjss#73)
fmarco76 added a commit to fmarco76/jss that referenced this pull request Aug 28, 2023
**SSLEngine** is by design unaware of the underlying communication channel.
In tomcat the communication channel is started by the classes
`NioEndpoint` and it is maintained in `SecureNioChannel` which will
create the buffer used with the SSLEngine in order to wrap and unwrap
the messages.

To allow the audit of TLS messages to include IP addresses of the client
and server, the above classed have been extended in order to store the
IPs in the SSLEngine session after its creation.

Replace the tomcatJSS PR#73
(dogtagpki/tomcatjss#73)
fmarco76 added a commit to fmarco76/jss that referenced this pull request Aug 28, 2023
**SSLEngine** is by design unaware of the underlying communication channel.
In tomcat the communication channel is started by the classes
`NioEndpoint` and it is maintained in `SecureNioChannel` which will
create the buffer used with the SSLEngine in order to wrap and unwrap
the messages.

To allow the audit of TLS messages to include IP addresses of the client
and server, the above classed have been extended in order to store the
IPs in the SSLEngine session after its creation.

Replace the tomcatJSS PR#73
(dogtagpki/tomcatjss#73)
fmarco76 added a commit to fmarco76/jss that referenced this pull request Aug 29, 2023
**SSLEngine** is by design unaware of the underlying communication channel.
In tomcat the communication channel is started by the classes
`NioEndpoint` and it is maintained in `SecureNioChannel` which will
create the buffer used with the SSLEngine in order to wrap and unwrap
the messages.

To allow the audit of TLS messages to include IP addresses of the client
and server, the above classed have been extended in order to store the
IPs in the SSLEngine session after its creation.

Replace the tomcatJSS PR#73
(dogtagpki/tomcatjss#73)
fmarco76 added a commit to dogtagpki/jss that referenced this pull request Aug 29, 2023
**SSLEngine** is by design unaware of the underlying communication channel.
In tomcat the communication channel is started by the classes
`NioEndpoint` and it is maintained in `SecureNioChannel` which will
create the buffer used with the SSLEngine in order to wrap and unwrap
the messages.

To allow the audit of TLS messages to include IP addresses of the client
and server, the above classed have been extended in order to store the
IPs in the SSLEngine session after its creation.

Replace the tomcatJSS PR#73
(dogtagpki/tomcatjss#73)
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.

None yet

3 participants