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

Refactor SSLSocket and JSSSocket #1014

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Refactor SSLSocket and JSSSocket #1014

merged 1 commit into from
Jul 23, 2024

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Jul 23, 2024

SSLSocket is an older code based on the plain Java Socket. JSSSocket is a newer code based on Java SSLEngine and should eventually replace SSLSocket.

To help the transition, SSLSocket has been modified to extend javax.net.ssl.SSLSocket and JSSSocket has been modified to extend SSLSocket. Once everything is migrated to JSSSocket, the SSLSocket can be deprecated and eventually dropped.

https://github.com/edewata/jss/blob/socket/docs/changes/v5.6.0/API-Changes.adoc

SSLSocket is an older code based on the plain Java Socket.
JSSSocket is a newer code based on Java SSLEngine and should
eventually replace SSLSocket.

To help the transition, SSLSocket has been modified to extend
javax.net.ssl.SSLSocket and JSSSocket has been modified to
extend SSLSocket. Once everything is migrated to JSSSocket, the
SSLSocket can be deprecated and eventually dropped.
@edewata edewata requested a review from fmarco76 July 23, 2024 02:23
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

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

The change for SSLSocket looks good to me. I have a concern for the JSSSocket but I am OK either way.

Since there is an alternative, could we add the deprecated now in SSLSocket?

import javax.net.ssl.X509KeyManager;
import javax.net.ssl.X509TrustManager;

import org.mozilla.jss.pkcs11.PK11Cert;
import org.mozilla.jss.pkcs11.PK11PrivKey;
import org.mozilla.jss.provider.javax.crypto.JSSTrustManager;
import org.mozilla.jss.ssl.SSLSocket;
Copy link
Member

Choose a reason for hiding this comment

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

If the goal is to deprecate/remove SSLSocket it is not clear to me the reason of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to simplify the transition by minimizing the required changes to existing code. Basically any code that creates the old SSLSocket can be changed to create JSSSocket instead, but the code that uses the SSLSocket instance can automatically use the JSSSocket instance without any changes since it's a subclass, and JSSSocket will inherit the constants defined in SSLSocket too.

Copy link
Member

Choose a reason for hiding this comment

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

The changes will be needed in order to remove SSLSocket but we can do later.

@edewata
Copy link
Contributor Author

edewata commented Jul 23, 2024

@fmarco76 Thanks! I'll merge it now but feel free to continue the discussion.

About deprecation, if we add it now it will just generate more warnings during the build. I'd rather we add it later once we migrate all of our code (i.e. in jss itself, ldapjdk and pki) to use JSSSocket and making sure that it can fully replace the old SSLSocket.

@edewata edewata merged commit 832022d into dogtagpki:master Jul 23, 2024
32 of 34 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.

2 participants