Skip to content

Fix failure to initialise Netty channel when using :manual-ssl? option #752

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix failure to initialise Netty channel when using :manual-ssl? option #752

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/aleph/http/server.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@
:handler handler
:server? true
:pipeline pipeline)]
(cond ssl?
(cond (and ssl? ssl-context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

While the fix would work, I think it's better if it's failing (maybe more gracefully with a clearer error message) because you don't expect a server to start without TLS when the ssl? parameter is passed with true.

Copy link
Collaborator

@DerGuteMoritz DerGuteMoritz May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note that ssl? isn't a parameter that the user can pass to aleph.http/start-server. Instead, it is derived from manual-ssl? and ssl-context.

FTR: I haven't yet reviewed the patch but intend to do so in the course of the week. Meanwhile, thanks a lot for your contribution, @scramjet! One thing I would like is to have a regression test for this but I can also take care of that if you don't have the bandwidth 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Off-topic: At first I accidentally linked to a Jitsi meeting room there because I failed to copy the intended URL - fixed now. Please ignore the other URL in case you got it via email notification 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking at this.

While the fix would work, I think it's better if it's failing (maybe more gracefully with a clearer error message) because you don't expect a server to start without TLS when the ssl? parameter is passed with true

:manual-ssl? = true indicates "I'm going to add the SSL handler(s) myself, trust me I know what I'm doing", and failing like it does breaks the ability to use it at all. As @DerGuteMoritz points out, ssl? is a handy internal flag to consult to see if SSL will be used at all.

I wish I had the time to write a test right now. In fact I wish I had made one in the original PR (#423) back when I had more time. Would have saved some grief now!

(let [ssl-handler (netty/ssl-handler (.channel pipeline) ssl-context)]
(log/debug "Setting up secure HTTP server pipeline.")
(log/debug "ALPN HTTP versions:" (mapv str (.nextProtocols ssl-context)))
Expand Down