Skip to content

fail deploy when RELP transport is combined with TLS#252

Open
dlinsley wants to merge 1 commit into
cloudfoundry:mainfrom
dlinsley:feature/fix-relp-tls-cleartext
Open

fail deploy when RELP transport is combined with TLS#252
dlinsley wants to merge 1 commit into
cloudfoundry:mainfrom
dlinsley:feature/fix-relp-tls-cleartext

Conversation

@dlinsley

Copy link
Copy Markdown

Description

Fixes a security defect where syslog_forwarder configured with transport: relp and
tls_enabled: true would render and deploy successfully while silently shipping logs in
cleartext.

The $ActionSendStreamDriver* directives emitted when tls_enabled is true only configure
the netstream driver used by the omfwd module (TCP/UDP @/@@ actions). They have no
effect on omrelp, which is rendered via legacy selector syntax
(*.* :omrelp:<addr>:<port>;Template) — a syntax form with no TLS parameters at all. RELP's
own TLS stack (librelp) can only be configured via RainerScript
(action(type="omrelp", tls="on", ...)), which this release doesn't use anywhere.

This PR adds a fail-fast guard — mirroring the existing UDP+TLS guard already in the
template — so bosh deploy now fails outright for this combination instead of quietly
running an insecure connection. The same guard is added for fallback_servers entries with
transport: relp, which had the identical hole.

Also fixes tests/manifests/relp-tls.yml, which omitted syslog.address/syslog.port and
therefore never actually exercised the relp transport (it silently fell back to the
storer link's default tcp) — meaning the RELP+TLS combination was previously untested
entirely.

Real RELP TLS support (converting the RELP forwarding rule to RainerScript, wiring cert
properties, reworking fallback-server chaining) is intentionally out of scope for this fix;
it's a larger feature-level change that isn't required to close this security exposure.
Can be introduced in future PR.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Note: this is technically breaking for anyone currently (mis)configured with
transport: relp + tls_enabled: true — their next deploy will now fail instead of
silently running in cleartext. That's the intended outcome.

Testing performed?

  • Unit tests
  • Integration tests
  • Acceptance tests

tests/acceptance_test.go's "when TLS is configured over relp" context now asserts that
bosh deploy fails (mirroring the existing broken-rules.yml invalid-config test) instead
of asserting successful log forwarding.

Checklist:

  • This PR is being made against the main branch, or relevant version branch
  • I have made corresponding changes to the documentation
  • I have added testing for my changes

If you have any questions, or want to get attention for a PR or issue please reach out on the #logging-and-metrics channel in the cloudfoundry slack

RELP has no TLS support in this release: TLS directives only affect
the omfwd module (TCP/UDP), and RELP's legacy selector syntax has no
TLS parameters. Previously transport: relp + tls_enabled: true
rendered and deployed successfully while silently shipping logs in
cleartext. Raise a template error instead, mirroring the existing
UDP+TLS guard, for both the primary transport and RELP fallback
servers. Also fix relp-tls.yml, which omitted syslog.address/port and
so never actually exercised the relp transport.

@jorbaum jorbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

Nice find! Looks like this bug has been there for roughly 7 years if I looked correctly. Also nice to see you both applying a minimal fix and proposing how to add tls support in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants