Feature/004 token auth by alxayo · Pull Request #4 · alxayo/rtmp-go

alxayo · 2026-03-03T11:31:22Z

This pull request introduces a comprehensive token-based authentication system to the RTMP server, along with corresponding updates to CLI flags, configuration validation, documentation, and architecture diagrams. The changes ensure authentication can be enforced at the publish/play command level via configurable modes (none, token, file, callback), with support for dynamic reload and external validation. Documentation and code comments have been updated throughout to reflect the new feature.

Defines the implementation plan for Feature 004: pluggable authentication with three validator backends (static tokens, JSON file, HTTP webhook). Includes: - Validator interface design (auth.Validator) - 14 implementation tasks with dependency graph - Server behavior specification (onStatus codes, connection lifecycle) - Integration and interop test plan - CLI flag design (-auth-mode, -auth-token, -auth-file, -auth-callback) - Full code examples for all components

Adds pluggable authentication for publish and play requests with three validator backends: - TokenValidator: in-memory map from CLI flags (-auth-token) - FileValidator: JSON file with SIGHUP reload (-auth-file) - CallbackValidator: HTTP webhook delegation (-auth-callback) New auth package (internal/rtmp/server/auth/): - Validator interface with ValidatePublish/ValidatePlay - AllowAllValidator (default, backward-compatible) - ParseStreamURL: extracts query params from stream names - Sentinel errors: ErrUnauthorized, ErrTokenMissing, ErrTokenExpired Modified existing code: - RPC parsers: publish.go and play.go now parse ?token=xxx from stream names - ConnectCommand: captures Extra fields for auth context - command_integration.go: auth checkpoint before HandlePublish/HandlePlay - server.Config: added AuthValidator field - errors.go: added AuthError type - hooks/events.go: added EventAuthFailed event type - CLI flags: -auth-mode, -auth-token, -auth-file, -auth-callback - main.go: wires auth validator, SIGHUP reload for file mode Documentation updated: - README.md, architecture.md, design.md, implementation.md, getting-started.md - copilot-instructions.md Tests: 19 auth unit tests + updated RPC/errors tests (all 15 packages pass)

- Extract authenticateRequest() helper in command_integration.go, eliminating ~25 lines of duplicated auth checkpoint logic between OnPublish and OnPlay handlers - Remove unused ErrTokenExpired sentinel (no validator returns it) - Remove unused AuthError type from errors.go (validators use auth package sentinel errors instead) - Remove redundant empty-name check in publish.go (ParseStreamURL already handles this case) - Update docs to reflect removed AuthError type

- auth.go: 'Three' → 'Four' validators (AllowAll was missing from count) - auth.go: clarify command_integration.go reference with package context - design.md: same count fix ('Three' → 'Four') - getting-started.md: remove escaped quotes in code block examples

- allow_all_test: add t.Run for proper subtest naming - token_test: replace loop with explicit t.Run(publish/play) subtests - file_test: consolidate 3 rejection tests into table-driven test, add ValidatePlay coverage to valid token test - callback_test: add ValidatePlay coverage alongside ValidatePublish - publish_handler_test: add TestHandlePublishWithQueryParams verifying query params are stripped from registry key - play_handler_test: add TestHandlePlayWithQueryParams verifying stream lookup uses clean key (not raw name with ?token=...)

- docs/definition-of-done.md: comprehensive checklist covering implementation, code quality, tests, documentation, and git hygiene - .github/prompts/post-feature-review.prompt.md: reusable Copilot prompt that runs all optimization/simplification steps from this session - docs/README.md: add DoD to documentation index

- Remove unused decodeArrayValue function from amf/array.go - Fix stale encodeAny comment to reflect Strict Array support - Update registry.go header: remove task reference, reflect current capabilities - Update PublisherDisconnected comment to match actual usage

- Fix copilot-instructions.md link to archived/000-constitution.md - Fix escaped quotes in implementation.md code block - Add token-auth spec to docs/README.md specifications table

Copilot

Pull request overview

Introduces token-based authentication enforcement for RTMP publish/play commands via a pluggable validator interface, wiring it through the server command integration and exposing configuration via CLI flags and documentation updates.

Changes:

Added internal/rtmp/server/auth package with multiple validator backends (allow-all, static token map, JSON file, HTTP callback) and URL query parsing for stream names.
Enforced auth at publish/play time in command integration; updated RPC parsing to strip query params from stream keys while preserving parsed params.
Updated CLI flags, docs, architecture diagrams, and tests to reflect auth support and query-param stream keys.

Reviewed changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
specs/004-token-auth/spec.md	New feature spec describing auth modes, flow, wiring, and test plan.
internal/rtmp/server/server.go	Adds `AuthValidator` to server config.
internal/rtmp/server/registry.go	Updates stream registry header comments.
internal/rtmp/server/publish_handler_test.go	Adds test ensuring query params are stripped from publish stream key.
internal/rtmp/server/publish_handler.go	Updates comments related to publisher disconnect behavior.
internal/rtmp/server/play_handler_test.go	Adds test ensuring query params are stripped for play lookups.
internal/rtmp/server/hooks/events.go	Adds `auth_failed` hook event type.
internal/rtmp/server/command_integration.go	Wires auth checks into publish/play command handling.
internal/rtmp/server/auth/url_test.go	Adds table-driven tests for parsing stream query params.
internal/rtmp/server/auth/url.go	Implements `ParseStreamURL` for stream-name query parsing.
internal/rtmp/server/auth/token_test.go	Adds tests for static token validation behavior.
internal/rtmp/server/auth/token.go	Implements in-memory token validator.
internal/rtmp/server/auth/file_test.go	Adds tests for JSON-file token validator + reload behavior.
internal/rtmp/server/auth/file.go	Implements file-backed token validator with reload support.
internal/rtmp/server/auth/callback_test.go	Adds tests for HTTP callback validator (status codes, timeout, cancel).
internal/rtmp/server/auth/callback.go	Implements HTTP webhook-based validator.
internal/rtmp/server/auth/auth_test.go	Adds tests for sentinel errors and Request zero-value expectations.
internal/rtmp/server/auth/auth.go	Defines `Validator`, `Request`, and sentinel errors with package docs.
internal/rtmp/server/auth/allow_all_test.go	Adds tests for allow-all validator.
internal/rtmp/server/auth/allow_all.go	Implements allow-all validator backend.
internal/rtmp/rpc/publish_test.go	Extends publish parsing tests for query params and stripping.
internal/rtmp/rpc/publish.go	Parses query params from publishing name; adds `QueryParams` to command.
internal/rtmp/rpc/play_test.go	Extends play parsing tests for query params and stripping.
internal/rtmp/rpc/play.go	Parses query params from stream name; adds `QueryParams` to command.
internal/rtmp/rpc/connect_test.go	Adds test verifying extra connect-object fields are captured.
internal/rtmp/rpc/connect.go	Captures unknown connect-object fields into `Extra`.
internal/rtmp/amf/object.go	Updates AMF0 encoder documentation comment.
internal/rtmp/amf/array.go	Updates strict array helper comment (cleanup).
docs/implementation.md	Updates architecture tree and publish flow to include auth check.
docs/getting-started.md	Documents auth modes, examples, and flags table updates.
docs/design.md	Adds design section describing token-based auth approach.
docs/definition-of-done.md	Adds a feature completion checklist document.
docs/architecture.md	Adds auth package row to package map.
docs/README.md	Links definition-of-done and new token-auth spec.
cmd/rtmp-server/main.go	Builds auth validator from flags; wires SIGHUP reload for file mode.
cmd/rtmp-server/flags.go	Adds auth flags and validation for auth configuration.
README.md	Promotes Authentication feature and documents new CLI flags.
.github/prompts/post-feature-review.prompt.md	Adds a post-feature review prompt/checklist for contributors.
.github/copilot-instructions.md	Updates architecture overview to include auth package and path fix.

Copilot · 2026-03-03T11:40:14Z

+func (v *TokenValidator) validate(req *Request) error {
+	token := req.QueryParams["token"]
+	if token == "" {
+		return ErrTokenMissing
+	}
+	expected, exists := v.Tokens[req.StreamKey]
+	if !exists || token != expected {
+		return ErrUnauthorized
+	}


TokenValidator assumes req.QueryParams is non-nil; indexing a nil map will panic. This conflicts with the package's own zero-value Request test/comments and makes validators unsafe if callers pass a nil/zero Request. Please treat nil req / nil QueryParams as "token missing" (or unauthorized) instead of panicking.

Suggested change

func (v *TokenValidator) validate(req *Request) error {

token := req.QueryParams["token"]

if token == "" {

return ErrTokenMissing

}

expected, exists := v.Tokens[req.StreamKey]

if !exists || token != expected {

return ErrUnauthorized

}

func (v *TokenValidator) validate(req *Request) error {

// Treat nil request or nil QueryParams as "token missing" rather than panicking.

if req == nil || req.QueryParams == nil {

return ErrTokenMissing

}

token := req.QueryParams["token"]

if token == "" {

return ErrTokenMissing

}

expected, exists := v.Tokens[req.StreamKey]

if !exists || token != expected {

return ErrUnauthorized

}

Copilot · 2026-03-03T11:40:15Z

+func (v *FileValidator) validate(req *Request) error {
+	token := req.QueryParams["token"]
+	if token == "" {
+		return ErrTokenMissing
+	}
+	v.mu.RLock()
+	expected, exists := v.tokens[req.StreamKey]
+	v.mu.RUnlock()
+	if !exists || token != expected {
+		return ErrUnauthorized


FileValidator.validate indexes req.QueryParams without guarding against nil. A nil map will panic at runtime; validators should be robust to a zero-value/nil Request (as indicated by auth/auth_test.go). Consider handling nil req / nil QueryParams by returning ErrTokenMissing (or another sentinel) instead of panicking.

Copilot · 2026-03-03T11:40:15Z

+		rawName = "default"
 	}
+	parsed := auth.ParseStreamURL(rawName)
+	publishingName := parsed.StreamName


After parsing query params, publishingName can become empty (e.g., rawName="?token=abc"), producing a stream key like "app/". Previously empty names defaulted to "default"; this should probably also apply after query parsing (or be rejected explicitly) to avoid registering/authorizing an empty stream name.

Suggested change

publishingName := parsed.StreamName

publishingName := parsed.StreamName

if publishingName == "" {

publishingName = "default"

}

Copilot · 2026-03-03T11:40:15Z

 	"github.com/alxayo/go-rtmp/internal/errors"
 	"github.com/alxayo/go-rtmp/internal/rtmp/amf"
 	"github.com/alxayo/go-rtmp/internal/rtmp/chunk"
+	"github.com/alxayo/go-rtmp/internal/rtmp/server/auth"
 )


internal/rtmp/rpc now imports internal/rtmp/server/auth for ParseStreamURL, which violates the documented layered protocol stack rule that higher layers should not depend on server-layer packages (see docs/design.md: layered stack; "each layer only depends on the one below it"). To keep layering clean (and avoid future dependency cycles), consider moving ParseStreamURL into rpc (or a lower-level package) and keep auth validators in server/auth.

Copilot · 2026-03-03T11:40:16Z

 	"github.com/alxayo/go-rtmp/internal/errors"
 	"github.com/alxayo/go-rtmp/internal/rtmp/amf"
 	"github.com/alxayo/go-rtmp/internal/rtmp/chunk"
+	"github.com/alxayo/go-rtmp/internal/rtmp/server/auth"
 )


internal/rtmp/rpc now imports internal/rtmp/server/auth for ParseStreamURL, but docs/design.md describes a layered protocol stack where each layer only depends on the one below it. Having rpc depend on server/auth breaks that layering and risks future import cycles. Consider relocating the stream-name query parsing helper into rpc (or another lower-level package) and keeping server/auth limited to validation backends.

Copilot · 2026-03-03T11:40:16Z

+			timeout = 5 * time.Second
+		}
+		return auth.NewCallbackValidator(cfg.authCallbackURL, timeout), nil
+	default: // "none"


buildAuthValidator returns an AllowAllValidator for auth-mode "none", which makes Config.AuthValidator non-nil even when auth is disabled. This conflicts with server.Config's documented behavior (nil means no auth configured) and causes the auth checkpoint/logging to run for every publish/play even in "none" mode. Consider returning nil for "none" and letting the server treat nil as disabled.

Suggested change

default: // "none"

case "none", "":

// No auth configured: return nil so server treats auth as disabled.

return nil, nil

default:

// Fallback to allow-all behavior for any other mode, preserving existing semantics.

Copilot · 2026-03-03T11:40:16Z

 	cfg.relayDestinations = relayDests
 	cfg.hookScripts = hookScripts
 	cfg.hookWebhooks = hookWebhooks
+	cfg.authTokens = authTokens


cfg.authTokens is a []string, but authTokens is a stringSliceFlag (defined type). The assignment cfg.authTokens = authTokens will not compile without an explicit conversion (e.g., cfg.authTokens = []string(authTokens)) or making stringSliceFlag a type alias. (Same pattern applies to other stringSliceFlag assignments in this function.)

Suggested change

cfg.relayDestinations = relayDests

cfg.hookScripts = hookScripts

cfg.hookWebhooks = hookWebhooks

cfg.authTokens = authTokens

cfg.relayDestinations = []string(relayDests)

cfg.hookScripts = []string(hookScripts)

cfg.hookWebhooks = []string(hookWebhooks)

cfg.authTokens = []string(authTokens)

Copilot · 2026-03-03T11:40:16Z

+	body := callbackRequest{
+		Action:     action,
+		App:        req.App,
+		StreamName: req.StreamName,
+		StreamKey:  req.StreamKey,
+		Token:      req.QueryParams["token"],


CallbackValidator.call reads req.QueryParams["token"] without checking for nil, which will panic if QueryParams is nil (or if req itself is nil). Since Request is passed across package boundaries, please make this nil-safe (treat missing QueryParams as token missing).

Suggested change

body := callbackRequest{

Action: action,

App: req.App,

StreamName: req.StreamName,

StreamKey: req.StreamKey,

Token: req.QueryParams["token"],

// Be defensive about nil requests or query params: treat missing token as empty.

if req == nil {

return ErrUnauthorized

}

token := ""

if req.QueryParams != nil {

token = req.QueryParams["token"]

}

body := callbackRequest{

Action: action,

App: req.App,

StreamName: req.StreamName,

StreamKey: req.StreamKey,

Token: token,

Copilot · 2026-03-03T11:40:17Z

+	statusCode := "NetStream." + strings.ToUpper(action[:1]) + action[1:] + ".Unauthorized"
+	errStatus, _ := buildOnStatus(msg.MessageStreamID, streamKey, statusCode, "Authentication failed.")
+	_ = c.SendMessage(errStatus)


authenticateRequest builds an Unauthorized onStatus using buildOnStatus(), but buildOnStatus currently hard-codes info["level"] = "status". For auth failures, the RTMP onStatus level should be "error" (and the spec/docs in this PR describe it as an error). Consider extending buildOnStatus to accept a level (status/error) or adding a dedicated builder for error responses.

Copilot · 2026-03-03T11:40:17Z

+// Sentinel errors for authentication failures.
+var (
+    ErrUnauthorized = errors.New("authentication failed: invalid credentials")
+    ErrTokenMissing = errors.New("authentication failed: token missing")
+    ErrTokenExpired = errors.New("authentication failed: token expired")
+    ErrForbidden    = errors.New("authentication failed: access denied")
+)


The spec's T001 code snippet defines an ErrTokenExpired sentinel, but the implemented auth package (internal/rtmp/server/auth/auth.go) only defines ErrUnauthorized, ErrTokenMissing, and ErrForbidden. Either add ErrTokenExpired (and document when it's returned) or remove it from the spec to avoid drifting documentation.

…dlock Wire the disconnect handler in attachCommandHandling to perform full cleanup when a connection's readLoop exits: - Stop MediaLogger (prevents goroutine + ticker leak per connection) - Close FLV recorder under stream.mu lock (safe with cleanupAllRecorders) - Call PublisherDisconnected() to clear publisher reference (fixes stream key lockout where stale publishers block reuse via ErrPublisherExists) - Call SubscriberDisconnected() to remove dead subscribers - Call RemoveConnection() to remove from s.conns (fixes memory leak where connections accumulated forever, only cleared on server shutdown) - Fire publish_stop/play_stop/connection_close hook events Add role field to commandState (set to 'publisher'/'subscriber' by OnPublish/OnPlay handlers) to determine cleanup path. Add RemoveConnection(id) method to Server for single-connection removal. Fix deadlock in Stop(): previously held s.mu.Lock() while calling c.Close() -> wg.Wait(), but the disconnect handler's RemoveConnection() also acquires s.mu.Lock(). Fix: collect connections, clear map, release lock, then close connections outside the lock. Resolves issues #1 (conn memory leak), #2 (stale publishers), #3 (stale subscribers), #4 (MediaLogger leak), #6 (unclosed recorders) from spec 005.

alxayo added 10 commits March 3, 2026 10:22

style: run gofmt on all auth-related files

0d00d3c

fix: rename deprecated 'mode' to 'agent' in prompt frontmatter

9251a2a

docs: fix broken link, escaped quotes, and add spec reference

f92d34d

- Fix copilot-instructions.md link to archived/000-constitution.md - Fix escaped quotes in implementation.md code block - Add token-auth spec to docs/README.md specifications table

alxayo requested a review from Copilot March 3, 2026 11:31

Copilot started reviewing on behalf of alxayo March 3, 2026 11:32 View session

alxayo merged commit c10bf98 into main Mar 3, 2026
3 checks passed

Copilot AI reviewed Mar 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/004 token auth#4

Feature/004 token auth#4
alxayo merged 10 commits intomainfrom
feature/004-token-auth

alxayo commented Mar 3, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-	default: // "none"
+	case "none", "":
+		// No auth configured: return nil so server treats auth as disabled.
+		return nil, nil
+	default:
+		// Fallback to allow-all behavior for any other mode, preserving existing semantics.

Conversation

alxayo commented Mar 3, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants