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

Refactoring in security (TLS) #25056

Merged
merged 14 commits into from
Jul 22, 2024
Merged

Refactoring in security (TLS) #25056

merged 14 commits into from
Jul 22, 2024

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Jul 19, 2024

  • Simplified
  • Fixed many NPEs and swallowed exceptions
  • Fixed HK2 related issues (missing @Contract, inaccessible classes in a context)
  • Narrowed dependencies (a bit)
  • I believe it is not worse than before :-) , but the progress is very slow and painful. Usual mistakes:
    • Typo in reflection. GlassFish vs. Glassfish took 3 hours.
    • Missing @Contract was the cause of all that original boilerplate code
    • Swallowed exceptions -> misleading logs reporting "something broken" when it was already broken three steps before.
    • Added dependency just for one import = bad architecture, something should be on another place.
  • All tests passed on OmniFish's Jenkins including TCK

Review

  • Better per commit. All changes are related - usually I broke something then noticed the error is not logged or is shadowed by another error, so I had to fix that first. Finally I reached some "safe point", committed, and then I split that commit to several smaller commits to at least partially meaningful blocks of the work.

@dmatej dmatej force-pushed the tls branch 4 times, most recently from 252d005 to 0756c8a Compare July 20, 2024 10:10
dmatej added 13 commits July 20, 2024 23:05
- It was referred just from two other constants which had to have the
  dependency on deployment-common
- JWS will be removed soon OR replaced by the Open Web Start

Signed-off-by: David Matějček <[email protected]>
- the sleep can be interrupted
- reproduced randomly on local machine
- shortened sleep time to 100 ms

Signed-off-by: David Matějček <[email protected]>
- Added missing logs (when some class was not available, it did not even log it)
- Removed e.printstacktrace when we already have logging or throw
- Added missing exception cause

Signed-off-by: David Matějček <[email protected]>
- When I broke some classes critical for startup, this was throwing NPE too

Signed-off-by: David Matějček <[email protected]>
- import order, wildchar imports removed
- final fields
- redundant modifiers, fixed order
- improved braces
- improved generics
- habitat renamed to locator
- using foreach were possible
- throwing exceptions with causes

Signed-off-by: David Matějček <[email protected]>
- Now it is injectable
- Simplified initialization of related classes

Signed-off-by: David Matějček <[email protected]>
- SSLImplementation is an interface and HK2 Contract, GlassfishSSLImpl
  implements it
- SecurityRoleMapperFactoryGen moved to core-ee where is the only usage of it
- SSLConfigurator - lookup removed; in all cases I have seen now works the
    locator variant, but I left the class loader variant here for now.
- JSSEImplementation was used just in tests
    -> moved there as JSSEImplementation4Tests
- SecureAdminConfigUpgrade - the comment was not true any more
- GlassfishServerSocketFactory now always uses locator

Signed-off-by: David Matějček <[email protected]>
- Changes based on directly used packages in the module
- I will do that with maven plugin later again.

Signed-off-by: David Matějček <[email protected]>
@dmatej dmatej marked this pull request as ready for review July 20, 2024 23:42
@dmatej dmatej requested review from arjantijms, pzygielo, avpinchuk and a team July 20, 2024 23:42
@dmatej dmatej self-assigned this Jul 20, 2024
@dmatej dmatej added this to the 7.0.16 milestone Jul 20, 2024
@@ -45,7 +46,7 @@
public class SecureRMIServerSocketFactory extends SslRMIServerSocketFactory {

private final InetAddress mAddress;
private final ServiceLocator habitat;
private final ServiceLocator locator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely tiny remark, and no need to change it, but I think "serviceLocator" as variable name would be more clear (various other locations use that too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use both, I prefer shorter version as it is clear in the context what it is.

@dmatej dmatej requested a review from pzygielo July 22, 2024 11:29
@dmatej dmatej merged commit 1dc7069 into eclipse-ee4j:master Jul 22, 2024
2 checks passed
@dmatej dmatej deleted the tls branch July 22, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants