Skip to content

Lift ssl-context coercion to connection-pool fn#746

Merged
DerGuteMoritz merged 2 commits intoclj-commons:masterfrom
PawelStroinski:aleph-728-input-stream-trust-store
Sep 9, 2025
Merged

Lift ssl-context coercion to connection-pool fn#746
DerGuteMoritz merged 2 commits intoclj-commons:masterfrom
PawelStroinski:aleph-728-input-stream-trust-store

Conversation

@PawelStroinski
Copy link
Copy Markdown
Contributor

Addresses #728.

@KingMob KingMob removed their request for review March 3, 2025 07:19
@DerGuteMoritz
Copy link
Copy Markdown
Collaborator

Hey @PawelStroinski, sorry for the delay! Responding to the main commit message:

Keeping the client-ssl-context call in http-connection as is,
even though it might seem superfluous considering the code path taken in
the test, but http-connection is a public API, so we have to keep
the call (which for us is a no-op, if we ignore the repeated ALPN check)
even for our case when the protocol is https and ssl-context is supplied.

Hmm I wonder how public that API is really - note how the aleph.http.client has ^:no-doc metadata 🤔 Also, the docstring doesn't explain any of the options and there are no dedicated tests for it either. So IMHO we can consider this an internal API in practice and drop the client-ssl-context call here.

NOTE: This highlights a difference we are introducing here. Previously,
if we specified ssl-context, but the protocol wasn't https, we would just
ignore the ssl-context. Currently, we are coercing it ahead-of-time,
before knowing the request protocol. This could be alleviated by wrapping
the coercion in a delay, so it wouldn't happen until needed. Yet, given
how unlikely this scenario seems, I have doubts whether it'd be worth it.

I agree. If anyone runs into it, they can easily fix it since such a combination of options doesn't make sense to begin with 😅

I slightly dislike the repetition of [:http1] default value,
but since it serves as documentation in http-connection,
I decided to keep it as is rather than to extract it out.

See above - we can get rid of this then, too.

Also, I slightly dislike the repetition of a pattern to call
ensure-consistent-alpn-config and then coerce-ssl-client-context
but it's only now in 2 places, which I think is a better alternative
than adding yet another ssl-coercion layer/wrapping function.
Obviously, we cannot just move ensure-consistent-alpn-config to
ssl-client-context, since ALPN is only for HTTP.

Technically, ALPN can also be used for other purposes but in the context of Aleph's out-of-the-box support, this is true 😄 But as per above, we can get rid of this then, too.

Thanks for your thoughtful notes as always! 🙏

Both testing contexts are failing. The serial one is to demonstrate that
the InputStream cannot be read twice without resetting, which obviously
is not done by Netty/Aleph.

This is also the case in the concurrent context, which was intended to
resemble the original report in clj-commons#728 and is a more likely scenario,
since it doesn't disable keep-alive. IIUC, the concurrent scenario
could fail in an even more unpleasant way, if the test certificate file
was greater than the 8192-byte buffer used to read it, but ours is not
(the fix would be the same).

NB: `with-http-ssl-servers` already runs things twice, so `repeatedly`
is not required to make it fail, but that would be harder to read and
wouldn't cover (at some level, at least) both servers.
@PawelStroinski PawelStroinski force-pushed the aleph-728-input-stream-trust-store branch 2 times, most recently from ee00a76 to d9ae1de Compare September 6, 2025 15:53
@PawelStroinski
Copy link
Copy Markdown
Contributor Author

Thank you for the feedback, @DerGuteMoritz. Certainly, http-connection didn't even have a docstring until a recent (well... relatively, considering long Aleph history) refactoring and the ^:no-doc metadata on that namespace is a good hint regarding the intended usage (or lack of it), indeed. I've updated my PR taking advantage of that fact.

@PawelStroinski PawelStroinski force-pushed the aleph-728-input-stream-trust-store branch from d9ae1de to ae12ffa Compare September 6, 2025 16:08
As suggested by @DerGuteMoritz in clj-commons#728. This fixes the issue and makes
the test added in the previous commit pass.

To avoid a repeated coercion *also* in `http-connection` fn (which is an
internal fn called only from `connection-pool` fn indirectly), the coercion
is being removed from it. As a result, we have to rely solely on
`connection-pool` fn for the coercion, where we don't know whether SSL
context will be required or not. This includes a situation when a user hasn't
supplied any `ssl-context` options, and we are still building an
`SslContext`, not knowing whether actual requests will require it or not.
This might look like a bit of potentially unnecessary work, but this
hopefully won't affect anyone performance-wise, as it would be unusual to run
`connection-pool` fn in a tight loop. On the positive side, all users will
profit from the elimination of repeated `SslContext` building, whether they
supply `ssl-context` options or not. Above all, having consistent code paths
in both situations is nice.

Notes:
- `http-connection` fn has now always some `ssl-context` available regardless
  of `ssl?` flag, but the only place where it matters
  (`client/make-pipeline-builder`) already checks `ssl?` flag before making
  use of `ssl-context`, so this makes no difference.
- All rich comments exercising `http-connection` fn with `ssl?` flag were
  updated, as now the caller has to supply `ssl-context` in that case.
@PawelStroinski PawelStroinski force-pushed the aleph-728-input-stream-trust-store branch from ae12ffa to f5b1039 Compare September 6, 2025 16:22
@DerGuteMoritz DerGuteMoritz merged commit 6465e3a into clj-commons:master Sep 9, 2025
1 check 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