Skip to content

nntp: Add TLS support for the news session - #472

Merged
rgushchin merged 2 commits into
sashiko-dev:mainfrom
chucklever:nntp-tls
Sep 23, 2026
Merged

rgushchin merged 2 commits into
sashiko-dev:mainfrom
chucklever:nntp-tls

Conversation

@chucklever

Copy link
Copy Markdown
Contributor

NntpClient speaks only cleartext. lore.kernel.org offers neither
STARTTLS nor a 563 listener, but an internal public-inbox mirror
can require TLS.

NNTPS is implicit TLS. The handshake runs before the greeting, so
every command method is untouched (patch 1). Trust anchors come
from the host certificate store, which retains the local CA as a
property of the deployment rather than of the config. The config
does name the rustls crypto provider: ring and aws-lc-rs both
reach this binary through reqwest and lettre, and neither installs
a process default.

The knob is nntp.tls, default off, so an existing Settings.toml
continues to connect in the clear (patch 2). The port is not
derived from it. NNTPS listens on 563 rather than 119, and
deriving one from the other would silently move a server an
operator had pinned.

Note: I thought our internal security policy might require the use
of NNTPS, so I implemented these patches. But it turns out only port
22 is open between our internal sashiko and public-inbox instances.
So the patches are untested. Feel free to drop them.

@chucklever

Copy link
Copy Markdown
Contributor Author

Rebased onto 54a74ba to clear the merge conflict. The pair
applies cleanly again.

Two notes on the rebase.

The conflict in src/nntp.rs was mechanical. Your
group_validates_matching_group_name and this series'
tls_article_round_trip both append to the end of the same test
module. Both are kept.

d798e62 adds two NntpClient::connect() call sites, one in the
GROUP error path and one in the ARTICLE error path. This series
gives connect() a tls argument, so both sites needed updating.
They now pass settings.nntp.tls. Left passing false, a reconnect
after a failed GROUP or ARTICLE would drop to plaintext with
nntp.tls set, while the initial connection stayed on TLS.

The caveat in the description still holds. None of this has been
tested against a real NNTPS server.

NntpClient speaks only cleartext, so a deployment whose news server
sits across an untrusted network cannot protect the session.
lore.kernel.org offers neither STARTTLS nor a 563 listener, but an
internal public-inbox mirror can.

Wrap the session in an optional TLS layer. NNTPS is implicit, so the
handshake runs before the greeting and every command method is
unchanged. Trust anchors come from the host certificate store, which
leaves an internal CA a property of the deployment rather than of
the config. Walking that store is blocking file I/O, so the load
runs off the executor thread. The ingestor continues to connect in
the clear.

Dropping a TlsStream sends no close_notify, and a session that ends
that way is a truncation to a strict peer. quit() shuts the stream
down after the 205 so the TLS close is orderly.

Both ring and aws-lc-rs reach this binary through reqwest and
lettre. rustls cannot choose between them on its own, and neither
crate installs a process default, so the config names the provider.

Signed-off-by: Chuck Lever <cel@kernel.org>
The NNTP client can wrap its session in TLS, but nothing reaches
that path. A deployment whose news server requires NNTPS has no way
to ask for it.

Add nntp.tls and pass it to both connect sites. It defaults to off,
so an existing Settings.toml continues to connect in the clear.

The port is left alone. NNTPS listens on 563 rather than 119, and
deriving one from the other would silently move a server an
operator had pinned.

Signed-off-by: Chuck Lever <cel@kernel.org>
@chucklever

Copy link
Copy Markdown
Contributor Author

Rebased onto 06ba233 to clear the merge conflict; the pair applies cleanly again. Three notes.

The Cargo.toml conflict was mechanical: upstream added rustix beside the two rustls crates and a dev-dependency block beside rcgen. Both sides are kept. rustls-native-certs was already in the lock through reqwest's platform verifier, so the only new build weight is rcgen and its dev-only parsers.

The nntp section gained a Default impl upstream when it became optional, so the tls field is now initialized there too.

Three small fixes to patch 1 from a review pass over the rebased code: the close_notify write in quit() now runs under the client timeout like every other I/O, since a peer that has stopped reading can stall it; the TCP connect takes a (host, port) tuple, so an IPv6 literal that ServerName accepts also addresses the socket; and a dead Arc import in the test module is gone.

The caveat in the description still holds. None of this has been tested against a real NNTPS server.

@sashiko-bot

sashiko-bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

Sashiko review — v2

No issues found across 2 commits.

Full review log on sashiko.sashiko.dev

@rgushchin
rgushchin merged commit 3d45cdb into sashiko-dev:main Sep 23, 2026
3 checks passed
@chucklever
chucklever deleted the nntp-tls branch September 23, 2026 19:11
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