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.

Sign up for GitHub

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

Jump to bottom

Split SslContextFactory into Client and Server #3464

Closed
sbordet opened this issue Mar 14, 2019 · 7 comments
Closed

Split SslContextFactory into Client and Server #3464

sbordet opened this issue Mar 14, 2019 · 7 comments
Labels
Enhancement

Comments

@sbordet
Copy link
Contributor

sbordet commented Mar 14, 2019

Following discussion on #3454 (comment), the proposal is to add 2 subclasses of SslContextFactory, namely SslContextFactory.Client and SslContextFactory.Server and each subclass overrides both the default configuration and the warning it emits.

Currently there is only one set of defaults, and it's not possible to configure them in a way that work well for both client and server.

For example, the default is endpointIdentificationAlgorithm="HTTPS" which is good for the client. However, when using client certificate authentication on the server, this default setting is not good and needs to be changed to null, but null emits a warning at server startup which is not ideal and may confuse people.

@gregw thoughts?
@joakime
Copy link
Contributor

joakime commented Mar 15, 2019

almost think having a mode (CLIENT | SERVER) on SslContextFactory makes more sense.
the rest of the configuration is fine, its just the combination of client certificates being used when connecting to a server that messes things up with regards to endpointIdentificationAlgorithm.

or what about a SslContextFactory clientSsl = scf.toClient() and SslContextFactory serverSsl = scf.toServer() that handles the client vs server specifics, but still allows for single configuration location for common things like the keystore/truststore setup?

@joakime
Copy link
Contributor

joakime commented Mar 15, 2019

Meh, mode and toClient/toServer wouldn't help address the warnings tho.

@sbordet
Copy link
Contributor Author

sbordet commented Mar 15, 2019

@joakime exactly, hence the idea of subclassing to make the behavior overridable.

@sbordet sbordet mentioned this issue Mar 15, 2019
Closed
@gregw
Copy link
Contributor

gregw commented Mar 15, 2019

What behaviour for current users that are using the class directly?

@sbordet
Copy link
Contributor Author

sbordet commented Mar 15, 2019

@gregw the current behavior.

sbordet added a commit that referenced this issue Mar 21, 2019
@sbordet
Issue #3464 - Split SslContextFactory into Client and Server
6fb243f 
Introduced SslContextFactory subclasses Client and Server.
Replaced all usages of SslContextFactory with either Client or Server
as required.

Refactored configuration checking so that warnings are not emitted
when non necessary.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet mentioned this issue Mar 21, 2019
Merged
@joakime
Copy link
Contributor

joakime commented Mar 21, 2019

Opened PR #3480

sbordet added a commit that referenced this issue Apr 2, 2019
@sbordet
Issue #3464 - Split SslContextFactory into Client and Server
eaada1a 
Updated documentation referencing the 2 new subclasses.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Apr 2, 2019
@sbordet
Merge pull request #3480 from eclipse/jetty-9.4.x-3464-split_sslconte…
c4b2621 
…xtfactory

Issue #3464 - Split SslContextFactory into Client and Server
sbordet added a commit that referenced this issue Apr 3, 2019
@sbordet
Issue #3464 - Split SslContextFactory into Client and Server
a18c2c4 
Fixed a couple of places where Server was used instead of Client.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@joakime
Copy link
Contributor

joakime commented Apr 8, 2019

Merged, closing issue.

@joakime joakime closed this as completed Apr 8, 2019
sbordet added a commit that referenced this issue Apr 15, 2019
@sbordet
Issue #3464 - Split SslContextFactory into Client and Server
3f0ec63 
Fixed instanceof check to maintain backwards compatibility.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@joakime joakime mentioned this issue Apr 15, 2019
Closed
kmailkarthik pushed a commit to kmailkarthik/spring-boot that referenced this issue Jul 17, 2019
Changes to fix client auth issue. Fix to not use deprecated class.
38578f4 
jetty/jetty.project#3454
jetty/jetty.project#3464
@kmailkarthik kmailkarthik mentioned this issue Jul 17, 2019
Closed
@jdeppe-pivotal jdeppe-pivotal mentioned this issue Oct 16, 2019
Merged
6 tasks
@joakime joakime mentioned this issue Dec 3, 2019
Closed
@andy-xian-hai andy-xian-hai mentioned this issue Mar 27, 2020
Closed
viv added a commit to viv/Openfire that referenced this issue May 16, 2023
@viv
WIP: Upgrades Jetty 9.4.43.v20210629 to 10.0.15
2c0f741 
Remaining Issues:
- JSP compilation failure caused by:

apache/tomcat@224fb6c#diff-ef02fd9c3f90ca4838d3cdaa051489556eca75d537fc396022e0ed456c24d895R329

- `WebSocketClientConnectionHandler.onConnect` assumes that `session.getRemoteAddress()` supplies an `InetSocketAddress` which might not always be true. What's the best approach in our context?

Fixed in this commit:
- Websocket server Maven artifact renamed from `websocket-server` to `websocket-jetty-server`
- `WebSocketServlet` renamed to `JettyWebSocketServlet`
- `WebSocketServletFactory` renamed to `JettyWebSocketServletFactory`

See: https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-migration-94-to-10

- `GzipHandler` is no longer part of `ServletContextHandler`
jetty/jetty.project#4765

- Renamed `setWebInfClassesDirs` to `setWebInfClassesResources`
jetty/jetty.project#4506

- Split `SslContextFactory` into Client and Server
jetty/jetty.project#3464
@viv viv mentioned this issue May 16, 2023
Merged
viv added a commit to viv/Openfire that referenced this issue Sep 4, 2023
@viv
WIP: Upgrades Jetty 9.4.43.v20210629 to 10.0.15
5e9befe 
Remaining Issues:
- JSP compilation failure caused by:

apache/tomcat@224fb6c#diff-ef02fd9c3f90ca4838d3cdaa051489556eca75d537fc396022e0ed456c24d895R329

- `WebSocketClientConnectionHandler.onConnect` assumes that `session.getRemoteAddress()` supplies an `InetSocketAddress` which might not always be true. What's the best approach in our context?

Fixed in this commit:
- Websocket server Maven artifact renamed from `websocket-server` to `websocket-jetty-server`
- `WebSocketServlet` renamed to `JettyWebSocketServlet`
- `WebSocketServletFactory` renamed to `JettyWebSocketServletFactory`

See: https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-migration-94-to-10

- `GzipHandler` is no longer part of `ServletContextHandler`
jetty/jetty.project#4765

- Renamed `setWebInfClassesDirs` to `setWebInfClassesResources`
jetty/jetty.project#4506

- Split `SslContextFactory` into Client and Server
jetty/jetty.project#3464
guusdk pushed a commit to igniterealtime/Openfire that referenced this issue Sep 7, 2023
@viv @guusdk
WIP: Upgrades Jetty 9.4.43.v20210629 to 10.0.15
09c12d2 
Remaining Issues:
- JSP compilation failure caused by:

apache/tomcat@224fb6c#diff-ef02fd9c3f90ca4838d3cdaa051489556eca75d537fc396022e0ed456c24d895R329

- `WebSocketClientConnectionHandler.onConnect` assumes that `session.getRemoteAddress()` supplies an `InetSocketAddress` which might not always be true. What's the best approach in our context?

Fixed in this commit:
- Websocket server Maven artifact renamed from `websocket-server` to `websocket-jetty-server`
- `WebSocketServlet` renamed to `JettyWebSocketServlet`
- `WebSocketServletFactory` renamed to `JettyWebSocketServletFactory`

See: https://www.eclipse.org/jetty/documentation/jetty-10/programming-guide/index.html#pg-migration-94-to-10

- `GzipHandler` is no longer part of `ServletContextHandler`
jetty/jetty.project#4765

- Renamed `setWebInfClassesDirs` to `setWebInfClassesResources`
jetty/jetty.project#4506

- Split `SslContextFactory` into Client and Server
jetty/jetty.project#3464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gregw @joakime @sbordet