Feature permissions - #119
Open
alowrydi wants to merge 13 commits into
Open
Conversation
…ention, admin input validation, permission-check select clauses
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
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.
Role-based access control and authentication, di.permissions
Consolidates four TorQ files -
code/handlers/permissions.q(.pm),writeaccess.q(.readonly),ldap.q(.ldap), andcode/common/execas.q- into a kdb-x module. Owns theexecphase of every message-handling.z.*event via the injecteddi.handlersdependency, permission-checks each incoming query against a user's roles and groups, and optionally enforces a whole-process read-only mode.controlaccess.q's separate tiered engine (superuser/poweruser/defaultuser) is deliberately deferred - see Design decisions.Trello ticket - https://trello.com/c/0ZvTnvAa/131-kdb-x-permissions-module
Files created
di/permissions/init.qpermissions.q, readsversionfromVERSION, defines export of 13 functionsdi/permissions/permissions.qdi/permissions/permissions.mddi/permissions/deps.qdi/permissions/VERSIONdi/permissions/test.csvdi/permissions/test_integration.csvHow to test
Unit tests:
k4unit counts every row it executes, so 534 includes the
before,runandcommentsetup rows. Of those, 263 are assertions - 188trueand 75fail- with 0 failures. Coverage includes: users/groups/roles and transitive group membership, query classification (select/update/delete, bare references, named function calls,.q-keyword calls including joins, lambda expressions), virtual tables, result size capping, read-only mode toggled at runtime, the full admin API including revoke/remove, LDAP dispatch and the injected-ldapbindcache/lockout/expiry path, root-name publication and teardown, dependency-versus-config key separation, and a systematic sweep asserting every admin entry point rejects a wrong-typed argument with a module-prefixed, logged error.Integration tests (spawns a real child q process on an OS-assigned port;
moduletestonly ever loadstest.csv, so this suite is loaded and run directly):10/10 passing. This suite exists because
reval's read-only restriction is not applied at.z.w=0- a unit test asserting a blocked write would pass against broken code. It proves read-only enforcement and parse-tree call handling over a real IPC handle.It wires the real
di.handlers, which ships on this branch, so it does exercise real phase andexec-ownership dispatch. Which path ran is asserted, not assumed: the.pi.realhrow requires the child to report1b, so a silent fall-through to the inline stand-in fails the suite rather than passing quietly. Verified both ways - withdi.handlersremoved fromQPATHthat row fails, leaving 9/10.Bugs fixed, all inherited from TorQ
Each is a real defect in the legacy code, not a refactor, and each has a test that fails against the unfixed version.
select p:first secret`pin from openreturnedsecret's data. Every readable object named in a clause is now grant-checked, using the same predicate a bare reference already uses.cloneuserbuilt and evaluated a string to hash the new password. It throws on any password containing a space or a backtick, and evaluates caller-supplied text in an auth path. Replaced with a directmd5 p.-publicthrows'typeinstead of refusing. Legacy evaluatesif["B"$(.Q.opt .z.x)[`public][0;0]]; with no-publicflag that'sif[`boolean$()], so on any process started without the flag, login throws for every unknown user rather than returning0b. Replaced with apublicboolean config key.val/valpwere bound once at load time; now resolved per call.valpthrew on every parse tree under read-only - the standard sync-IPC call shape. Legacy callsparseon it, which requires a string and throws'typeon a list, so a read-only process (canonically an HDB) rejected the commonest client idiom outright.assignrole/addtogrouprefuse undefined names, and the provisioning path assignedpublicuser/publicwithout ever creating them. TorQ has the identical gap, unreachable there only because bug 3 threw first.description:()is a general list; q collapses it to a typed vector once every row holds only atoms, so a one-character description turned the whole column into a char vector and the next multi-character description threw a bare'type..ldap.server/.ldap.portare undefined. The cache upsert writes them; the real setting isservers, plural. Both columns are dropped rather than fixed, since nothing ever reads them back.Design decisions
1. Owns the
execphase of six phased events -di.permissionsregistersexecon.z.pw,.z.pg,.z.ps,.z.pi,.z.ppand.z.ws, plus a simple.z.pcobserver, all under the stable namedi.permissionsso re-init reclaims rather than collides.di.handlersrejects apre/postregistration when noexecowner exists, so a pre-only design cannot register at all. It's also the only way to reproduce, inside a single-owner model, the three incompatible composition idioms TorQ uses on.z.pwalone (flat replace inpermissions.q, gate-and-call-through incontrolaccess.q, AND-compose inldap.q).2.
.z.phis deliberately not claimed;.h.valis assigned directly - On kdb+ 3.5+, HTTP GET permissioning happens at.h.val, which is whatpermissions.qitself sets - not.z.ph..h.valisn't a.z.*event, sodi.handlersrejects the symbol by design; the module assigns it directly, capturing the original once (guarded, sinceinitis idempotent) and restoring it on teardown. Verified end to end over real HTTP: a granted user's GET returns data, an ungranted table and a system call are both refused, an unauthenticated GET is rejected at.z.pw.3. Legacy
.pm.*names are republished at root, but only when enabled -usemangles module code into a private namespace, andconfig/permissions/*.qgrant files are executable q calling.pm.addrole,.pm.grantfunction,.pm.ALLand others at root on their first line. Publishing the seven grant-script functions plus the wildcard constant is what lets an unmodified TorQ grant file load, following the TorqX convention for.gw.*/.u.upd/.hdb.reload. Diverges from TorQ in publishing only whenenabled- safe, sincegateway.qalready guards on existence, and better, since a disabled module shouldn't advertise admin functions that gate nothing. Verified against the real, unmodifiedTorQ/config/permissions/default.q.4. RBAC only;
controlaccess.q's tiered engine is deferred - Theengineconfig key ships from v1, defaultsrbac, and rejectstieredwith a clear message, so the second engine can land later without reshaping the config schema. Evidence for deferring: outsidecontrolaccess.qitself, there is no functional caller of.access.*anywhere in TorQ - the three references that exist are a broken mutual-exclusion guard inpermissions.q(@[1b;...]returns0beven when the flag is set), API descriptions inapidetails.q(dissolved in the modular world), and a write-only setting nothing reads back. The module shipsenabled:0bby default, andtieredis rejected outright.6. Select clauses are permission-checked using the same predicate as a bare reference -
rbac.checkclausesreusesrbac.isdefinedvar, the identical function the bare-reference and lambda paths already use, minus the target table's own column names. An object can no longer be reachable through a clause while a direct reference to it is refused - the inconsistency an earlier, table-only version of this check left in place. Verified against both real leaks (a plain vector and a plain dict smuggled through a columns expression) and six ordinary queries that must not be over-blocked, including one where a column name collides with an unrelated global.7.
ignorelistdefaults empty, unlikezpsignore.q- TorQ ships it enabled with(`upd;"upd";`.u.upd;".u.upd"). Silently exemptingupdfrom permission checks is not a safe default for an access-control module. A process taking.u.upd-shaped feed traffic must set it explicitly, and the docs say so prominently.8. LDAP is optional and off by default, with an injectable
ldapbindseam - Matches TorQ's shipped settings, notldap.q's own file default, so the whole suite runs with no.sopresent.initaccepts an optionalldapbindkey that replaces the native library outright - a dependency, not a config value, since deps are process wiring the module already trusts absolutely. It exists so the cache, lockout and expiry logic can be exercised without a directory server; the native path is separately verified against a realkdbldap.so- all four symbols bind at the arities this module uses,initialiseopens a session, and a realkdbldap_bind_sexecutes with lockout bookkeeping intact.9. Public-user detection keeps TorQ's first-row lookup, not a full membership check - Looks like a bug and was implemented as one during the port, then reverted. It's correct by construction: the provisioning branch puts an anonymous user in exactly one group. A full check would change who's authorized in both directions - a real user also in
publiccould be rejected while presenting a valid password, or have their user row silently upserted over. That's an account-takeover path, so the narrower legacy check is the safer contract.10.
exit 1is not reproduced -permissions.q's mutual-exclusion guard againstcontrolaccess.qcallsexit 1. A module that can kill the process at load time is untestable; the equivalent guard here raises a normal, catchable error instead.11. All 21 admin functions ship, including the full revoke/remove half - Legacy
permissions.qdefines all of them;apidetails.qsimply doesn't advertise nine. An access-control module that can grant but not revoke at runtime is a real operational gap during an incident.12.
inittakes a single dict, like every other module -init[deps], with dependency keys (log,handlers, optionalldapbind) and configuration keys sharing one flat dict. Everydi.*module that has aninittakes one argument, anddi.logshipslogdictas a ready-made deps dict precisely becausedi.torqpasses one. A two-argumentinitcannot be wired at all, and fails silently rather than loudly - q returns a projection instead of throwing, so the module would register nothing while reporting no error. Dependency keys are stripped before the configuration is stored, so they never reachstatus[]or the unrecognised-key warning. A test wires the module straight offdi.log'slogdict, the shapedi.torqwill use.Deliberate omissions
.access.loginvalid- references an undefined handle and tables that exist nowhere in the repo, and is never called. Its counterpart.access.FILEis set bydotz.qand read by nobody. Both removed.init,setOption,bind_sanderr2stringare ever called; each binding is a load-time failure point..pm.cando- no callers anywhere, differed fromallowedonly by parsing first, whichallowednow does itself.Caveats worth disclosing up front
ldapdebugis a breaking config change - was an int (0i), is now a boolean (0b). A caller passingldapdebug:1iwill now failinit. The value was only ever read as an on/off flag; the int type was a lie the validator now refuses.rbac.checkclauses's shape guard is defence in depth, not reachable through the public API -2_qassumes a query tree of at least 5 elements; a change torbac.isq's threshold elsewhere could silently make the check permit rather than refuse. Now fails loudly and is tested directly against the module's own namespace.di.handlersprints (not throws) a bare'typewhen several phasedexecowners are removed in sequence. Reproducible withdi.handlersalone, nodi.permissionsinvolved; teardown completes correctly regardless. Raised separately against that module.Checklist
di.handlersconsistency.mdandstyle.mdpermissions.mddocuments all exported functions, config, usage examples and notesconfig/permissions/default.qdi.depcheckaudit clean - 0 failures, 0 warningsDocumentation
See
permissions.mdfor full reference including the dependency table, configuration table, exported function documentation with examples, clause-checking rationale and false-positive measurements, LDAP coverage notes, root-name publication details, migration guidance from.pm/.access, and notes.