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