TOTP DTMF Authentication#1057
Open
SigmaScott wants to merge 23 commits into
Open
Conversation
Adds an optional RFC 6238 TOTP authentication mechanism that lets a user
unlock an additional, privileged DTMF command-set stanza for a sliding
session timeout, then drop back to the default command set on logout or
expiry.
Why
---
Until now every DTMF function in rpt.conf was reachable by anyone who
could key the radio. There was no way to gate privileged actions
(cop subcommands, shell-out cmd entries, link control) behind any kind
of per-user credential. This change introduces a minimal, self-contained
auth layer that uses a standard smartphone TOTP app (Google
Authenticator, Authy, FreeOTP, etc.) as the second factor, with no new
external dependencies (uses Asterisk's built-in SHA1 primitive).
Wire format
-----------
Admin picks a DTMF prefix and binds it to the new 'auth' function:
*A<id4><otp6> login (4-digit user-id + 6-digit TOTP, no separator)
*A* logout
*A status (courtesy tone only; details via CLI)
Behaviour
---------
- Per-node single session slot; second login displaces the first.
- Sliding timeout refreshed on every successful authed function dispatch.
- Authed-stanza walk runs FIRST in collect_function_digits(); on prefix
collision the authed entry wins (admin's responsibility to avoid).
- All four failure modes (bad OTP / locked out / feature disabled /
malformed input) return identical DC_ERROR so a caller cannot
enumerate user state by listening to courtesy/error tones. Detail
appears only in the Asterisk log and via the new CLI commands.
- Configurable lockout threshold and duration; threshold=0 disables.
- TOTP step and skew window are runtime-configurable per node so admins
can tune for clock drift on field-deployed repeaters.
Security model
--------------
Shared secrets live in a separate file (path set per-node via
'auth_users') so they do not leak through the widely-shared rpt.conf.
The file is plain base32 protected by filesystem permissions
(0640 root:asterisk recommended). OTP digits are never logged; only
user-id, result code, and reason are. Constant-time OTP comparison was
considered and deliberately not added: the DTMF channel makes the
timing side-channel non-exploitable in practice (documented inline).
New files
---------
apps/app_rpt/rpt_totp.{c,h} RFC 6238 + HMAC-SHA1 + base32
apps/app_rpt/rpt_auth.{c,h} session/lockout/file-load layer
configs/samples/rpt_auth.conf.sample documented user-secret file
adminauth.md admin & developer guide
Modified files
--------------
apps/app_rpt/app_rpt.h 6 config fields + opaque auth
state pointer on struct rpt
apps/app_rpt/rpt_config.c register auth_* keys; reload/free
hooks at lock-release / teardown
apps/app_rpt/rpt_functions.{h,c} new function_auth top-level DTMF
handler (Option A: not a cop
sub-action, for discoverability
and clean future growth)
apps/app_rpt.c function_table[] registration;
collect_function_digits() now
walks authed stanza first,
extends the longestfunc gate by
the authed stanza length, and
calls rpt_auth_touch() on every
successful dispatch (sliding
timeout)
apps/app_rpt/rpt_cli.c 'rpt auth show <node>' and
'rpt auth logout <node>' with
node-name tab completion
configs/rpt/rpt.conf documented auth_* keys + worked
example showing prefix wiring
and a [functions-admin] stanza
Build
-----
The existing apps/Makefile.diff uses $(wildcard app_rpt/rpt_*.c) so
the two new compilation units are picked up automatically; no Makefile
change required.
Verification
------------
See adminauth.md for an end-to-end smoke-test recipe (generate a
secret, render a QR code with qrencode, enrol on a phone authenticator,
drive logins via 'rpt fun <node> ...' and inspect via 'rpt auth show
<node>'). RFC 6238 Appendix B test vectors are validated by the
RPT_TOTP_SELFTEST block in rpt_totp.c (build with -DRPT_TOTP_SELFTEST
to enable).
app_rpt.h uses AST_MAX_EXTENSION (declared in asterisk/channel.h) in
struct rpt. rpt_auth.c was the only new translation unit that included
app_rpt.h without first pulling in channel.h, so it failed to build
against asterisk-22.x:
app_rpt/app_rpt.h:1004:20: error: 'AST_MAX_EXTENSION' undeclared
char exten[AST_MAX_EXTENSION];
Other rpt_*.c files that consume app_rpt.h (e.g. rpt_config.c) already
include asterisk/channel.h. Match that pattern.
The asterisk/sha1.h API is no longer exported by modern Asterisk, causing an undefined symbol error (SHA1Result) at module load time. Switch to OpenSSL's SHA1 API which Asterisk already depends on and link app_rpt against -lcrypto.
…mpletes When digits arrive one-at-a-time through the macro buffer, function_auth matched on the bare 'A' prefix and returned DC_COMPLETEQUIET with an empty digit buffer, treating it as a status query. The remaining 10 digits were then orphaned. Return DC_INDETERMINATE for partial input (len 0 and 1-9) so the framework keeps collecting until the full 10-digit login payload (4-digit user-id + 6-digit OTP) or the single '*' logout arrives.
Replace the single A=auth prefix with three config entries using the param field to select the sub-command: A1 = auth,a (login — collects 10 trailing digits) A2 = auth,s (status — fires immediately) A3 = auth,l (logout — fires immediately) This avoids the single-character prefix matching problem where the framework fired function_auth on the first digit before the full login payload arrived. Each sub-command now has a distinct multi-char prefix so collect_function_digits waits for the complete prefix. Update adminauth.md to reflect the new wire format, config, usage examples, and developer documentation.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds per-user TOTP authentication over DTMF with RFC‑6238 verification, per-node single-session lifecycle (touch/expiry/lockout/reload), authed-first privileged-stanza dispatch, DTMF login/logout/status handler, CLI controls, config and build wiring, and documentation/sample configs. ChangesPer-user TOTP authentication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/app_rpt.c (1)
1651-1715:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake auth resolution and privileged dispatch atomic.
This authorizes the command by selecting the authenticated stanza, then executes the resolved handler later with no lock held. A concurrent
rpt auth logout <node>can clear the session after the stanza match and before Line 1715, so one privileged command can still slip through after logout/expiry. Please move the auth decision into an API that also guards dispatch, or revalidate against an auth generation/token immediately before invoking the handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/app_rpt.c` around lines 1651 - 1715, The code currently selects authed_stanza via rpt_auth_active_set(myrpt) and later calls the resolved handler (*function_table[i].function) without holding the auth lock, allowing a concurrent rpt auth logout to invalidate the session between match and dispatch; fix by making auth resolution and dispatch atomic: either add or use an API that both resolves the correct stanza and invokes the handler under the same auth guard (e.g. create rpt_auth_validate_and_dispatch(myrpt, vp->name, handler, param, functiondigits, command_source, mylink) or extend rpt_auth_active_set to return a token/generation and then revalidate that token immediately before calling rpt_function_lookup()/function_table[i].function), ensuring you reference authed_stanza/rpt_auth_active_set, vp->name, rpt_function_lookup, function_table[i].function and rpt_auth_longestfunc when implementing the guarded dispatch or token revalidation.
🧹 Nitpick comments (1)
apps/app_rpt/rpt_totp.h (1)
4-5: ⚡ Quick winUpdate the dependency contract in this header.
These lines still say the implementation uses
asterisk/sha1.hand has no OpenSSL dependency, butrpt_totp.cnow includes<openssl/sha.h>and this feature links-lcrypto. Leaving this stale will mislead maintainers and packagers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/app_rpt/rpt_totp.h` around lines 4 - 5, Update the header comment in rpt_totp.h to reflect the current dependency contract: replace the claim that the implementation only depends on Asterisk's <asterisk/sha1.h> and standard C with a statement that rpt_totp.c now includes <openssl/sha.h> and the feature requires linking against OpenSSL libcrypto (e.g., -lcrypto); ensure the comment mentions both the OpenSSL header and the link-time dependency so packagers and maintainers are not misled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adminauth.md`:
- Around line 293-296: Update the documentation in the rpt_totp.c section to
reflect the current implementation: replace the paragraph that claims Asterisk's
asterisk/sha1.h was used and OpenSSL avoided with a clear statement that
OpenSSL/libcrypto is used (per Makefile and commits) because Asterisk no longer
exports asterisk/sha1.h; mention rpt_totp.c and the Makefile as the sources of
truth and remove the outdated rationale about avoiding external deps.
- Line 276: Update the stale documentation in adminauth.md for "Design goal `#9`"
to remove the claim "No OpenSSL, no libcrypt" and explicitly state that the
project depends on OpenSSL's libcrypto for SHA1 (reference the Makefile linking
against -lcrypto and the change from asterisk/sha1.h to OpenSSL SHA1 in the
commit history); mention that asterisk/sha1.h is no longer exported so the
implementation uses OpenSSL SHA1 and ensure the rationale and current dependency
are clearly documented.
In `@apps/app_rpt.c`:
- Around line 1718-1721: The code currently calls rpt_auth_touch(myrpt) for any
successful dispatch (rv == DC_COMPLETE || rv == DC_COMPLETEQUIET || rv ==
DC_DOKEY) which extends auth sessions for public commands; change the condition
so rpt_auth_touch(myrpt) is only invoked when the dispatch came from an
authenticated/privileged source (i.e., the match was from the auth-specific
stanza or an authenticated session) rather than the normal source stanza—keep
the rv checks (DC_COMPLETE, DC_COMPLETEQUIET, DC_DOKEY) but add a guard that
verifies the dispatch context indicates an authenticated/privileged match (e.g.,
check the dispatch/source/auth-session flag used in your dispatch code) before
calling rpt_auth_touch(myrpt).
In `@apps/app_rpt/rpt_auth.c`:
- Around line 449-452: The log reads u->command_set after calling
rpt_mutex_unlock(&myrpt->lock), which can race with
rpt_auth_reload()/rpt_auth_free() and dereference freed memory; fix by capturing
the command_set string while still holding the lock (e.g., strdup/copy into a
local buffer) or move the ast_log call to before rpt_mutex_unlock, then use that
local copy (and free it after unlocking) when calling ast_log so you never
access u->command_set after releasing myrpt->lock.
- Around line 334-352: rpt_auth_active_set() currently returns a raw pointer
into st->session_command_set after releasing myrpt->lock, creating a
use-after-free risk when rpt_auth_logout()/rpt_auth_reload() free it; change
rpt_auth_active_set() to return a caller-owned copy (e.g., strdup of
st->session_command_set) or accept a caller-provided buffer to copy into, and
update callers such as collect_function_digits() to free the returned string (or
supply a buffer) accordingly; keep the locking and expiry checks (use
clear_session_locked()) but ensure any pointer returned is heap-allocated and
documented as caller-owned so no dereference occurs after the lock is released.
In `@apps/app_rpt/rpt_config.c`:
- Line 680: The code currently frees the live authentication state (myrpt->auth)
during reload (via rpt_free_config_vars()/rpt_auth_free()) then recreates it
with rpt_auth_reload(), which logs out the active operator and clears lockout
data; instead, change load_rpt_vars() so it does NOT call rpt_auth_free() or let
rpt_free_config_vars() free myrpt->auth during a normal reload—instead update
the existing auth state in place by calling rpt_auth_reload() against
myrpt->auth (or add a flag to rpt_free_config_vars() to preserve auth on
reload); keep rpt_auth_free() only for final teardown. Target symbols:
load_rpt_vars, rpt_free_config_vars, rpt_auth_reload, rpt_auth_free,
myrpt->auth.
In `@apps/app_rpt/rpt_functions.c`:
- Around line 2037-2040: The status subcommand currently always reports COMPLETE
and returns DC_COMPLETEQUIET without checking authentication; update the branch
handling subcmd "s" to consult the auth layer before emitting telemetry and
returning. Specifically, call the appropriate auth check (e.g.,
auth_session_is_authenticated(command_source) or rpt_auth_get_status(myrpt,
command_source)) and if authenticated invoke rpt_telem_select(...) and
rpt_telemetry(...) with the success status and return DC_COMPLETEQUIET,
otherwise emit a telemetry status for auth failure (e.g., rpt_telemetry(myrpt,
AUTH_REQUIRED, NULL) or similar) and return the correct error code (e.g.,
DC_DENIED or DC_AUTHFAIL). Ensure you reference and use the existing symbols
rpt_telem_select, rpt_telemetry, myrpt, and command_source when inserting the
auth check and conditional behavior.
- Around line 2031-2033: The current check uses myrpt->remote to reject auth,
which is incorrect because non-local command sources (link/phone/EchoLink/TLB)
still arrive here; change the gate to reject based on the actual command source
instead: replace the myrpt->remote check with a condition that inspects
command_source (and mylink where appropriate) so only truly local sources are
allowed to authenticate; adjust the logic in the login/logout/status handlers in
rpt_functions.c to use command_source (and test mylink for linked callers)
before returning DC_ERROR for non-local auth attempts.
In `@apps/app_rpt/rpt_totp.c`:
- Around line 75-81: In rpt_base32_decode, stop accepting input at the first '='
only if the rest of the input contains nothing but padding; currently the loop
breaks on '=' and returns success even when trailing garbage follows (e.g.,
"JBSWY3DP=garbage"). Change the logic inside the loop (the for over p and the
branch that checks c == '=') so that when you hit '=', you scan the remainder of
in from p to the terminator and if any character other than '=' is found you
return failure (use the function's existing error return path), otherwise treat
the trailing '='s as valid padding and proceed; ensure you reference and update
the same control flow used around p/in in rpt_base32_decode so callers keep the
same semantics on valid input.
In `@configs/rpt/rpt.conf`:
- Around line 519-550: The example in configs/rpt/rpt.conf is inconsistent with
function_auth() behavior: change the single wildcard entry "*A = auth," into
three explicit function-table entries (e.g., "A1 = auth,a", "A2 = auth,s", "A3 =
auth,l") and update the wire-format examples to match adminauth.md and
apps/app_rpt/rpt_functions.c (i.e., "*A1<id4><otp6>" for login, "*A2" for
status, "*A3" for logout) so that function_auth() correctly dispatches login
(param "a"), status (param "s"), and logout (param "l").
---
Outside diff comments:
In `@apps/app_rpt.c`:
- Around line 1651-1715: The code currently selects authed_stanza via
rpt_auth_active_set(myrpt) and later calls the resolved handler
(*function_table[i].function) without holding the auth lock, allowing a
concurrent rpt auth logout to invalidate the session between match and dispatch;
fix by making auth resolution and dispatch atomic: either add or use an API that
both resolves the correct stanza and invokes the handler under the same auth
guard (e.g. create rpt_auth_validate_and_dispatch(myrpt, vp->name, handler,
param, functiondigits, command_source, mylink) or extend rpt_auth_active_set to
return a token/generation and then revalidate that token immediately before
calling rpt_function_lookup()/function_table[i].function), ensuring you
reference authed_stanza/rpt_auth_active_set, vp->name, rpt_function_lookup,
function_table[i].function and rpt_auth_longestfunc when implementing the
guarded dispatch or token revalidation.
---
Nitpick comments:
In `@apps/app_rpt/rpt_totp.h`:
- Around line 4-5: Update the header comment in rpt_totp.h to reflect the
current dependency contract: replace the claim that the implementation only
depends on Asterisk's <asterisk/sha1.h> and standard C with a statement that
rpt_totp.c now includes <openssl/sha.h> and the feature requires linking against
OpenSSL libcrypto (e.g., -lcrypto); ensure the comment mentions both the OpenSSL
header and the link-time dependency so packagers and maintainers are not misled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84e2bcec-8a0c-4598-a977-80156dfb8f08
📒 Files selected for processing (14)
adminauth.mdapps/Makefile.diffapps/app_rpt.capps/app_rpt/app_rpt.happs/app_rpt/rpt_auth.capps/app_rpt/rpt_auth.happs/app_rpt/rpt_cli.capps/app_rpt/rpt_config.capps/app_rpt/rpt_functions.capps/app_rpt/rpt_functions.happs/app_rpt/rpt_totp.capps/app_rpt/rpt_totp.hconfigs/rpt/rpt.confconfigs/samples/rpt_auth.conf.sample
| myrpt->p.ldisc_buf = NULL; | ||
| } | ||
|
|
||
| rpt_auth_free(myrpt); |
Contributor
There was a problem hiding this comment.
Don't tear down live auth state on every reload.
load_rpt_vars() now frees myrpt->auth through rpt_free_config_vars() and then recreates it with rpt_auth_reload(). That logs out the active operator and clears any in-flight lockout whenever rpt.conf is reloaded, which undermines the new session and brute-force protections during normal reloads. Keep rpt_auth_free() for final teardown, and let reload update the existing auth state in place.
Also applies to: 1331-1331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/app_rpt/rpt_config.c` at line 680, The code currently frees the live
authentication state (myrpt->auth) during reload (via
rpt_free_config_vars()/rpt_auth_free()) then recreates it with
rpt_auth_reload(), which logs out the active operator and clears lockout data;
instead, change load_rpt_vars() so it does NOT call rpt_auth_free() or let
rpt_free_config_vars() free myrpt->auth during a normal reload—instead update
the existing auth state in place by calling rpt_auth_reload() against
myrpt->auth (or add a flag to rpt_free_config_vars() to preserve auth on
reload); keep rpt_auth_free() only for final teardown. Target symbols:
load_rpt_vars, rpt_free_config_vars, rpt_auth_reload, rpt_auth_free,
myrpt->auth.
Author
There was a problem hiding this comment.
Yes a user would be de-authenticated if the app_rpt was reloaded. Reloading apt_rpt is an infrequent activity on a stable system and this behavior was design decision to require a user to re-auth if this did happen.
Contributor
There was a problem hiding this comment.
@SigmaScott, understood — that's a reasonable design choice. Clearing the session on reload provides a clean slate and avoids stale session state after a configuration change, at the cost of requiring the operator to re-authenticate. Acknowledged.
✏️ Learnings added
Learnt from: SigmaScott
Repo: AllStarLink/app_rpt PR: 1057
File: apps/app_rpt/rpt_config.c:680-680
Timestamp: 2026-05-28T19:43:49.756Z
Learning: In AllStarLink/app_rpt PR `#1057`, de-authenticating the active TOTP session (and clearing lockout state) on `app_rpt` reload via `rpt_free_config_vars()` → `rpt_auth_free()` is an **intentional design decision**. The rationale is that `app_rpt` reloads are infrequent on stable systems, and requiring a user to re-authenticate after a reload is acceptable and preferred to carrying over stale session state. Do not flag this as a bug or suggest preserving auth state across reloads.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mkmer
Repo: AllStarLink/app_rpt PR: 929
File: res/res_rpt_http_registrations.c:219-240
Timestamp: 2026-02-05T14:40:39.509Z
Learning: In the AllStarLink/app_rpt codebase, ensure return values from ast_str_set and ast_str_append are checked and errors are handled or propagated. Do not assume success; handle failures to avoid silent partial strings. Apply this guidance broadly across all C files in the repository (e.g., res/**/*.c) where these functions are used.
Learnt from: mkmer
Repo: AllStarLink/app_rpt PR: 964
File: apps/app_rpt/rpt_bridging.c:229-229
Timestamp: 2026-03-04T15:24:06.975Z
Learning: Guideline: In the AllStarLink/app_rpt codebase, preserve blank CallerID (empty string "") for internal repeater Rx/Tx infrastructure calls, specifically in rpt_bridging.c's __rpt_request when passing the node argument. Only outbound link call sites (e.g., in rpt_link.c and rpt_link_connect) should supply a meaningful CallerID (such as l->name) to identify the remote node. This helps distinguish internal channels from external links. When auditing or modifying bridging and link code, ensure internal calls continue to pass "" for node, and validate that only outbound link paths populate CallerID. This pattern should apply to all relevant .c files within apps/app_rpt.
… example) - rpt_auth.c: move ast_log before unlock in rpt_auth_login() to prevent use-after-free of u->command_set on concurrent reload - rpt_totp.h: update header comment to reflect OpenSSL dependency - adminauth.md: update design goal AllStarLink#9 and §2.3 to reflect OpenSSL/libcrypto usage (asterisk/sha1.h is no longer exported) - rpt.conf: fix auth example to use three explicit function table entries (A1=auth,a / A2=auth,s / A3=auth,l) matching function_auth() dispatch Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Allan-N
reviewed
May 28, 2026
…ecode When '=' is encountered, validate that only '=' characters follow. Previously, input like "JBSWY3DP=garbage" would silently succeed; now it returns -1 (error) for strict RFC 4648 conformance.
rpt_auth_active_set() previously returned a raw pointer into auth state after releasing myrpt->lock. The CLI 'rpt auth logout' handler runs on a separate thread and could free session_command_set between the unlock and the caller's ast_variable_browse() dereference. Change the API to copy the stanza name into a caller-provided buffer while still holding the lock, matching the existing function_table_name pattern in collect_function_digits().
Contributor
|
Actionable comments posted: 0 |
Allan-N
reviewed
May 28, 2026
Also simplify parse_user_value() using strsep/ast_strip/ast_strdup instead of manual pointer arithmetic. Same semantics, 21 fewer lines.
Collaborator
|
Most of these errors can be "fixed" locally by running the pre-commit hook scripts (and activating them while working). Instructions are in the readme. |
…AC API Replaces manual ipad/opad HMAC construction using deprecated SHA1_Init/SHA1_Update/SHA1_Final with the modern EVP_MAC interface. OpenSSL handles HMAC internals; no deprecation warnings on OpenSSL 3.x.
…list The user table is now allocated to exactly the number of entries in rpt_auth.conf, eliminating the hard RPT_AUTH_MAX_USERS cap and avoiding unused memory allocation for typical deployments with few users.
- adminauth.md: replace stale references to openssl/sha.h and manual HMAC construction with accurate EVP_MAC API description - rpt_auth.conf.sample: remove 64-user cap note (now dynamic)
Allan-N
reviewed
May 30, 2026
| ; | ||
| ;auth_users = /etc/asterisk/rpt_auth.conf ; Path to user/secret file. Unset = feature disabled. | ||
| ;auth_timeout = 300 ; Sliding session timeout, seconds. Range 30..86400, default 300. | ||
| ;auth_lockout_threshold = 5 ; Failed-login attempts before lockout. 0 disables lockout. Range 0..1000. |
Collaborator
There was a problem hiding this comment.
What's the default value?
Author
There was a problem hiding this comment.
5 is the default. I will update this.
|
|
||
| [functions](functions-main) | ||
|
|
||
| ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; |
Collaborator
There was a problem hiding this comment.
I'm thinking that this block may be a bit too much documentation for the rpt.conf file. Seems like the ASL Manual would be a better place for the details. Should we limit the rpt.conf content to just :
; Enable/disable functions requiring authentication
; A1 = auth,a ; login: *A1<id4><otp6> e.g. *A11234849203
; A2 = auth,s ; status: *A2 (courtesy tone only; details via "rpt auth show <node>")
; A3 = auth,l ; logout: *A3
Also, do these function definitions belong a few lines up (in [node-main](!))?
| int i, rv; | ||
| char *stringp, *action, *param, *functiondigits; | ||
| char function_table_name[30] = ""; | ||
| char authed_stanza_buf[30] = ""; |
| return DC_ERROR; | ||
| } | ||
| for (i = 0; i < 10; i++) { | ||
| if (digits[i] < '0' || digits[i] > '9') { |
Collaborator
There was a problem hiding this comment.
Could this be :
if (!isdigit(digits[i])) {
| return 0; | ||
| } | ||
| for (i = 0; i < RPT_AUTH_USER_ID_LEN; i++) { | ||
| if (name[i] < '0' || name[i] > '9') { |
- Replace manual '0'-'9' range checks with isdigit() in rpt_totp.c, rpt_auth.c, and rpt_functions.c for consistency with rest of codebase - Add #include <ctype.h> to files using isdigit() - Rename authed_stanza_buf to authed_function_table_name in collect_function_digits() for consistency with function_table_name
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/app_rpt/rpt_totp.c (3)
235-254:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe window self-test is generating the current-step OTP, not the previous-step one.
At
now = 59with a 30-second step, the active counter is1, sototp_at(..., 1)should pass even withwindow=0. This assertion only makes sense if you generate counter0here.Suggested fix
- prev_step_otp = (keylen > 0) ? totp_at(key, (size_t) keylen, 1) : -1; + prev_step_otp = (keylen > 0) ? totp_at(key, (size_t) keylen, 0) : -1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/app_rpt/rpt_totp.c` around lines 235 - 254, The self-test is generating the current-step OTP (using totp_at(..., 1)) but then expects behavior for the previous-step OTP; change the counter passed to totp_at to 0 so prev_step_otp represents the prior time-step, i.e. replace totp_at(key, (size_t) keylen, 1) with totp_at(key, (size_t) keylen, 0) when computing prev_step_otp used by rpt_totp_verify(secret, buf, &last, 59, 30, ...).
92-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject non-zero trailing bits in the last Base32 quantum.
This decoder still accepts malformed secrets like
AB======by emitting the same bytes asAA======and ignoring the leftover non-zero bits. For auth secrets, that should fail closed.Suggested fix
} + if (bits > 0 && (buf & (((size_t)1 << bits) - 1)) != 0) { + return -1; + } + return (int) written;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/app_rpt/rpt_totp.c` around lines 92 - 103, The decoder currently ignores leftover non-zero bits in the final Base32 quantum (variables buf and bits), allowing malformed inputs like "AB======"; modify the end of the decode routine in rpt_totp.c so that after the loop completes you check if bits > 0 and that the low (bits) bits of buf are zero, and if not return an error (e.g., -1) instead of accepting the input; update any callers that rely on the return value if necessary and keep the existing behavior for perfectly aligned inputs where bits == 0 (use the variables buf, bits, written, out, outlen, v to locate the logic).
155-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSecurely clear decoded TOTP secret (
key) on all exit paths inrpt_totp_verify.
rpt_totp_verifyreturnsRPT_TOTP_BAD_SECRETimmediately afterkeylen = rpt_base32_decode(...)whenkeylen <= 0(lines ~155-158), without wiping the stack bufferuint8_t key[64]. Sincerpt_base32_decodecan write partial output tokeybefore encountering an error, this leaves sensitive material uncleared. Unify cleanup so every return zeroizeskey, and replace the current plainmemset(key, 0, sizeof(key))usage with a guaranteed zeroization primitive (e.g.,memset_s/explicit_bzero/OPENSSL_cleanse) or a local secure-wipe wrapper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/app_rpt/rpt_totp.c` around lines 155 - 190, The function rpt_totp_verify leaves partially-decoded secret bytes in the local uint8_t key[64] on some early returns (notably after rpt_base32_decode when keylen <= 0); ensure key is securely zeroed on every exit: replace direct memset(key, 0, ...) with a guaranteed zeroization primitive (explicit_bzero / OPENSSL_cleanse / memset_s or a local secure_wipe wrapper) and either call that secure_wipe before each return (including the RPT_TOTP_BAD_SECRET return immediately after rpt_base32_decode and the RPT_TOTP_BAD_PARAM/RPT_TOTP_REPLAY/RPT_TOTP_OK returns inside the loop) or centralize cleanup via a single exit label (e.g., goto cleanup) that invokes secure_wipe(key, sizeof key) then returns; reference the local buffer key, the rpt_base32_decode call, the totp_at call, and the existing memset usages to locate all return paths to update.apps/app_rpt/rpt_auth.c (1)
347-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpire stale sessions in every accessor, not just
rpt_auth_get_active_stanza().Right now timeout cleanup only happens in
rpt_auth_get_active_stanza(). After the idle window passes,rpt_auth_longestfunc()still returns the cached privileged length andrpt_auth_touch()can extend the same stale slot instead of expiring it first. Per the stack context,collect_function_digits()usesrpt_auth_longestfunc()in its digit gate, so a timed-out auth session can keep affecting command collection after it should be gone.A small shared
expire_session_if_needed_locked()helper used byget_active_stanza,touch,longestfunc, andstatuswould keep the auth state consistent.🛠️ Possible fix
+static int expire_session_if_needed_locked(struct rpt *myrpt, struct rpt_auth_state *st, time_t now) +{ + if (!st || !st->session_active || now < st->session_expires) { + return 0; + } + + ast_log(LOG_NOTICE, "rpt_auth: session for user %s expired on node %s\n", + st->session_user, myrpt->name); + clear_session_locked(st); + return 1; +} + int rpt_auth_longestfunc(struct rpt *myrpt) { struct rpt_auth_state *st; int ret = 0; + time_t now = time(NULL); rpt_mutex_lock(&myrpt->lock); st = myrpt->auth; - if (st && st->session_active) { + if (st && !expire_session_if_needed_locked(myrpt, st, now) && st->session_active) { ret = st->session_longestfunc; } rpt_mutex_unlock(&myrpt->lock); return ret; } void rpt_auth_touch(struct rpt *myrpt) { struct rpt_auth_state *st; time_t now = time(NULL); rpt_mutex_lock(&myrpt->lock); st = myrpt->auth; - if (st && st->session_active) { + if (st && !expire_session_if_needed_locked(myrpt, st, now) && st->session_active) { st->session_expires = now + effective_timeout(myrpt); } rpt_mutex_unlock(&myrpt->lock); }Also applies to: 360-370, 463-485
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/app_rpt/rpt_auth.c` around lines 347 - 357, Add a shared helper expire_session_if_needed_locked(struct rpt *myrpt, struct rpt_auth_state *st) that must be called while holding myrpt->lock: it should check current time vs st->session_expires and, if expired, clear/disable the session (set st->session_active false and reset any cached privileged fields used by rpt_auth_longestfunc/status). Replace the timeout-checking logic in rpt_auth_get_active_stanza, rpt_auth_touch, rpt_auth_longestfunc, and the status path to call expire_session_if_needed_locked(...) while locked before using/refreshing st so stale sessions are always expired first and cannot be extended or read. Ensure rpt_auth_touch still updates session_expires only when session_active remains true after the expire check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/rpt/rpt.conf`:
- Around line 472-481: The stanza names in this template ([totp-admin] and
[totp-operator]) do not match the shipped auth examples (which use
functions-admin / functions-controlop / functions-diag); rename the sections in
this file to use the exact stanza names from rpt_auth.conf (e.g., change
[totp-admin] -> [functions-admin] and [totp-operator] -> [functions-controlop])
so authenticated users map to the correct command-set stanzas, and ensure any
commented command entries (e.g., the cop lines) remain under the updated stanza
names.
---
Outside diff comments:
In `@apps/app_rpt/rpt_auth.c`:
- Around line 347-357: Add a shared helper
expire_session_if_needed_locked(struct rpt *myrpt, struct rpt_auth_state *st)
that must be called while holding myrpt->lock: it should check current time vs
st->session_expires and, if expired, clear/disable the session (set
st->session_active false and reset any cached privileged fields used by
rpt_auth_longestfunc/status). Replace the timeout-checking logic in
rpt_auth_get_active_stanza, rpt_auth_touch, rpt_auth_longestfunc, and the status
path to call expire_session_if_needed_locked(...) while locked before
using/refreshing st so stale sessions are always expired first and cannot be
extended or read. Ensure rpt_auth_touch still updates session_expires only when
session_active remains true after the expire check.
In `@apps/app_rpt/rpt_totp.c`:
- Around line 235-254: The self-test is generating the current-step OTP (using
totp_at(..., 1)) but then expects behavior for the previous-step OTP; change the
counter passed to totp_at to 0 so prev_step_otp represents the prior time-step,
i.e. replace totp_at(key, (size_t) keylen, 1) with totp_at(key, (size_t) keylen,
0) when computing prev_step_otp used by rpt_totp_verify(secret, buf, &last, 59,
30, ...).
- Around line 92-103: The decoder currently ignores leftover non-zero bits in
the final Base32 quantum (variables buf and bits), allowing malformed inputs
like "AB======"; modify the end of the decode routine in rpt_totp.c so that
after the loop completes you check if bits > 0 and that the low (bits) bits of
buf are zero, and if not return an error (e.g., -1) instead of accepting the
input; update any callers that rely on the return value if necessary and keep
the existing behavior for perfectly aligned inputs where bits == 0 (use the
variables buf, bits, written, out, outlen, v to locate the logic).
- Around line 155-190: The function rpt_totp_verify leaves partially-decoded
secret bytes in the local uint8_t key[64] on some early returns (notably after
rpt_base32_decode when keylen <= 0); ensure key is securely zeroed on every
exit: replace direct memset(key, 0, ...) with a guaranteed zeroization primitive
(explicit_bzero / OPENSSL_cleanse / memset_s or a local secure_wipe wrapper) and
either call that secure_wipe before each return (including the
RPT_TOTP_BAD_SECRET return immediately after rpt_base32_decode and the
RPT_TOTP_BAD_PARAM/RPT_TOTP_REPLAY/RPT_TOTP_OK returns inside the loop) or
centralize cleanup via a single exit label (e.g., goto cleanup) that invokes
secure_wipe(key, sizeof key) then returns; reference the local buffer key, the
rpt_base32_decode call, the totp_at call, and the existing memset usages to
locate all return paths to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b929b9dd-96ee-42f1-9406-a427a55702ee
📒 Files selected for processing (12)
.gitignoreadminauth.mdapps/app_rpt.capps/app_rpt/app_rpt.happs/app_rpt/rpt_auth.capps/app_rpt/rpt_auth.happs/app_rpt/rpt_functions.capps/app_rpt/rpt_totp.capps/app_rpt/rpt_totp.hconfigs/rpt/rpt.confconfigs/rpt/rpt_auth.confconfigs/samples/rpt_auth.conf.sample
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- adminauth.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/app_rpt/app_rpt.h
- apps/app_rpt/rpt_totp.h
- apps/app_rpt.c
Comment on lines
+472
to
+481
| ;[totp-admin] | ||
| ; Add commands here for users authenticated as admin | ||
| ;901 = cop,1 ; System warm boot | ||
| ;902 = cop,2 ; System enable | ||
| ;903 = cop,3 ; System disable | ||
|
|
||
| ;[totp-operator] | ||
| ; Add commands here for users authenticated as operator. | ||
| ;916 = cop,16 ; Scheduler disable | ||
|
|
Contributor
There was a problem hiding this comment.
Keep the privileged stanza names consistent with the shipped rpt_auth.conf examples.
rpt_auth.conf and rpt_auth.conf.sample map users to functions-admin / functions-controlop / functions-diag, but this template defines [totp-admin] and [totp-operator]. Copying the examples as-is leaves authenticated users pointing at the wrong command-set stanza. Either rename these sections or update the auth-file examples so the names match exactly.
Suggested alignment
-;[totp-admin]
+;[functions-admin]
; Add commands here for users authenticated as admin
;901 = cop,1 ; System warm boot
;902 = cop,2 ; System enable
;903 = cop,3 ; System disable
-;[totp-operator]
+;[functions-controlop]
; Add commands here for users authenticated as operator.
;916 = cop,16 ; Scheduler disable
+
+;[functions-diag]
+; Add commands here for users authenticated as read-only diagnostics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configs/rpt/rpt.conf` around lines 472 - 481, The stanza names in this
template ([totp-admin] and [totp-operator]) do not match the shipped auth
examples (which use functions-admin / functions-controlop / functions-diag);
rename the sections in this file to use the exact stanza names from
rpt_auth.conf (e.g., change [totp-admin] -> [functions-admin] and
[totp-operator] -> [functions-controlop]) so authenticated users map to the
correct command-set stanzas, and ensure any commented command entries (e.g., the
cop lines) remain under the updated stanza names.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add TOTP DTMF Authentication for app_rpt
Summary
Adds per-user, per-node TOTP (RFC 6238) authentication to app_rpt, allowing administrators to gate privileged DTMF commands behind a one-time password verified via any standard
authenticator app (Google Authenticator, Authy, FreeOTP, etc.).
Motivation
Previously, every DTMF function in
rpt.confwas accessible to anyone who could key the radio — there was no concept of user-level access control. This feature introduces an authentication layer so that sensitive control commands (e.g., system key/unkey, restart scripts) can be restricted to authorized operators.
What It Does
rpt auth show <node>andrpt auth logout <node>.Configuration
rpt.conf: Newauth_*keys on the node stanza (auth_users,auth_timeout,auth_lockout_threshold,auth_lockout_duration,auth_otp_step,auth_otp_window), plus function table entries for login/status/logout sub-commands.
rpt_auth.conf(new file): Maps 4-digit user IDs to base32 TOTP secrets and privileged function stanzas. Contains shared secrets — file permissions enforced (0640 root: asterisk).Files Changed
apps/app_rpt/rpt_totp.{h,c}apps/app_rpt/rpt_auth.{h,c}struct rpt.apps/app_rpt/rpt_functions.{h,c}function_authDTMF handler (login/status/logout sub-commands).apps/app_rpt/app_rpt.hstruct rpt::p+ opaquestruct rpt_auth_state *authpointer.apps/app_rpt/rpt_config.cauth_*) and reload/free hooks.apps/app_rpt.cfunction_table[]registration +collect_function_digits()modified for authed-stanza-first dispatch and sliding timeout touch.apps/app_rpt/rpt_cli.crpt auth show/rpt auth logoutCLI commands.configs/rpt/rpt.confauth_*keys + example admin stanza.configs/samples/rpt_auth.conf.sampleDesign Decisions
rpt_auth.c; no ABI churn for future extensions.collect_function_digitssurgery — authed-stanza walk is O(1) skip when no session is active; hot path impact is negligible for nodes that don't enable auth.rpt.confis widely shared/version-controlled; secrets stay in a permission-protected file with no network attack surface.Testing
rpt_totp.cincludesRPT_TOTP_SELFTESTvalidating against RFC 6238 Appendix B canonical SHA-1 test vectors.Summary by CodeRabbit
New Features
Documentation