-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fixes #3553 - Support sslSession() in Jetty Client. #12179
Fixes #3553 - Support sslSession() in Jetty Client. #12179
Conversation
Implemented Connection.getSSLSession(), where the Connection can be obtained from the Request: request.getConnection().getSSLSession(). Updated documentation. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
* @return the {@link SSLSession} associated with the connection, or {@code null} | ||
* if the connection is not secure or the {@link SSLSession} is not available. | ||
*/ | ||
default SSLSession getSSLSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any benefit of returning an EndPoint.SslSessionData instead? It can provide more information or just the ID if the actual session is abstracted away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see much benefit? SSLSession
is from the JDK, so it is well known.
Also, this is the client, and it does not need the number of tricks the server should support when it's behind a reverse proxy (e.g. support non-standard HTTP headers that a proxy may add that specify the cipher suite, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw actually, I changed my mind.
The reason being HTTP/3, which won't have an SSLSession
, but there could be enough information to return the cipher suite and the peer certificates.
The reason is to allow return some information for HTTP/3. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
if (sslSessionData == null) | ||
r.abort(new IllegalStateException()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we are saying that having a non null SslSessionData
, even with all the fields null, is saying that the endpoint/connection is Ssl. I think this is consistent with other usage in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's just that we could not gather the TLS data (cipher, certificates, etc.).
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Implemented Connection.getSSLSession(), where the Connection can be obtained from the Request: request.getConnection().getSSLSession().
Updated documentation.