feature(auth/EXTERNAL): Add support for EXTERNAL Authentication mechanism - X509 Certificates by lukas8219 · Pull Request #1897 · cloudamqp/lavinmq

lukas8219 · 2026-04-30T19:28:26Z

WHAT is this pull request doing?

Adding support for EXTERNAL mechanism allowing for X509 Certificate CN or SAN authentication.

HOW can this pull request be tested?

WIP. Manually for now

Work In Progress Tracking

Rename configuration as expected
Write Specs to ensure the behavior is in-place
Move errors to their own classe
Move EXTERNAL to separate module/class
Evaluate wether we really need to store SSL CN/SAN in-memory for each connection. Too heavy for tons of connections
Review log strategy. What needs to be logged?
The SAN type is not handled properly by X509::Extension. It's a pure string. Should we have parsing in NativeSocket?

Closes #1782

lukas8219 · 2026-04-30T19:29:06Z

          resp = start_ok.response
          if i = resp.index('\u0000', 1)
-            {resp[1...i], resp[(i + 1)..-1]}
+            {resp[1...i], resp[(i + 1)..-1], false}


Just for local development for now...

claude · 2026-04-30T19:30:06Z

PR Review: EXTERNAL Authentication Mechanism (x509)

1. Security: Authentication bypass for EXTERNAL users

src/lavinmq/auth/authenticators/local.cr:12

return user unless !context.is_ssl_login

This line returns the user immediately when is_ssl_login is true, skipping all remaining checks: default_user_only_loopback?, password existence, and password verification. This means:

A client presenting a certificate whose CN matches the default_user can authenticate from a remote address even when default_user_only_loopback is enabled. This defeats an explicit security restriction.
Any user in the user store can be accessed via EXTERNAL auth with no further validation beyond CN matching — there is no check that the matched user is permitted to use certificate-based auth.

Consider adding an explicit permission or flag on the user that controls whether certificate-based login is allowed, rather than unconditionally trusting any CN that matches a username.

2. Crash on EXTERNAL mechanism without valid certificate CN

src/lavinmq/amqp/connection_factory.cr:112

{connection_info.ssl_cn.not_nil!, "", true}

ssl_cn is String? (nil when no client certificate is presented, or when the certificate has no CN field, or when the connection is not TLS at all). Using not_nil! will raise NilAssertionError, crashing that fiber. There is no guard ensuring:

The connection is actually TLS (connection_info.ssl is not checked).
A client certificate was presented.
The certificate contains a CN.

A client connecting on a plaintext AMQP port and sending EXTERNAL as the mechanism will hit this crash path. This should return an authentication error, not raise an unhandled exception. Replace with a guard that returns a proper auth failure.

3. `extract_subject_alternative_name` / `extract_common_name` should be private

src/lavinmq/server.cr:245-262

Both extract_subject_alternative_name and extract_common_name are defined as def (public) on Server. They are internal helpers and should be private def to avoid polluting the public API.

4. Missing specs

CLAUDE.md requires: "All code changes must have corresponding specs."

This PR adds a new authentication mechanism, modifies authentication logic in LocalAuthenticator, adds new fields to Context and ConnectionInfo, and adds certificate parsing methods — but includes zero specs. At minimum, specs should cover:

EXTERNAL mechanism happy path (valid CN, user exists)
EXTERNAL mechanism with nil ssl_cn (no certificate / no CN)
EXTERNAL mechanism on a non-TLS connection
LocalAuthenticator behavior when is_ssl_login is true (including the default_user_only_loopback interaction)
extract_common_name and extract_subject_alternative_name with various certificate shapes

5. Debug logging should be removed

Multiple Log.debug lines across the diff look like development artifacts:

src/lavinmq/amqp/connection_factory.cr:94 — logs mechanism
src/lavinmq/amqp/connection_factory.cr:118 — logs full connection_info (may leak TLS details)
src/lavinmq/server.cr:254,257,259 — logs certificate subject details
src/lavinmq/launcher.cr:302 — logs TLS cert path

These should be cleaned up before merge. In particular, line 118 logs the entire connection_info object at debug level, which could include sensitive connection metadata.

lukas8219 · 2026-04-30T19:31:18Z

        context = Auth::Context.new(
          username,
          password.to_slice,
+          is_ssl_login: is_external_mechanism,


Based on RabbitMQ implementation, it looks like it uses the UsersStore (defined users) for ACL.

Meaning, you need to define users so X509 works. All examples in the documentation I've seen was using this manner.

So for LocalAuthenticator logic to work, reused in HTTP etc, there might be the need for some things to move around or simply add a flag like this.

On a second thought: maybe renaming to allow_passwordless could be more than enough for this...

what is it used for? Just displaying in the UI?

@carlhoerberg is used for this LocalAuthenticator check.

X509 auth is "passwordless" but still uses the Local users

ok, but it should be called something else, right?

Suggested change

is_ssl_login: is_external_mechanism,

external_authentication: is_external_mechanism,

lukas8219

There's also a change in amqp-protocol.cr needed it seems...

The START packet is not offering EXTERNAL.

Matter of fact - in RabbitMQ that's configurable - the ordering.

Does it make sense for us to also do it?

claude · 2026-04-30T19:35:29Z

PR Review: EXTERNAL (x509) authentication mechanism

1. `not_nil!` crash when EXTERNAL is used without a client certificate

src/lavinmq/amqp/connection_factory.cr:112

{connection_info.ssl_cn.not_nil!, "", true}

If a client sends EXTERNAL as the SASL mechanism but no client certificate was presented (or the connection isn't even TLS), ssl_cn is nil and not_nil! raises NilAssertionError, crashing the fiber. This should be a graceful auth rejection, not a crash. The code should check that ssl_cn is present and return an auth failure if it isn't.

2. Security: No verification that EXTERNAL is only used over TLS with a verified client cert

src/lavinmq/amqp/connection_factory.cr:110-112

There is no check that connection_info.ssl? is true before accepting the EXTERNAL mechanism. A client connecting over plain TCP can claim EXTERNAL mechanism, hit the not_nil! crash (issue 1), or if that's fixed, potentially authenticate without any certificate. The EXTERNAL mechanism should be rejected unless the connection is TLS and a client certificate was actually presented and verified.

3. Security: EXTERNAL auth bypasses `default_user_only_loopback?` check

src/lavinmq/auth/authenticators/local.cr:12

return user unless !context.is_ssl_login

When is_ssl_login is true, the user is returned immediately — bypassing both password verification (intended) and the default_user_only_loopback? guard. This means a client cert with CN matching the default user (typically "guest") can authenticate from any remote IP, even when default_user_only_loopback is configured. The loopback check should still apply for the default user regardless of auth mechanism.

4. Stale TODO comment contradicts implementation

src/lavinmq/amqp/connection_factory.cr:109

# TODO handle "EXTERNAL"
when "EXTERNAL"

The comment says "TODO handle EXTERNAL" but the handler is implemented right below it.

5. Missing specs

Per project rules, all code changes must have corresponding specs. This PR has no specs for:

The EXTERNAL authentication mechanism
extract_common_name / extract_subject_alternative_name
The modified LocalAuthenticator#authenticate behavior with is_ssl_login
Edge cases: no client cert, non-TLS + EXTERNAL, default user via EXTERNAL from remote IP

lukas8219 · 2026-04-30T19:36:17Z

Why is Claude being activated on Draft's also? I am being roasted for nothing lol

lukas8219 · 2026-04-30T21:06:39Z

Blocked by #1896

carlhoerberg · 2026-05-10T09:05:31Z

        context = Auth::Context.new(
          username,
          password.to_slice,
+          is_ssl_login: is_external_mechanism,


what is it used for? Just displaying in the UI?

claude · 2026-06-03T15:06:43Z

PR Review: EXTERNAL (x509) authentication mechanism

Re-reviewed the current diff. The refactor into per-mechanism Mechanism classes is clean, and the security-critical path is sound: EXTERNAL derives the username exclusively from ssl_cn / ssl_san_entries, which are only populated from a verified peer certificate in with_ssl. A client with no cert (or a non-TLS connection) hits || raise, so the password bypass in LocalAuthenticator (return user if context.external_authentication?) cannot be triggered without a real certificate. Coverage across unit + e2e is good.

No blocking issues found. Two low-severity notes (non-blocking):

src/lavinmq/auth/mechanisms/external.cr:35 — matching[san_index]? with a negative external_auth_san_index (e.g. -1) silently selects from the end of the array via Crystal negative indexing rather than raising "missing SAN". A misconfigured negative index resolves to an unexpected identity instead of failing. Consider guarding san_index >= 0.
Leftover debug logging: connection_factory.cr:551 and :558, and launcher.cr:306. These are Log.debug (gated), so harmless in production, but authenticate#connection_info=... dumps the full ConnectionInfo — worth trimming.

lukas8219 · 2026-06-06T00:34:54Z

Config Keys

RabbitMQ uses root-level config keys such as ssl_cert_login_from.

Should we have an INI section only for that?

I initially started with:

[external_auth]

X509 Extension Parsing

The X509::Extension class does not handle the type properly.

X509::Extension.value is a key:value string that needs to be parsed, likely by splitting it, for the compliant keys used here:

https://github.com/rabbitmq/rabbitmq-server/blob/a3b748234e3d69a921c5f4e01fc761157e7c8c9b/deps/rabbit/src/rabbit_ssl.erl#L212

Design Question

What is the usual mindset here?

Should we parse it early, for example at NativeSocket::Server or a similar boundary, or should we leave it as manual handling where it is actually used?

Current Preference

I am leaning toward not storing the SAN or CN in ConnectionInfo.

Instead, I would load or parse them on demand for callers, because storing them directly could clutter ConnectionInfo and make each connection more memory-expensive.

…stance

- Moved `ssl_client` to ConnectionInfo - Might stop storing in-memory and just letting ConnectionFactory itself parse the CN and ALT_NAME on-demand since it is only used during Authentication - ConnectionFactory needs access to the global Config object so it decides which config to use - `Type` is not handled in X509::Extension in Crystal. `value` is a plain string that needs parsing. Look into either 1) make it simple 2) properly parse those

Now we have the configurations in place and how that would work. Lots of bad `Nil?` handling in place. Code is getting cluttered and needs some cleaningm, perharps moving into a separate class for the EXTERNAL Authencation logic. Next step is writing E2E Test Cases. Will take some reference from TLS tests - [] Move errors to their own classe - [] Move EXTERNAL to separate module/class - [] Evaluate wether we really need to store SSL CN/SAN in-memory for each connection. Too heavy for tons of connections - [] Review log strategy. What needs to be logged?

lukas8219 · 2026-07-10T20:40:35Z

For proper specs, it needs cloudamqp/amqp-client.cr#74

github-actions Bot assigned lukas8219 Apr 30, 2026

github-actions Bot added the missing-docs label Apr 30, 2026

lukas8219 commented Apr 30, 2026

View reviewed changes

Comment thread src/lavinmq/server.cr Outdated

lukas8219 commented Apr 30, 2026

View reviewed changes

lukas8219 mentioned this pull request Apr 30, 2026

x.509 certificate authentication #1782

Open

carlhoerberg requested changes May 10, 2026

View reviewed changes

kickster97 removed the missing-docs label May 26, 2026

lukas8219 commented Jun 6, 2026

View reviewed changes

Comment thread src/lavinmq/amqp/connection_factory.cr Outdated

lukas8219 commented Jun 6, 2026

View reviewed changes

Comment thread src/lavinmq/connection_info.cr Outdated

lukas8219 commented Jun 6, 2026

View reviewed changes

Comment thread src/lavinmq/config/options.cr Outdated

lukas8219 added 7 commits July 10, 2026 16:04

wip: initial changes to make EXTERNAL work. A/B Test with RabbitMQ In…

f56dcca

…stance

wip: working version! found bug in passwordless users

0e7f276

Restored src/stdlib/native_socket.cr

a44bbbb

More cosmetic changes

b288db6

removed native_socket file

7dbadae

lukas8219 force-pushed the x509 branch from 27ef746 to 7dbadae Compare July 10, 2026 19:08

Unit Tests and Bug fixes

7c00e13

	is_ssl_login: is_external_mechanism,
	external_authentication: is_external_mechanism,

Conversation

lukas8219 commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHAT is this pull request doing?

HOW can this pull request be tested?

Work In Progress Tracking

Uh oh!

lukas8219 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Apr 30, 2026

PR Review: EXTERNAL Authentication Mechanism (x509)

1. Security: Authentication bypass for EXTERNAL users

2. Crash on EXTERNAL mechanism without valid certificate CN

3. extract_subject_alternative_name / extract_common_name should be private

4. Missing specs

5. Debug logging should be removed

Uh oh!

lukas8219 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

lukas8219 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

carlhoerberg May 10, 2026

Choose a reason for hiding this comment

Uh oh!

lukas8219 May 11, 2026

Choose a reason for hiding this comment

Uh oh!

carlhoerberg May 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukas8219 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Apr 30, 2026

PR Review: EXTERNAL (x509) authentication mechanism

1. not_nil! crash when EXTERNAL is used without a client certificate

2. Security: No verification that EXTERNAL is only used over TLS with a verified client cert

3. Security: EXTERNAL auth bypasses default_user_only_loopback? check

4. Stale TODO comment contradicts implementation

5. Missing specs

Uh oh!

lukas8219 commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukas8219 commented Apr 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlhoerberg May 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: EXTERNAL (x509) authentication mechanism

Uh oh!

lukas8219 commented Jun 6, 2026

Config Keys

X509 Extension Parsing

Design Question

Current Preference

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukas8219 commented Jul 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

lukas8219 commented Apr 30, 2026 •

edited

Loading

3. `extract_subject_alternative_name` / `extract_common_name` should be private

lukas8219 left a comment •

edited

Loading

1. `not_nil!` crash when EXTERNAL is used without a client certificate

3. Security: EXTERNAL auth bypasses `default_user_only_loopback?` check

lukas8219 commented Apr 30, 2026 •

edited

Loading

claude Bot commented Jun 3, 2026 •

edited

Loading