From 987f5c0d0f4c85bde61b1c5ca83f9eb65b62cefc Mon Sep 17 00:00:00 2001 From: cproudlock Date: Fri, 18 Sep 2026 13:30:08 -0400 Subject: [PATCH] Stop the one-line install handing a standard user a way in The bootstrap command staged its script into C:\Windows\Temp and then ran it elevated. That directory grants Users create-file with CREATOR OWNER full control, so somebody without admin rights can pre-plant the exact path the elevated run is about to execute. It now stages under Program Files, creates the directory with an explicit DACL when it is missing, and refuses to run a staged file that is not owned by an admin principal. A failed hardening is loud rather than silent: the run stops instead of carrying on with a file it could not protect. The token moved out of the query string into a header. In a query string it lands verbatim in the reverse proxy and IIS logs, which is a credential written to a file nobody thinks of as a secret store. The query form is still accepted, because bays provisioned before this change are holding install commands that use it, and it logs that it was used - without logging the token - so a site can see who still needs a re-issued command before turning it off with plugin.endpoints.bootstrapquerytoken. Two smaller things this also closes: A request with no token at all answered 400 naming the header, which told an anonymous caller the state of that switch and contradicted the module's own promise that anything unusable answers 404. No token is now 404. The 400 is kept for a token genuinely presented in the query form while the query form is off, which is a caller who needs to know their command is stale. The rollout documentation was republishing the vulnerable one-liner verbatim, so a tech following the runbook reinstated exactly what this fixes. Both documents now carry the current command. Also corrects the operator-facing copy for the token, which named a query argument and a header that do not exist: it is ?t= and X-Endpoint-Bootstrap-Token, per bootstrap.py's own constants. Anybody following the old text got a 404 with no reason given. Both switches added in this range are now declared with the other plugin settings and reachable from their settings page. Read through Setting.get but never declared, they were rows that did not exist until someone wrote one by hand, which on an air-gapped box means the feature was unreachable. --- plugins/endpoints/AGENT-CONTRACT.md | 92 ++++-- plugins/endpoints/api/bootstrap.py | 295 +++++++++++++++--- .../docs/PER-PC-CREDENTIALS-ROLLOUT.md | 71 ++++- .../frontend/views/AgentKeySettings.vue | 82 +++++ plugins/endpoints/plugin.py | 44 +++ .../test_endpoints_bootstrap_install.py | 286 ++++++++++++++++- .../test_endpoints_settings_declared.py | 145 +++++++++ 7 files changed, 934 insertions(+), 81 deletions(-) create mode 100644 tests/test_plugins/test_endpoints_settings_declared.py diff --git a/plugins/endpoints/AGENT-CONTRACT.md b/plugins/endpoints/AGENT-CONTRACT.md index b7813459..cf1a6b2e 100644 --- a/plugins/endpoints/AGENT-CONTRACT.md +++ b/plugins/endpoints/AGENT-CONTRACT.md @@ -791,27 +791,62 @@ One-line install and hands a technician one command, which is run in an ELEVATED PowerShell on a PC with no agent: ``` -powershell -ExecutionPolicy Bypass -c "try { Invoke-WebRequest -UseBasicParsing '/api/endpoints/siteagent/bootstrap.ps1?t=' -OutFile 'C:\Windows\Temp\shopdb-bootstrap.ps1'; & 'C:\Windows\Temp\shopdb-bootstrap.ps1' } finally { Remove-Item 'C:\Windows\Temp\shopdb-bootstrap.ps1' -Force -ErrorAction SilentlyContinue }" +powershell -ExecutionPolicy Bypass -c "try { New-Item -ItemType Directory -Force -Path 'C:\Program Files\ShopDB\bootstrap' | Out-Null; icacls 'C:\Program Files\ShopDB\bootstrap' /inheritance:r /grant '*S-1-5-32-544:(OI)(CI)F' '*S-1-5-18:(OI)(CI)F' | Out-Null; if ((Get-Variable LASTEXITCODE -ValueOnly) -ne 0) { Write-Host 'Refusing to run: could not restrict C:\Program Files\ShopDB\bootstrap to administrators (icacls failed). Fix the directory or delete it, then run the command again.' -ForegroundColor Red; exit 6 }; Remove-Item 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1' -Force -ErrorAction SilentlyContinue; Invoke-WebRequest -UseBasicParsing '/api/endpoints/siteagent/bootstrap.ps1' -Headers @{'X-Endpoint-Bootstrap-Token'=''} -OutFile 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1'; if ((Get-Acl 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1').GetOwner([Security.Principal.SecurityIdentifier]).Value -notin @('S-1-5-32-544','S-1-5-18',[Security.Principal.WindowsIdentity]::GetCurrent().User.Value)) { Write-Host 'Refusing to run: the staged bootstrap script is not owned by an administrator.' -ForegroundColor Red; exit 5 }; & 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1' } finally { Remove-Item 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1' -Force -ErrorAction SilentlyContinue }" ``` Download then run, never `irm ... | iex`: Microsoft Defender scores the piped-to-iex shape as `Trojan:Win32/Commando.A!ml` and kills the process before a line of it runs (reproduced 5 of 5 on a default install, no ASR rules), and -every bay runs Defender. The shape of the line is load-bearing too: +every bay runs Defender. The rest of the shape is load-bearing too: -- a literal temp path and no `$` anywhere, because pasting into an - already-running PowerShell would otherwise expand `$p` and `$env:TEMP` in the - PARENT and hand the child a broken line - as written it means the same in - cmd.exe and in PowerShell; +- the token rides in the `X-Endpoint-Bootstrap-Token` HEADER. A query string + lands verbatim in the IIS and waitress access logs, and a live install token + has no business sitting in a log file; +- staging is `C:\Program Files\ShopDB\bootstrap`, **not** + `C:\Windows\Temp`. That directory grants Users create-file and gives CREATOR + OWNER full control of what they create, so any standard user on the bay can + pre-plant `C:\Windows\Temp\shopdb-bootstrap.ps1` and wait for a tech to + paste the command, which then runs the planted content elevated; +- the line re-asserts an admin-only DACL on the directory it creates + (`icacls /inheritance:r` granting only the well-known SIDs `S-1-5-32-544` and + `S-1-5-18`, never group names, which a rename or a localized install would + break), and CHECKS that icacls succeeded: it is a native command, so it + reports failure only in an exit code and `$ErrorActionPreference` never + applies to it. Exit 6 with a red message, rather than downloading into a + directory that still inherits whatever the parent grants; +- it refuses (exit 5) to run a staged file owned by anyone but an + administrator, `SYSTEM`, or the elevated caller, which is what a pre-planted + file looks like; +- a literal staging path and no `$` anywhere - including the exit-code read, + which is `Get-Variable LASTEXITCODE -ValueOnly`. Pasting into an + already-running PowerShell would otherwise expand every `$` in the PARENT and + hand the child a broken line. As written it means the same in cmd.exe and in + PowerShell; - single quotes for every inner literal, so the outer `-c "..."` is never closed early; - `try`/`finally`, so the dropped file is removed even when the script exits non-zero or the download itself fails. The wrapper's own exit code is the removal's, so read the script's printed result rather than `%ERRORLEVEL%`. -The same URL is offered in the UI as a **Download .ps1** link, for a tech who -would rather save the file and run it. Fetching it does not spend a use either -way. +The same script is offered in the UI as a **Download .ps1** link, for a tech +who would rather save the file and run it. That link is the one caller that +cannot send a header, so it uses `?t=&via=download`. Fetching the script +does not spend a use either way. + +The `?t=` form without `via=download` is accepted ONLY so a bay provisioned +before this change - one still running a command pasted when the query form was +the published shape - keeps working. It is deprecated: every such fetch logs a +warning naming the caller's IP, and that warning is how the rollout finds bays +whose command needs re-issuing. Do not publish it, do not paste it into a new +bay, and do not build it by hand. A site that has re-issued all of its live +commands turns the query form off with +`plugin.endpoints.bootstrapquerytoken` = false, which also disables the +Download .ps1 link. + +The key is declared in the endpoints plugin's `get_settings_defaults()`, so it +seeds as a boolean row, and it is flipped from Settings > Endpoints > Build +site agent > One-line install ("Accept the token in the URL"). It is read +through the core settings API, not a route of this plugin's own. The token is only an authorization to FETCH the installer. The package it serves carries no shared key, whatever `plugin.endpoints.sharedkeyallowed` @@ -822,26 +857,41 @@ Admin routes (admin role, audited): - `POST /api/endpoints/siteagent/bootstrap/tokens` `{"hours": 24, "maxuses": 25}` (`hours` 1-168, `maxuses` 1-500). Returns - `{"secret", "command", "scripturl", "token"}`; `scripturl` is the same token - URL the command downloads, for the UI's Download .ps1 link. - The secret is in the response exactly once; only its sha256 and a 12-character - `tokenprefix` are stored. + `{"secret", "command", "scripturl", "token"}`. `command` carries the token in + the header; `scripturl` is the same route for the UI's Download .ps1 link, in + the `?t=&via=download` form a browser link can actually use. The + `via=download` marker is what keeps an admin's own click out of the + un-migrated-bay warning count - it is a hint, not authorization, and nothing + is granted by it; what matters is that a command generated before this change + cannot carry it. The secret is in the response exactly once; only its sha256 + and a 12-character `tokenprefix` are stored. - `GET /api/endpoints/siteagent/bootstrap/tokens` - prefix, created, expires, `usecount` / `maxuses`, `revokedat`, `lastusedat`, `lastusedip` and a `state` of `active`, `expired`, `exhausted` or `revoked`. Never a secret. - `POST /api/endpoints/siteagent/bootstrap/tokens//revoke` -Token-authenticated routes. No login; the token is the whole authorization and -travels in the query string as `t`, because a PC with no agent has neither a -credential nor a user session to put in a header: +Token-authenticated routes. No login; the token is the whole authorization, +because a PC with no agent has neither a credential nor a user session. It +travels in the `X-Endpoint-Bootstrap-Token` header: | Route | Spends a use | Returns | |---|---|---| -| `GET /api/endpoints/siteagent/bootstrap.ps1?t=...` | no | `text/plain` PowerShell, generated per token | -| `GET /api/endpoints/siteagent/bootstrap/package?t=...` | yes | the keyless agent package ZIP | +| `GET /api/endpoints/siteagent/bootstrap.ps1` | no | `text/plain` PowerShell, generated per token | +| `GET /api/endpoints/siteagent/bootstrap/package` | yes | the keyless agent package ZIP | + +Both also accept `?t=` while +`plugin.endpoints.bootstrapquerytoken` is true (the default), for bays +provisioned before the header form and for the UI Download .ps1 link. A query +token without `via=download` logs a deprecation warning against the caller's +IP; the Download link's own fetches log at info instead and are audited as +`(query-string token, admin download link)`, so the warning count stays a list +of bays to re-issue. With the setting false, a query token answers **400** +naming the header. Both answer **404** for a token that is unknown, malformed, expired, revoked or -exhausted, so the URL confirms nothing about which tokens exist. Both are +exhausted, and for a request that presents no token at all - so the URL +confirms nothing about which tokens exist, and a bare probe cannot learn +whether this site still accepts the query form. Both are rate-limited per IP (fixed window, `ENDPOINTS_BOOTSTRAP_RATELIMIT_MAX` in `ENDPOINTS_BOOTSTRAP_RATELIMIT_WINDOW_SECONDS`, 429 `RATE_LIMITED` over budget) and both write an audit row against `EndpointBootstrapToken` carrying @@ -851,7 +901,9 @@ retried paste must not burn the download budget. The generated script: - refuses to run unelevated (exit 1) and says to reopen PowerShell as - administrator; + administrator (the wrapper line has already refused with exit 6 if the + staging directory could not be locked down, or exit 5 if the staged file was + owned by someone else); - refuses when the agent service already exists (exit 2) and says a managed bay should get the agent from GE-Enforce, unless `SHOPDB_BOOTSTRAP_FORCE` is set to anything but `0` in the same session; diff --git a/plugins/endpoints/api/bootstrap.py b/plugins/endpoints/api/bootstrap.py index b8ea3b8e..e602a52b 100644 --- a/plugins/endpoints/api/bootstrap.py +++ b/plugins/endpoints/api/bootstrap.py @@ -14,14 +14,25 @@ Routes POST /siteagent/bootstrap/tokens admin, audited; secret once GET /siteagent/bootstrap/tokens admin; never a secret POST /siteagent/bootstrap/tokens//revoke admin, audited - GET /siteagent/bootstrap.ps1?t=... token only; not a use - GET /siteagent/bootstrap/package?t=... token only; one use + GET /siteagent/bootstrap.ps1 token only; not a use + GET /siteagent/bootstrap/package token only; one use -The two token-authenticated routes answer 404 for anything not currently -usable - unknown, expired, revoked or exhausted - so the URL confirms nothing -about which tokens exist. Both are per-IP rate limited with the same fixed -window shape the login and payload-download limiters use, and both write an -audit row carrying the caller's IP. +The two token-authenticated routes take the token in the +X-Endpoint-Bootstrap-Token header, and still accept the deprecated ?t= form so +a command already pasted on a bay keeps working. They answer 404 for anything +not currently usable - unknown, malformed, expired, revoked, exhausted, or no +token presented at all - so the URL confirms nothing about which tokens exist, +nor about whether the query form is still accepted here. The one 400 is a real +query token offered at a site that has turned the query form off. + +The admin UI's Download .ps1 link has to use the query form (a browser link +cannot send a header), so its URL carries a marker that tells it apart from a +bay still running a command pasted before this change. Only the bay logs the +deprecation warning, which is what the rollout counts un-migrated bays with. + +Both routes are per-IP rate limited with the same fixed window shape the login +and payload-download limiters use, and both write an audit row carrying the +caller's IP. """ import hashlib @@ -60,12 +71,60 @@ LIST_LIMIT = 100 SCRIPT_PATH = '/api/endpoints/siteagent/bootstrap.ps1' PACKAGE_PATH = '/api/endpoints/siteagent/bootstrap/package' -# Where the one-line command drops the script before running it. A literal, not -# $env:TEMP: the line has to survive being pasted into an already-running +# Where the one-line command stages the script before running it. A literal, +# not $env:TEMP: the line has to survive being pasted into an already-running # PowerShell, which expands every $ inside the outer double quotes before the -# child process sees them. The install needs elevation anyway, so this path is -# writable by definition. -COMMAND_SCRIPT_PATH = r'C:\Windows\Temp\shopdb-bootstrap.ps1' +# child process sees them. +# +# NOT C:\Windows\Temp. That directory grants Users create-file and gives +# CREATOR OWNER full control of what they create, so any standard user on the +# bay can pre-plant this exact path and wait for a tech to paste the command, +# which then runs the planted content elevated. Program Files is admin-only on +# a stock install, the command re-asserts an admin-only DACL on the directory +# it creates, and it refuses to run a staged file owned by anyone else. +COMMAND_STAGE_DIR = r'C:\Program Files\ShopDB\bootstrap' +COMMAND_SCRIPT_PATH = COMMAND_STAGE_DIR + r'\shopdb-bootstrap.ps1' + +# Well-known SIDs, not names: the bays are domain-joined and a renamed or +# localized Administrators group would quietly break both the grant and the +# ownership test. +ADMINISTRATORS_SID = 'S-1-5-32-544' +SYSTEM_SID = 'S-1-5-18' + +# The token the two public routes accept. A header, not ?t=: a query string +# lands verbatim in the reverse proxy and gunicorn access logs, and a live +# install token has no business sitting in a log file. +BOOTSTRAP_TOKEN_HEADER = 'X-Endpoint-Bootstrap-Token' +QUERY_TOKEN_ARG = 't' + +# Where the token on this request came from. Kept apart because the rollout +# counts un-migrated bays by the deprecation warning SOURCE_QUERY logs: an +# admin clicking Download .ps1 in the browser must not land in that count, and +# a request carrying no token at all must not answer differently from one +# carrying a token nobody issued. +SOURCE_HEADER = 'header' +SOURCE_QUERY = 'query' +SOURCE_UI_LINK = 'uilink' +SOURCE_NONE = 'none' + +# The marker on the admin Download .ps1 link. A hint, NOT authentication: a +# caller can add it by hand, and nothing is granted by it. What matters is the +# other direction - a one-line command generated before this change cannot +# carry it, so a request without it really is a bay to go and re-issue. +UI_LINK_ARG = 'via' +UI_LINK_VALUE = 'download' + +# DEPRECATED, kept on purpose: every command already pasted on a bay, and the +# UI's "Download .ps1" link (a browser link cannot send a header), carry the +# token in the query string. Accepting it stays the default so a mid-rollout +# bay keeps working; a site that has re-issued its live commands turns it off. +# Turning it off also disables the download link. +# +# Declared in the endpoints plugin's get_settings_defaults() (so the row seeds +# boolean, not string) and flipped from Settings > Endpoints > Build site agent +# > One-line install, "Accept the token in the URL". A switch with no seed and +# no control would be inert on an air-gapped box. +QUERY_TOKEN_SETTING = 'plugin.endpoints.bootstrapquerytoken' # Fixed-window per-IP limiter, same shape and cache extension as the login # limiter in shopdb.core.api.auth and the GE-Enforce payload limiter. A tech @@ -86,6 +145,22 @@ def _naive_utcnow(): def _caller_ip(): + """The caller address for the audit rows and the deprecation warning. + + Read the value before trusting it to identify a bay. client_ip() returns + remote_addr and nothing else. On the shipped Windows deploy IIS launches + waitress on loopback through HttpPlatformHandler, and deploy/windows/ + web.config ships the X-Forwarded-For rewrite rule COMMENTED OUT - IIS sets + no such header on its own, so waitress's --trusted-proxy=127.0.0.1 + --trusted-proxy-count=1 has nothing to rewrite REMOTE_ADDR from and leaves + it as the loopback peer. TRUSTED_PROXY_COUNT also defaults to 0, so + ProxyFix is not installed either. Every row and every warning then reads + 127.0.0.1, and the un-migrated-bay list identifies no bay. + + It is a real address only where the site enabled that rewrite rule (the + installer's -ClientIpSource direct), or set TRUSTED_PROXY_COUNT for a + front proxy that sets the header itself. + """ return client_ip(request) or 'unknown' @@ -142,11 +217,31 @@ def bootstrap_base_url(): return 'https://{0}{1}'.format(host, root) +def _base(baseurl=None): + resolved = baseurl if baseurl is not None else bootstrap_base_url() + return resolved.rstrip('/') + + def bootstrap_script_url(secret, baseurl=None): - """The token URL that serves the script: downloaded by the command, and - offered in the UI as a plain file a tech can save and run instead.""" - base = (baseurl if baseurl is not None else bootstrap_base_url()).rstrip('/') - return '{0}{1}?t={2}'.format(base, SCRIPT_PATH, secret) + """The UI's "Download .ps1" link: a plain file a tech can save and run. + + Still the query-string form, because an cannot set a header. + The one-line command no longer uses this shape - see bootstrap_command. + + It carries UI_LINK_ARG so the server can tell this click apart from a bay + running a pre-change command. Without that the admin's own Download link + fires the deprecation warning the rollout uses to FIND un-migrated bays, + and the count becomes noise the moment an admin mints a token. + """ + return '{0}{1}?{2}={3}&{4}={5}'.format(_base(baseurl), SCRIPT_PATH, + QUERY_TOKEN_ARG, secret, + UI_LINK_ARG, UI_LINK_VALUE) + + +def bootstrap_fetch_url(path, baseurl=None): + """A token-free URL for the machine-to-machine fetches. The token rides in + BOOTSTRAP_TOKEN_HEADER so it never reaches an access log.""" + return '{0}{1}'.format(_base(baseurl), path) def bootstrap_command(secret, baseurl=None): @@ -158,28 +253,145 @@ def bootstrap_command(secret, baseurl=None): Single quotes inside and no $ anywhere, so the one line means the same whether it is pasted into cmd.exe or into a PowerShell prompt. The download - and the run are wrapped in try/finally so the dropped file goes away even + and the run are wrapped in try/finally so the staged file goes away even when the script exits non-zero or the download itself fails. + + Staging is the part worth being careful about, so the line creates + COMMAND_STAGE_DIR with an admin-only DACL, clears any leftover file, and + refuses (exit 5) to run a staged script that some other principal owns - + which is what a pre-planted file looks like. Well-known SIDs throughout, + and the owner may also be the elevated caller, since a file created by an + administrator is owned by that account rather than by the group. + + The hardening is also CHECKED (exit 6). icacls is a native command, so it + reports failure only in an exit code: no exception is raised, and + $ErrorActionPreference never applies to it. Unchecked, an icacls that + cannot write the DACL leaves the directory inheriting whatever the parent + grants and the line downloads into it anyway - the silent version of the + hole this staging move was made to close. The same check also covers the + New-Item before it, because icacls on a directory that was never created + exits non-zero too. + + Read as `Get-Variable LASTEXITCODE -ValueOnly`, not `$LASTEXITCODE`: the + whole line must stay free of `$`, or a paste into an already-running + PowerShell expands it in the PARENT and the child is handed `-ne 0` with + the parent's last exit code baked in. """ - return ('powershell -ExecutionPolicy Bypass -c "' - "try {{ Invoke-WebRequest -UseBasicParsing '{0}' -OutFile '{1}'; " - "& '{1}' }} finally {{ Remove-Item '{1}' -Force " - '-ErrorAction SilentlyContinue }}"' - ).format(bootstrap_script_url(secret, baseurl), - COMMAND_SCRIPT_PATH) + return ( + 'powershell -ExecutionPolicy Bypass -c "' + "try {{ New-Item -ItemType Directory -Force -Path '{stagedir}' " + '| Out-Null; ' + "icacls '{stagedir}' /inheritance:r /grant '*{admin}:(OI)(CI)F' " + "'*{system}:(OI)(CI)F' | Out-Null; " + 'if ((Get-Variable LASTEXITCODE -ValueOnly) -ne 0) ' + "{{ Write-Host 'Refusing to run: could not restrict " + "{stagedir} to administrators (icacls failed). Fix the directory " + "or delete it, then run the command again.' -ForegroundColor Red; " + 'exit 6 }}; ' + "Remove-Item '{path}' -Force -ErrorAction SilentlyContinue; " + "Invoke-WebRequest -UseBasicParsing '{url}' " + "-Headers @{{'{header}'='{secret}'}} -OutFile '{path}'; " + "if ((Get-Acl '{path}').GetOwner(" + '[Security.Principal.SecurityIdentifier]).Value -notin ' + "@('{admin}','{system}'," + '[Security.Principal.WindowsIdentity]::GetCurrent().User.Value)) ' + "{{ Write-Host 'Refusing to run: the staged bootstrap script is not " + "owned by an administrator.' -ForegroundColor Red; exit 5 }}; " + "& '{path}' }} finally {{ Remove-Item '{path}' -Force " + '-ErrorAction SilentlyContinue }}"' + ).format(stagedir=COMMAND_STAGE_DIR, + path=COMMAND_SCRIPT_PATH, + url=bootstrap_fetch_url(SCRIPT_PATH, baseurl), + header=BOOTSTRAP_TOKEN_HEADER, + secret=secret, + admin=ADMINISTRATORS_SID, + system=SYSTEM_SID) def _hash_token(secret): return hashlib.sha256(secret.encode('utf-8')).hexdigest() -def _resolve_token(): - """The usable token named by ?t=, else None. +def _presented_token(): + """(secret, source) for the token this request carries. + + Header first, query string second. The query form is deprecated but still + the only one a browser download link can use. + + SOURCE_NONE when nothing was presented, which is NOT the same as an empty + query token: reporting a bare GET as 'query' made it answer 400 naming the + header, telling an anonymous caller whether this site had turned the query + form off, and contradicting the 404-for-everything-unusable rule at the top + of this module. + """ + header = (request.headers.get(BOOTSTRAP_TOKEN_HEADER) or '').strip() + if header: + return header, SOURCE_HEADER + query = (request.args.get(QUERY_TOKEN_ARG) or '').strip() + if not query: + return '', SOURCE_NONE + if request.args.get(UI_LINK_ARG) == UI_LINK_VALUE: + return query, SOURCE_UI_LINK + return query, SOURCE_QUERY + + +def _is_query_source(source): + """Both query-string forms, the bay's and the admin download link's. The + setting is about a token in an access log, and the admin link puts one + there just as surely as a pasted command does.""" + return source in (SOURCE_QUERY, SOURCE_UI_LINK) + + +def _query_token_allowed(): + """Defaults to True: today's pasted commands and the UI download link both + use the query form, and turning that off mid-rollout strands a bay.""" + return bool(Setting.get(QUERY_TOKEN_SETTING, True)) + + +def _query_token_refused(): + """Answered before any lookup, so it confirms nothing about any token.""" + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'Send the bootstrap token in the {0} header.'.format( + BOOTSTRAP_TOKEN_HEADER), + http_code=400) + + +def _note_token_source(source): + """Audit suffix for a token that really resolved, and, for a query-string + token, one log line naming the caller and WHICH query path it was. + + Only the bay warns. That warning is the rollout's un-migrated-bay counter + (see PER-PC-CREDENTIALS-ROLLOUT.md), so the admin's own Download .ps1 link + logs at info instead - otherwise every mint-and-test by an admin adds a + bay that does not exist. + + Never the token itself, in either line: keeping it out of the logs is the + whole point of the header form. + """ + if source == SOURCE_UI_LINK: + current_app.logger.info( + 'bootstrap script fetched by %s from the admin Download .ps1 ' + 'link; query-string token, but the admin browser path, NOT an ' + 'un-migrated bay', _caller_ip()) + return ' (query-string token, admin download link)' + if source == SOURCE_QUERY: + current_app.logger.warning( + 'bootstrap token presented in the query string by %s with no ' + 'admin download marker, so this is an un-migrated bay running a ' + 'command pasted before the header form; that form is deprecated ' + 'and slated for removal, re-issue the one-line install command', + _caller_ip()) + return ' (query-string token, deprecated)' + return '' + + +def _resolve_token(secret): + """The usable token with this secret, else None. Unknown, malformed, expired, revoked and exhausted all resolve to None so the caller can answer 404 for every one of them. """ - secret = (request.args.get('t') or '').strip() if not _TOKEN_PATTERN.match(secret): return None row = EndpointBootstrapToken.query.filter_by( @@ -218,6 +430,7 @@ _SCRIPT_TEMPLATE = '''# ShopDB endpoint agent bootstrap $ErrorActionPreference = "Stop" $BootstrapPackageUrl = {packageurl} +$BootstrapPackageHeaders = @{{ {tokenheader} = {token} }} $BootstrapServiceName = "ShopDbEndpointAgent" $BootstrapInstallDir = "C:\\Program Files\\ShopDB\\EndpointAgent" $BootstrapAgentSha256 = {agentsha} @@ -308,7 +521,7 @@ try {{ New-Item -ItemType Directory -Force -Path $bootstrapTempDir | Out-Null Write-Host "[1/4] Downloading the agent package ..." - Invoke-WebRequest -Uri $BootstrapPackageUrl -OutFile $bootstrapZipPath -UseBasicParsing + Invoke-WebRequest -Uri $BootstrapPackageUrl -Headers $BootstrapPackageHeaders -OutFile $bootstrapZipPath -UseBasicParsing Write-Host "[2/4] Expanding and verifying ..." Expand-Archive -Path $bootstrapZipPath -DestinationPath $bootstrapExtractDir -Force @@ -394,11 +607,12 @@ exit 0 def build_bootstrap_script(secret, baseurl=None): """The PowerShell text served for one token, with the URL and hashes baked in.""" - base = (baseurl if baseurl is not None else bootstrap_base_url()).rstrip('/') + base = _base(baseurl) hashes = siteagent.staged_binary_hashes() return _SCRIPT_TEMPLATE.format( - packageurl=json.dumps('{0}{1}?t={2}'.format(base, PACKAGE_PATH, - secret)), + packageurl=json.dumps(bootstrap_fetch_url(PACKAGE_PATH, base)), + tokenheader=json.dumps(BOOTSTRAP_TOKEN_HEADER), + token=json.dumps(secret), agentsha=json.dumps(hashes.get('agent', '')), traysha=json.dumps(hashes.get('tray', '')), forcevariable=FORCE_VARIABLE, @@ -500,7 +714,8 @@ def revoke_bootstrap_token(tokenid): # --------------------------------------------------------------------------- -# Token-authenticated: no login, the token in ?t= is the whole authorization +# Token-authenticated: no login, the token is the whole authorization. It +# rides in the X-Endpoint-Bootstrap-Token header; ?t= is still accepted. # --------------------------------------------------------------------------- @endpoints_bp.route('/siteagent/bootstrap.ps1', methods=['GET']) @@ -516,12 +731,15 @@ def serve_bootstrap_script(): """ if _ratelimited(): return _rate_limit_response() - row = _resolve_token() + secret, source = _presented_token() + if _is_query_source(source) and not _query_token_allowed(): + return _query_token_refused() + row = _resolve_token(secret) if row is None: return _not_found() - secret = (request.args.get('t') or '').strip() - _audit('requested', row, 'bootstrap script served') + _audit('requested', row, + 'bootstrap script served' + _note_token_source(source)) db.session.commit() return Response(build_bootstrap_script(secret), mimetype='text/plain') @@ -536,7 +754,10 @@ def download_bootstrap_package(): """ if _ratelimited(): return _rate_limit_response() - row = _resolve_token() + secret, source = _presented_token() + if _is_query_source(source) and not _query_token_allowed(): + return _query_token_refused() + row = _resolve_token(secret) if row is None: return _not_found() @@ -544,8 +765,8 @@ def download_bootstrap_package(): row.usecount = (row.usecount or 0) + 1 row.lastusedat = now row.lastusedip = _caller_ip()[:45] - _audit('downloaded', row, 'bootstrap package, use {0} of {1}'.format( - row.usecount, row.maxuses)) + _audit('downloaded', row, 'bootstrap package, use {0} of {1}{2}'.format( + row.usecount, row.maxuses, _note_token_source(source))) db.session.commit() buffer, warning = siteagent.build_keyless_package(bootstrap_base_url()) diff --git a/plugins/endpoints/docs/PER-PC-CREDENTIALS-ROLLOUT.md b/plugins/endpoints/docs/PER-PC-CREDENTIALS-ROLLOUT.md index 259ec22d..3998487e 100644 --- a/plugins/endpoints/docs/PER-PC-CREDENTIALS-ROLLOUT.md +++ b/plugins/endpoints/docs/PER-PC-CREDENTIALS-ROLLOUT.md @@ -292,17 +292,63 @@ says so when it finds an agent already installed. a new one if it is lost. 3. On the target PC, open PowerShell **as administrator** and paste it: +**Copy the command the UI gives you.** Do not retype it from here and do not +reuse one you saved from an older round - see the warning under this block. It +looks like this: + ``` -powershell -ExecutionPolicy Bypass -c "try { Invoke-WebRequest -UseBasicParsing 'https:///api/endpoints/siteagent/bootstrap.ps1?t=' -OutFile 'C:\Windows\Temp\shopdb-bootstrap.ps1'; & 'C:\Windows\Temp\shopdb-bootstrap.ps1' } finally { Remove-Item 'C:\Windows\Temp\shopdb-bootstrap.ps1' -Force -ErrorAction SilentlyContinue }" +powershell -ExecutionPolicy Bypass -c "try { New-Item -ItemType Directory -Force -Path 'C:\Program Files\ShopDB\bootstrap' | Out-Null; icacls 'C:\Program Files\ShopDB\bootstrap' /inheritance:r /grant '*S-1-5-32-544:(OI)(CI)F' '*S-1-5-18:(OI)(CI)F' | Out-Null; if ((Get-Variable LASTEXITCODE -ValueOnly) -ne 0) { Write-Host 'Refusing to run: could not restrict C:\Program Files\ShopDB\bootstrap to administrators (icacls failed). Fix the directory or delete it, then run the command again.' -ForegroundColor Red; exit 6 }; Remove-Item 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1' -Force -ErrorAction SilentlyContinue; Invoke-WebRequest -UseBasicParsing 'https:///api/endpoints/siteagent/bootstrap.ps1' -Headers @{'X-Endpoint-Bootstrap-Token'=''} -OutFile 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1'; if ((Get-Acl 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1').GetOwner([Security.Principal.SecurityIdentifier]).Value -notin @('S-1-5-32-544','S-1-5-18',[Security.Principal.WindowsIdentity]::GetCurrent().User.Value)) { Write-Host 'Refusing to run: the staged bootstrap script is not owned by an administrator.' -ForegroundColor Red; exit 5 }; & 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1' } finally { Remove-Item 'C:\Program Files\ShopDB\bootstrap\shopdb-bootstrap.ps1' -Force -ErrorAction SilentlyContinue }" ``` -It downloads the script, runs it and deletes it again. Do not shorten it back -to `irm ... | iex`: Defender scores that shape as `Trojan:Win32/Commando.A!ml` -and kills the process before anything runs, on a default install with no ASR -rules. Paste it as one line into an elevated **cmd** or **PowerShell** window; -it means the same in both. **Download .ps1** in the same block saves the script -instead, for a PC where pasting a long line is awkward - it carries the same -token, so treat the file the way you treat the command. +It creates an admin-only staging directory, downloads the script into it, runs +it and deletes it again. Do not shorten it back to `irm ... | iex`: Defender +scores that shape as `Trojan:Win32/Commando.A!ml` and kills the process before +anything runs, on a default install with no ASR rules. Paste it as one line +into an elevated **cmd** or **PowerShell** window; it means the same in both. +**Download .ps1** in the same block saves the script instead, for a PC where +pasting a long line is awkward - it carries the same token, so treat the file +the way you treat the command. + +> **Do not use the old `?t=` one-liner that staged into +> `C:\Windows\Temp`.** Any standard user on the bay can create that exact path +> first and wait for a tech to paste the command, which then runs the planted +> file elevated; the token also lands in the web server's access log. The +> command above stages under `C:\Program Files`, locks the directory to +> Administrators and SYSTEM and stops if that fails, refuses to run a staged +> file owned by anyone else, and sends the token in a header. +> +> The `?t=` form is still ACCEPTED, but only so a bay provisioned before this +> change keeps working until its command is re-issued. Every such fetch logs a +> server warning and writes an audit row against the token: +> +> ``` +> bootstrap token presented in the query string by with no admin +> download marker, so this is an un-migrated bay running a command pasted +> before the header form; ... re-issue the one-line install command +> ``` +> +> **`` is 127.0.0.1 on a stock Windows install.** IIS runs the app +> through HttpPlatformHandler over loopback and sets no `X-Forwarded-For` of +> its own; `deploy/windows/web.config` ships the rewrite rule that would add +> one commented out, so waitress has nothing to rewrite `REMOTE_ADDR` from. +> Check before you rely on the address: if these lines all say 127.0.0.1, the +> warning still tells you un-migrated bays EXIST and how many fetches they +> made, but not which bays. Get real addresses by enabling that rule (the +> Windows installer's `-ClientIpSource direct`, which installs URL Rewrite and +> allows `HTTP_X_FORWARDED_FOR` at server level); behind a further reverse +> proxy leave it off and set `TRUSTED_PROXY_COUNT` instead. Without that, the +> usable identifier is the token: each warning has a matching audit row on +> `EndpointBootstrapToken` carrying the 12-character `tokenprefix`, so you can +> at least tell which install round the stale command came from. +> +> Work that list down: mint a new token and hand out the new command. Clicking +> **Download .ps1** yourself does NOT add to it - that link is marked +> `&via=download` and logs at info, precisely so the warnings stay a list of +> bays. Once the list is empty, clear **Accept the token in the URL** in +> Settings -> Endpoints -> Build site agent -> One-line install +> (`plugin.endpoints.bootstrapquerytoken`) to refuse the query form outright. +> That also disables the Download .ps1 link and kills every command still in a +> technician's hands, so do it deliberately and re-issue first. The token is short-lived and only fetches the installer. It carries no shared key and provisions nothing by itself: the package it downloads is built with @@ -324,6 +370,15 @@ window to override). After a successful install it waits up to two minutes for the credential to land and prints `approved and enrolled` if an admin approves it in that window; still pending is a normal outcome, not a failure. +If it stops before any of that with a red **Refusing to run**, nothing was +installed and nothing was downloaded. Two cases, both meaning look at the bay +rather than retry: + +| Exit | Message | What to do | +|---|---|---| +| 6 | could not restrict `C:\Program Files\ShopDB\bootstrap` to administrators | the ACL could not be set - usually the directory already exists with an owner or DACL you did not put there. Inspect it (`icacls`), delete it, run the command again. | +| 5 | the staged bootstrap script is not owned by an administrator | something else wrote the staged file. Treat the bay as suspect, do not force past it. | + Revoke a token from the same block when the install round is done. Tokens list with their prefix, expiry and uses; they are also spent automatically (`usecount` reaching `maxuses`) and expire on their own. diff --git a/plugins/endpoints/frontend/views/AgentKeySettings.vue b/plugins/endpoints/frontend/views/AgentKeySettings.vue index e0ef8500..7ff5785d 100644 --- a/plugins/endpoints/frontend/views/AgentKeySettings.vue +++ b/plugins/endpoints/frontend/views/AgentKeySettings.vue @@ -482,6 +482,31 @@ Settings > Endpoints > Enrollment.

+ + +

{{ querytokenerror }}

+ @@ -764,6 +789,15 @@ const bootstraptokens = ref([]) const bootstrapmessage = ref('') const bootstraperror = ref('') +// plugin.endpoints.bootstrapquerytoken, read and written through the core +// settings API: the endpoints plugin has no settings route of its own for it, +// and the key is declared in get_settings_defaults so the upsert on PUT gets +// the boolean valuetype and the description from that declaration. +const QUERY_TOKEN_SETTING = 'plugin.endpoints.bootstrapquerytoken' +const querytokenallowed = ref(true) +const querytokenbusy = ref(false) +const querytokenerror = ref('') + function agentnextpininvalid() { const pin = agentnextserverpin.value return !!pin && !/^[0-9a-f]{64}$/.test(pin) @@ -863,6 +897,7 @@ onMounted(async () => { } loadEnrollment() loadBootstrapTokens() + loadQueryTokenSetting() try { const response = await api.get('/endpoints/siteagent') const status = response.data.data || {} @@ -878,6 +913,53 @@ onMounted(async () => { } }) +async function loadQueryTokenSetting() { + try { + const response = await api.get(`/settings/${QUERY_TOKEN_SETTING}`) + // The backend reads it as bool(Setting.get(key, True)), so only an + // explicit false is off. A row seeded as boolean comes back as a real + // false; an older row that was created as a string comes back as 'false'. + const value = response.data.data?.value + querytokenallowed.value = !(value === false || value === 'false') + } catch (loadError) { + // 404 = the row was never seeded (plugin installed before the key was + // declared). The backend default is on, so show on. + if (loadError.response?.status !== 404) console.error(loadError) + querytokenallowed.value = true + } +} + +async function onQueryTokenToggle(event) { + const turningOn = event.target.checked + if (!turningOn) { + const answer = prompt( + 'Stop accepting the token in the URL?\n\n' + + 'Every install command already handed out uses ?t= and stops ' + + 'working at once, including the Download .ps1 links. Re-issue them ' + + 'before turning this off.\n\n' + + 'Type OFF to confirm.') + if (!answer || answer.trim().toUpperCase() !== 'OFF') { + event.target.checked = true + return + } + } + querytokenbusy.value = true + querytokenerror.value = '' + try { + // PUT upserts, so this works on a site whose row was never seeded. + await api.put(`/settings/${QUERY_TOKEN_SETTING}`, { value: turningOn }) + querytokenallowed.value = turningOn + } catch (saveError) { + querytokenerror.value = saveError.response?.status === 403 + ? 'Only an administrator can change this setting' + : 'Could not save the setting' + event.target.checked = querytokenallowed.value + console.error(saveError) + } finally { + querytokenbusy.value = false + } +} + async function loadBootstrapTokens() { try { const response = await api.get('/endpoints/siteagent/bootstrap/tokens') diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index 9c417354..03276730 100644 --- a/plugins/endpoints/plugin.py +++ b/plugins/endpoints/plugin.py @@ -287,6 +287,15 @@ class EndpointsPlugin(BasePlugin): making revocation an instant kill switch. `agentkeyprovisioned` records that a UI-managed key has been minted so the settings page can render the rotate/revoke state instead of the first-run generate state. + + Every key an enforcement path reads belongs here, even when its default + is what the code already does with no row at all. Undeclared, the row + is only born when somebody writes it and the settings API has to guess + its type - which is how bootstrapquerytoken became a string row that + its boolean reader could never see turned off. Declared, the row is + seeded at install and enable (and by `flask plugin upgrade-all` on a + site that installed an earlier version), so the settings pages have + something to render and an operator has something to click. """ return [ { @@ -318,6 +327,41 @@ class EndpointsPlugin(BasePlugin): 'approval when the hostname and serial number ' 'match exactly one computer.', }, + { + 'key': 'plugin.endpoints.revokeddeniessharedkey', + 'value': 'false', + 'valuetype': 'boolean', + 'category': 'plugin.endpoints', + 'description': 'Also refuse the shared fleet key to a PC whose ' + 'per-PC credential was revoked. Off (the ' + 'default) is what the fleet does today: a ' + 'revoked PC falls back to the shared key, so a ' + 'credential rotation and the documented revoke ' + 'plus blockreenroll rollback to an older agent ' + 'both keep reporting. Turning it on makes a ' + 'revoke an eviction - that PC reports nothing ' + 'and receives no commands until it enrolls ' + 'again - and it stops that rollback working. ' + 'Safe only once every PC holds its own ' + 'credential.', + }, + { + 'key': 'plugin.endpoints.bootstrapquerytoken', + 'value': 'true', + 'valuetype': 'boolean', + 'category': 'plugin.endpoints', + 'description': 'Accept a one-line install bootstrap token in ' + 'the ?t= query string as well as the ' + 'X-Endpoint-Bootstrap-Token header. On (the ' + 'default) ' + 'keeps the pasted install commands and the ' + 'Download .ps1 link working; the query form is ' + 'deprecated because the token lands in IIS and ' + 'proxy logs. Turn it off only after every ' + 'outstanding install command has been re-issued ' + '- an in-flight one stops working the moment it ' + 'is off.', + }, { 'key': 'plugin.endpoints.agentkeyprovisioned', 'value': 'false', diff --git a/tests/test_plugins/test_endpoints_bootstrap_install.py b/tests/test_plugins/test_endpoints_bootstrap_install.py index 48f04d36..b063c1f1 100644 --- a/tests/test_plugins/test_endpoints_bootstrap_install.py +++ b/tests/test_plugins/test_endpoints_bootstrap_install.py @@ -11,6 +11,7 @@ import datetime import hashlib import io import json +import logging import zipfile import pytest @@ -27,17 +28,51 @@ TOKENS_URL = '/api/endpoints/siteagent/bootstrap/tokens' SCRIPT_URL = '/api/endpoints/siteagent/bootstrap.ps1' PACKAGE_URL = '/api/endpoints/siteagent/bootstrap/package' BASE_URL = 'https://shopdb.test/shopdb' -TEMP_SCRIPT = r'C:\Windows\Temp\shopdb-bootstrap.ps1' +TOKEN_HEADER = 'X-Endpoint-Bootstrap-Token' +QUERY_SETTING = 'plugin.endpoints.bootstrapquerytoken' +# Admin-only staging, NOT C:\Windows\Temp: a standard user can pre-plant a +# path there and have it run elevated. +STAGE_DIR = r'C:\Program Files\ShopDB\bootstrap' +STAGED_SCRIPT = STAGE_DIR + r'\shopdb-bootstrap.ps1' +ADMIN_SID = 'S-1-5-32-544' +SYSTEM_SID = 'S-1-5-18' +# The marker the admin "Download .ps1" link carries, so the server can tell an +# admin's browser click apart from a bay still on a pre-change pasted command. +UI_LINK_QUERY = 'via=download' def _expected_command(secret, base=BASE_URL): - """Download, run, delete. Never `irm | iex` - Defender kills that shape.""" - return ('powershell -ExecutionPolicy Bypass -c "' - "try {{ Invoke-WebRequest -UseBasicParsing " - "'{0}/api/endpoints/siteagent/bootstrap.ps1?t={1}' " - "-OutFile '{2}'; & '{2}' }} finally {{ " - "Remove-Item '{2}' -Force -ErrorAction SilentlyContinue }}\"" - ).format(base, secret, TEMP_SCRIPT) + """Stage admin-only, download with the token in a header, check the owner, + run, delete. Never `irm | iex` - Defender kills that shape.""" + return ( + 'powershell -ExecutionPolicy Bypass -c "' + "try {{ New-Item -ItemType Directory -Force -Path '{dir}' " + '| Out-Null; ' + "icacls '{dir}' /inheritance:r /grant '*{admin}:(OI)(CI)F' " + "'*{system}:(OI)(CI)F' | Out-Null; " + 'if ((Get-Variable LASTEXITCODE -ValueOnly) -ne 0) ' + "{{ Write-Host 'Refusing to run: could not restrict " + "{dir} to administrators (icacls failed). Fix the directory " + "or delete it, then run the command again.' -ForegroundColor Red; " + 'exit 6 }}; ' + "Remove-Item '{path}' -Force -ErrorAction SilentlyContinue; " + "Invoke-WebRequest -UseBasicParsing " + "'{base}/api/endpoints/siteagent/bootstrap.ps1' " + "-Headers @{{'{header}'='{secret}'}} -OutFile '{path}'; " + "if ((Get-Acl '{path}').GetOwner(" + '[Security.Principal.SecurityIdentifier]).Value -notin ' + "@('{admin}','{system}'," + '[Security.Principal.WindowsIdentity]::GetCurrent().User.Value)) ' + "{{ Write-Host 'Refusing to run: the staged bootstrap script is not " + "owned by an administrator.' -ForegroundColor Red; exit 5 }}; " + "& '{path}' }} finally {{ Remove-Item '{path}' -Force " + '-ErrorAction SilentlyContinue }}"' + ).format(dir=STAGE_DIR, path=STAGED_SCRIPT, base=base, header=TOKEN_HEADER, + secret=secret, admin=ADMIN_SID, system=SYSTEM_SID) + + +def _token_header(secret): + return {TOKEN_HEADER: secret} def _make_cert(): @@ -111,8 +146,8 @@ def test_generate_returns_the_secret_and_the_ready_made_command( assert len(secret) == len('sdbb_') + 43 assert data['command'] == _expected_command(secret) assert data['scripturl'] == ( - '{0}/api/endpoints/siteagent/bootstrap.ps1?t={1}'.format(BASE_URL, - secret)) + '{0}/api/endpoints/siteagent/bootstrap.ps1?t={1}&{2}'.format( + BASE_URL, secret, UI_LINK_QUERY)) token = data['token'] assert token['tokenprefix'] == secret[:12] assert token['maxuses'] == 3 @@ -136,13 +171,74 @@ def test_the_command_downloads_and_runs_instead_of_piping_into_iex( assert 'iex' not in command.lower() assert '$' not in command assert '\n' not in command - assert data['scripturl'] in command assert data['secret'] in command - assert "Remove-Item '{0}' -Force".format(TEMP_SCRIPT) in command + assert "Remove-Item '{0}' -Force".format(STAGED_SCRIPT) in command assert 'finally' in command # The link the UI offers instead is the route that really serves it. - assert client.get(SCRIPT_URL, - query_string={'t': data['secret']}).status_code == 200 + assert client.get(data['scripturl'].split(BASE_URL, 1)[1] + ).status_code == 200 + + +def test_the_command_stages_where_only_administrators_can_write( + client, db, auth_headers): + """C:\\Windows\\Temp lets any standard user create the exact path the + command runs elevated, so the staging directory moved under Program Files, + is created with an admin-only DACL, and the command refuses to run a staged + file owned by anyone but an administrator.""" + command = _secret(client, auth_headers)['command'] + + assert r'C:\Windows\Temp' not in command + assert "-OutFile '{0}'".format(STAGED_SCRIPT) in command + assert "New-Item -ItemType Directory -Force -Path '{0}'".format( + STAGE_DIR) in command + assert "icacls '{0}' /inheritance:r".format(STAGE_DIR) in command + # Well-known SIDs, so a renamed or localized group cannot widen the grant. + assert "/grant '*{0}:(OI)(CI)F' '*{1}:(OI)(CI)F'".format( + ADMIN_SID, SYSTEM_SID) in command + assert "Get-Acl '{0}'".format(STAGED_SCRIPT) in command + assert 'GetOwner([Security.Principal.SecurityIdentifier]).Value' in command + assert "-notin @('{0}','{1}',".format(ADMIN_SID, SYSTEM_SID) in command + assert '[Security.Principal.WindowsIdentity]::GetCurrent().User.Value' \ + in command + assert 'not owned by an administrator' in command + assert 'exit 5' in command + # A leftover from an earlier run is cleared before the download, so the + # owner test cannot pass on stale content. + assert command.index("Remove-Item '{0}'".format(STAGED_SCRIPT)) < \ + command.index('Invoke-WebRequest') + + +def test_the_command_sends_the_token_in_a_header_not_the_query_string( + client, db, auth_headers): + """A query string lands verbatim in the nginx and gunicorn access logs.""" + data = _secret(client, auth_headers) + command = data['command'] + + assert '?t=' not in command + assert "-Headers @{{'{0}'='{1}'}}".format(TOKEN_HEADER, + data['secret']) in command + assert "'{0}/api/endpoints/siteagent/bootstrap.ps1'".format( + BASE_URL) in command + # The browser download link is the one place that cannot send a header, + # and it is marked so its own fetches are not counted as un-migrated bays. + assert data['scripturl'].endswith( + '?t={0}&{1}'.format(data['secret'], UI_LINK_QUERY)) + + +def test_a_failed_acl_hardening_stops_the_command(client, db, auth_headers): + """icacls is a native command: it raises nothing and $ErrorActionPreference + never applies to it, so an unchecked call leaves a staging directory that + still inherits the parent DACL and the line downloads into it anyway. The + exit code is read sigil-free, because the whole line must hold no `$`.""" + command = _secret(client, auth_headers)['command'] + + assert '(Get-Variable LASTEXITCODE -ValueOnly) -ne 0' in command + assert 'LASTEXITCODE' in command and '$LASTEXITCODE' not in command + assert 'icacls failed' in command + assert 'exit 6' in command + # Checked before anything is fetched into the directory, not after. + assert command.index('LASTEXITCODE') > command.index('icacls') + assert command.index('LASTEXITCODE') < command.index('Invoke-WebRequest') def test_defaults_are_a_day_and_twenty_five_uses(client, db, auth_headers): @@ -226,8 +322,8 @@ def test_script_is_served_for_a_good_token(client, db, auth_headers): assert response.mimetype == 'text/plain' script = response.get_data(as_text=True) script.encode('ascii') - assert '{0}/api/endpoints/siteagent/bootstrap/package?t={1}'.format( - BASE_URL, secret) in script + assert '"{0}/api/endpoints/siteagent/bootstrap/package"'.format( + BASE_URL) in script assert 'Run as administrator' in script assert 'GE-Enforce' in script assert 'SHOPDB_BOOTSTRAP_FORCE' in script @@ -251,6 +347,163 @@ def test_script_bakes_the_staged_binary_hashes(client, db, auth_headers, assert hashlib.sha256(traypayload).hexdigest() in script +def test_the_generated_script_fetches_the_package_with_the_header( + client, db, auth_headers): + """Same reason as the one-liner: the package URL must not carry the token, + or every agent install writes it into an access log.""" + secret = _secret(client, auth_headers)['secret'] + script = client.get(SCRIPT_URL, + headers=_token_header(secret)).get_data(as_text=True) + + assert '?t=' not in script + assert '$BootstrapPackageHeaders = @{{ "{0}" = "{1}" }}'.format( + TOKEN_HEADER, secret) in script + assert ('Invoke-WebRequest -Uri $BootstrapPackageUrl ' + '-Headers $BootstrapPackageHeaders') in script + + +def test_both_routes_take_the_token_in_a_header(client, db, auth_headers, + servercert): + secret = _secret(client, auth_headers, maxuses=2)['secret'] + + script = client.get(SCRIPT_URL, headers=_token_header(secret)) + assert script.status_code == 200 + assert 'ShopDB endpoint agent bootstrap' in script.get_data(as_text=True) + assert 'install.ps1' in _files( + client.get(PACKAGE_URL, headers=_token_header(secret))) + assert EndpointBootstrapToken.query.one().usecount == 1 + + +@pytest.mark.parametrize('url', [SCRIPT_URL, PACKAGE_URL]) +def test_a_bad_header_token_is_not_found(client, db, auth_headers, url): + """The header is read first, so a good ?t= cannot rescue a bad header - one + request carries one token.""" + secret = _secret(client, auth_headers)['secret'] + response = client.get(url, query_string={'t': secret}, + headers=_token_header('sdbb_' + 'a' * 43)) + assert response.status_code == 404 + + +def test_the_query_form_still_works_and_is_audited_as_deprecated( + client, db, auth_headers, caplog): + """A bay running a command pasted before this change must keep working, so + the query form stays accepted; it is flagged in the audit row and warned + about in the log, with no token value in either.""" + secret = _secret(client, auth_headers)['secret'] + with caplog.at_level(logging.WARNING): + assert client.get(SCRIPT_URL, query_string={'t': secret}, + environ_overrides={'REMOTE_ADDR': '10.0.0.6'} + ).status_code == 200 + + row = AuditLog.query.filter_by(entitytype='EndpointBootstrapToken', + action='requested').one() + assert 'query-string token, deprecated' in row.details + assert secret not in row.details + warnings = [record.getMessage() for record in caplog.records + if record.levelno >= logging.WARNING] + assert any('query string' in message and '10.0.0.6' in message + for message in warnings) + assert all(secret not in message for message in warnings) + + +def test_the_header_form_is_not_flagged_as_deprecated(client, db, auth_headers, + caplog): + secret = _secret(client, auth_headers)['secret'] + with caplog.at_level(logging.WARNING): + client.get(SCRIPT_URL, headers=_token_header(secret)) + + row = AuditLog.query.filter_by(entitytype='EndpointBootstrapToken', + action='requested').one() + assert 'deprecated' not in row.details + assert not [record for record in caplog.records + if record.levelno >= logging.WARNING] + + +def test_the_admin_download_link_is_not_counted_as_an_un_migrated_bay( + client, db, auth_headers, caplog): + """The rollout finds un-migrated bays by the deprecation WARNING, and the + UI Download .ps1 link has to use the query form. Marked, it logs at info + and is audited as the admin path, so an admin testing a token does not + invent a bay that is not there.""" + data = _secret(client, auth_headers) + path = data['scripturl'].split(BASE_URL, 1)[1] + with caplog.at_level(logging.INFO): + assert client.get(path, environ_overrides={'REMOTE_ADDR': '10.0.0.7'} + ).status_code == 200 + + row = AuditLog.query.filter_by(entitytype='EndpointBootstrapToken', + action='requested').one() + assert 'admin download link' in row.details + assert 'deprecated' not in row.details + assert data['secret'] not in row.details + assert not [record for record in caplog.records + if record.levelno >= logging.WARNING] + infos = [record.getMessage() for record in caplog.records + if record.levelno == logging.INFO] + assert any('Download .ps1' in message and '10.0.0.7' in message + for message in infos) + assert all(data['secret'] not in message for message in infos) + + +def test_no_log_or_audit_row_ever_carries_the_token_value( + client, db, auth_headers, caplog, servercert): + secret = _secret(client, auth_headers, maxuses=4)['secret'] + with caplog.at_level(logging.DEBUG): + client.get(SCRIPT_URL, query_string={'t': secret}) + client.get(SCRIPT_URL, headers=_token_header(secret)) + client.get(PACKAGE_URL, query_string={'t': secret}) + client.get(PACKAGE_URL, headers=_token_header(secret)) + + assert all(secret not in record.getMessage() + for record in caplog.records) + rows = AuditLog.query.filter_by( + entitytype='EndpointBootstrapToken').all() + assert rows + for row in rows: + assert secret not in (row.details or '') + # The 12-char display prefix is the identifier the UI already shows. + assert row.entityname == secret[:12] + + +@pytest.mark.parametrize('url', [SCRIPT_URL, PACKAGE_URL]) +def test_the_query_form_can_be_turned_off_once_a_site_has_re_issued( + client, db, auth_headers, url): + """Off is the hardened end state, not the default: the UI download link + uses the query form too, so a site only turns it off deliberately.""" + secret = _secret(client, auth_headers)['secret'] + Setting.set(QUERY_SETTING, False, valuetype='boolean') + + refused = client.get(url, query_string={'t': secret}) + assert refused.status_code == 400 + assert TOKEN_HEADER in refused.get_json()['data']['error']['message'] + # The admin Download .ps1 link is a query token too: the setting is about + # a live token landing in an access log, and that link puts one there. + assert client.get(url, query_string={'t': secret, 'via': 'download'} + ).status_code == 400 + # Refused before any lookup, so it confirms nothing about the token. + assert EndpointBootstrapToken.query.one().usecount == 0 + assert AuditLog.query.filter_by( + entitytype='EndpointBootstrapToken', action='requested').count() == 0 + assert client.get(url, headers=_token_header(secret)).status_code == 200 + + +@pytest.mark.parametrize('url', [SCRIPT_URL, PACKAGE_URL]) +def test_no_token_is_still_not_found_when_the_query_form_is_off( + client, db, auth_headers, url): + """A bare GET presented no token, so it is unusable and answers 404 like + every other unusable request. Calling it a query token answered 400 naming + the header, which handed an anonymous caller the state of the switch.""" + _secret(client, auth_headers) + Setting.set(QUERY_SETTING, False, valuetype='boolean') + + assert client.get(url).status_code == 404 + # An empty ?t= presented nothing either. + assert client.get(url, query_string={'t': ''}).status_code == 404 + # And it is the same answer with the query form still on. + Setting.set(QUERY_SETTING, True, valuetype='boolean') + assert client.get(url).status_code == 404 + + def test_script_fetch_does_not_spend_a_use(client, db, auth_headers): secret = _secret(client, auth_headers, maxuses=1)['secret'] for _ in range(3): @@ -421,3 +674,4 @@ def test_the_command_falls_back_to_the_request_when_no_site_url_is_set( base='https://localhost') assert data['scripturl'].startswith( 'https://localhost/api/endpoints/siteagent/bootstrap.ps1?t=') + assert data['scripturl'].endswith('&' + UI_LINK_QUERY) diff --git a/tests/test_plugins/test_endpoints_settings_declared.py b/tests/test_plugins/test_endpoints_settings_declared.py new file mode 100644 index 00000000..8bf73036 --- /dev/null +++ b/tests/test_plugins/test_endpoints_settings_declared.py @@ -0,0 +1,145 @@ +"""The two endpoints switches are declared, seeded, and have a control. + +Both were readable by their enforcement code before they were declared: + + plugin.endpoints.revokeddeniessharedkey agentauth.revoked_denies_shared_key + plugin.endpoints.bootstrapquerytoken bootstrap._query_token_allowed + +An undeclared key has no row until somebody writes one, and until it was +declared the settings API upserted it with valuetype 'string' - so +bool(Setting.get(key, True)) read the text 'false' as True and the switch did +nothing. Declaring it fixes both halves: the row is seeded at plugin +install/enable, and a PUT lands as a real boolean. + +The UI half is checked by reading the pages. A switch with no control on an +air-gapped box can only be changed with SQL, which is not a setting. +""" + +from pathlib import Path + +import pytest + +from plugins.endpoints.api import agentauth, bootstrap +from plugins.endpoints.plugin import EndpointsPlugin +from shopdb.core.models import Setting + +REPO = Path(__file__).resolve().parents[2] +VIEWS = REPO / 'plugins' / 'endpoints' / 'frontend' / 'views' + +REVOKED_KEY = agentauth.REVOKED_DENIES_SHARED_KEY_SETTING +QUERY_KEY = bootstrap.QUERY_TOKEN_SETTING + + +def _declared(): + return {entry['key']: entry for entry in + EndpointsPlugin().get_settings_defaults()} + + +@pytest.mark.parametrize('key,value', [(REVOKED_KEY, 'false'), + (QUERY_KEY, 'true')]) +def test_the_switch_is_declared_with_todays_behaviour(key, value): + entry = _declared().get(key) + + assert entry is not None, key + ' is read but never declared' + assert entry['valuetype'] == 'boolean' + assert entry['category'] == 'plugin.endpoints' + # The declared default must be what the code does with no row at all, or + # seeding a site silently changes its behaviour. + assert entry['value'] == value + + +@pytest.mark.parametrize('key', [REVOKED_KEY, QUERY_KEY]) +def test_the_description_names_what_turning_it_on_costs(key): + """An operator on the shop floor decides from this text alone.""" + description = _declared()[key]['description'] + + assert len(description) > 120, description + if key == REVOKED_KEY: + # The revoke + blockreenroll rollback onto an older agent stops working. + assert 'rollback' in description + assert 'enrolls again' in description + else: + assert 're-issued' in description or 're-issue' in description + + +def test_the_declared_defaults_match_what_the_code_reads_with_no_row(db): + """No Setting rows at all: the readers fall back to these same values.""" + assert Setting.query.filter_by(key=REVOKED_KEY).first() is None + assert Setting.query.filter_by(key=QUERY_KEY).first() is None + + assert agentauth.revoked_denies_shared_key() is False + assert bootstrap._query_token_allowed() is True + + +def test_the_upgrade_pass_seeds_them_on_a_site_that_installed_earlier(app, db): + """install/enable fire only on a state transition, so a fleet already + running this plugin gets these rows from `flask plugin upgrade-all`, which + calls sync_all_plugin_settings.""" + pm = app.extensions.get('plugin_manager') + if pm is None: + pytest.skip('no plugin manager in this build') + assert Setting.query.filter_by(key=QUERY_KEY).first() is None + + pm.sync_all_plugin_settings() + + for key, value in ((REVOKED_KEY, False), (QUERY_KEY, True)): + row = Setting.query.filter_by(key=key).one() + assert row.valuetype == 'boolean' + assert row.category == 'plugin.endpoints' + assert row.get_typed_value() is value + + +def test_a_put_lands_as_a_boolean_and_flips_the_query_token(client, db, + auth_headers): + """End to end for bootstrapquerytoken through the API the page uses. + + The declaration is what makes the upsert boolean; as a string row the read + below would stay True whatever was saved. + """ + saved = client.put('/api/settings/' + QUERY_KEY, json={'value': False}, + headers=auth_headers) + + assert saved.status_code == 200, saved.get_json() + body = saved.get_json()['data'] + assert body['value'] is False + assert body['valuetype'] == 'boolean' + assert body['category'] == 'plugin.endpoints' + assert bootstrap._query_token_allowed() is False + + back = client.put('/api/settings/' + QUERY_KEY, json={'value': True}, + headers=auth_headers) + assert back.get_json()['data']['value'] is True + assert bootstrap._query_token_allowed() is True + + +def test_the_settings_api_lists_the_declared_switch(client, db, auth_headers): + """The agent-key page reads GET /api/settings/ to render the box.""" + client.put('/api/settings/' + QUERY_KEY, json={'value': True}, + headers=auth_headers) + + resp = client.get('/api/settings/' + QUERY_KEY, headers=auth_headers) + + assert resp.status_code == 200, resp.get_json() + assert resp.get_json()['data']['value'] is True + + +def test_the_enrollment_page_carries_the_eviction_checkbox(): + page = (VIEWS / 'EnrollmentSettings.vue').read_text(encoding='utf-8') + + assert 'toggle-revokeddenies' in page, 'no checkbox for the eviction switch' + assert 'onRevokedDeniesToggle' in page + # It sits with the two switches it belongs beside. + assert 'toggle-sharedkey' in page and 'toggle-autoenroll' in page + # The operator has to be told what it breaks before clicking it. + assert 'rollback' in page + assert 'enrolls again' in page + + +def test_the_agent_key_page_carries_the_query_token_control(): + page = (VIEWS / 'AgentKeySettings.vue').read_text(encoding='utf-8') + + assert QUERY_KEY in page, 'the page does not touch the setting' + assert 'toggle-querytoken' in page + assert 'onQueryTokenToggle' in page + # Turning it off breaks commands already handed out, so say so. + assert 're-issue' in page