Add IPs address to the session #972
Conversation
|
@edewata there are several missing dependencies during the build but I have not done any change on that. Are they related to the update done for Packit? |
Yes there is no |
This is strange. I have modified dependencies on |
| public String getRemoteAddr() { | ||
| return remoteAddr; | ||
| } | ||
|
|
||
| public void setRemoteAddr(String remoteAddr) { | ||
| this.remoteAddr = remoteAddr; | ||
| } |
There was a problem hiding this comment.
The class already has a peerHost. Should we just use that instead of adding a remoteAddr?
There was a problem hiding this comment.
The class already has a
peerHost. Should we just use that instead of adding aremoteAddr?
The peerHost contains the hostname of the client and is assigned by JSSEngine. The audit has the IP addresses of client and server.
There was a problem hiding this comment.
@ladycfu Do we need to capture IP addresses specifically, or are hostnames fine too?
There was a problem hiding this comment.
the current version of pp says we are allowed to provide at least one of [IP Address, FQDN, user FQDN, or cert DN], however it also added that in the future, they will all be required.
fmarco76
force-pushed
the
EventACCESSMissIP
branch
2 times, most recently
from
fa24a5f
to
c28b0dc
Compare
|
Additionally, the handshake failure would not fire the audit event because there was not invocation of |
| if (seen_exception == false && ssl_exception == null) { | ||
| ssl_exception = checkSSLAlerts(); | ||
| seen_exception = (ssl_exception != null); | ||
| SSLException checkException = checkSSLAlerts(); | ||
| if (checkException != null && !seen_exception) { | ||
| ssl_exception = checkException; | ||
| seen_exception = ssl_exception != null; | ||
| } |
There was a problem hiding this comment.
I'm not really familiar with this code, but it looks like the original code would check for SSL alerts then stop checking once it has seen an exception, and the new code will always check for SSL alerts but discard all exceptions after the first one. Is that the intended change?
I think the new code can be simplified like this:
SSLException checkException = checkSSLAlerts();
if (checkException != null && !seen_exception) {
ssl_exception = checkException;
seen_exception = true;
}
There was a problem hiding this comment.
I'm not really familiar with this code, but it looks like the original code would check for SSL alerts then stop checking once it has seen an exception, and the new code will always check for SSL alerts but discard all exceptions after the first one. Is that the intended change?
The problem with the previous code was that if an exception raise here the method checkSSLAlerts() is never invoked and TLS alerts are not propagated (and audit are not written). With this change it is invoked in any case but the first error is reported.
There was a problem hiding this comment.
I have one question due to a possible redundancy, but no objections if that's required to fix the issue. I'll defer to @ladycfu and @jmagne for merging. I suppose the more critical changes are in tomcatjss/pull/73.
The SSLEngine session "JSSSession" has been extended to container the IP addresses of the client and the server. These are used for the audit and have not other use in the protocol. By design the SSLEngine should be unaware of the underlying communication but this is need to keep the original audit format required for the certification.
When the handshake failure event fails it should fire an audit event for ACCESS_SESSION_ESTABLISH with the details (IPs, outcome, description, ...). These event were never fired because in case of handshake error the input stream is unwrapped until the end and output stream is wrapped and data sent to the peer, then closed together. All this without checking if TLS event were present in the stream. The code is fixed and now the check for event is done on any error and the first one is reported.
fmarco76
force-pushed
the
EventACCESSMissIP
branch
from
c28b0dc
to
3f7416d
Compare
|
Kudos, SonarCloud Quality Gate passed! |
The SSLEngine session "JSSSession" has been extended to container the IP addresses of the client and the server. These are used for the audit and have not other use in the protocol. By design the SSLEngine should be unaware of the underlying communication but this is need to keep the original audit format required for the certification.