ge-aerospace/shopdb-flask
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Let a site close the legacy collector routes, once it can afford to

Browse Source 
Turning "Shared key allowed" off refused the key on the endpoints plugin and
left four core collector routes open to it. Anything still holding the old key
could keep rewriting logged-in user, last boot time, serial number and installed
app inventory for ANY hostname, while the admin believed the shared key was off.
Data integrity only - no command execution, no credential issuance - but it is a
hole in a boundary the product advertises as closed.

It cannot simply be closed. Both GE-Enforce reporters still authenticate with
that key, so refusing it fleet-wide would stop asset and printer reporting
everywhere, and on an air-gapped site mid-migration that is the wrong order of
operations. collector.legacysharedkeyrefused therefore DEFAULTS TO TODAY'S
BEHAVIOUR, and its description names exactly what turning it on breaks and when
it should be turned on: after the reporters move to per-PC credentials, not
before.

A legacy route used with the shared key now logs a warning, so a site can find
out who still depends on the key before flipping anything. On a stock Windows
deployment that warning may name the proxy rather than the caller; the token
prefix in the same line is the identifier to go by.

The switch is seeded with the core defaults and has a control on the
authentication settings page. Undeclared, it was a row that did not exist, and
this UI has no generic key/value editor, so seeding alone would have rendered
nothing for an operator to click.

Two contract declarations corrected while in here:

notifications.alertscope was declared db.JSON in the model while the migration
that creates it ships TEXT, and those migrations state the constraint in their
own docstrings: no JSON columns. The model now matches the shipped column rather
than the schema being bent to the model.

usb and printedparts both import a name that arrived in contract 0.35.0 while
declaring core_version floors of 0.1.0 and 0.16.0, so the loader would have
accepted them on a core that cannot satisfy them - an ImportError at blueprint
registration instead of a refusal with a reason. Both floors now say what the
plugin actually needs.
This commit is contained in:
cproudlock
2026-09-18 13:30:40 -04:00
parent 7ac1ee3077
commit 2f98188feb
10 changed files with 850 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

98
shopdb/core/api/collector.py
View File

@@ -27,6 +27,44 @@ COLLECTOR_SCOPE = 'collector.ingest'
# declaring acceptsagentcredential.
AGENT_CREDENTIAL_HEADER = 'X-Endpoint-Credential'
# How a collector request authorized, recorded on g so a route can tell a
# managed service token from the shared env key after the check has passed.
AUTH_MANAGED_TOKEN = 'token'
AUTH_SHARED_KEY = 'sharedkey'
# The four legacy routes below (/pc, /apps, /heartbeat, /bulk) predate both the
# generic collector contract and per-PC credentials. They take a hostname from
# the body and rewrite that PC's logged-in user, boot time, serial number and
# installed-app inventory, so the shared key is an any-host write key on them.
# plugin.endpoints.sharedkeyallowed does not reach here - it gates the endpoints
# collector, not core - so turning it off leaves these open. This setting is the
# core counterpart. Default false = today's behaviour, unchanged.
LEGACY_SHARED_KEY_SETTING = 'collector.legacysharedkeyrefused'
# Declared here so the wording stays with the code that enforces it.
# build_default_settings (shopdb/core/api/settings.py) has to include this dict
# for the row to be seeded; until it does, the key is created on demand through
# the settings API and the absent-row default below is what applies.
LEGACY_SHARED_KEY_SETTING_DEFAULT = {
'key': LEGACY_SHARED_KEY_SETTING,
'value': 'false',
'valuetype': 'boolean',
'category': 'collector',
'description': 'Refuse the shared collector key on the legacy core routes '
'POST /api/collector/pc, /apps, /heartbeat and /bulk. Those '
'routes name their target PC in the body, so any holder of '
'the shared key can rewrite logged-in user, last boot time, '
'serial number and installed apps for ANY hostname. Turning '
'this on closes them to the shared key; a collector-scoped '
'managed token still works. Turn it on only after checking '
'the "Legacy collector route used with the shared key" '
'warnings in the log for a while: whatever still calls them '
'stops working the moment it is on. The two GE-Enforce '
'reporters post to /api/collector/computers and '
'/api/collector/printers and are NOT affected. This should '
'follow the move to per-PC credentials, not precede it.',
}
def _computer_models():
"""Lazily import the computers plugin models.
@@ -131,13 +169,17 @@ def _check_collector_auth(expected_key, pluginname=None):
api_key = request.headers.get('X-API-Key')
g.collectorauth = None
token = _collector_managed_token(api_key)
if token is not None and _token_is_collector(token, pluginname):
touch_apitoken_lastused(token)
g.collectorauth = AUTH_MANAGED_TOKEN
return None
if expected_key and api_key == expected_key \
and not _env_fallback_disabled(pluginname):
g.collectorauth = AUTH_SHARED_KEY
return None
# Fail-closed only on pure server misconfiguration: no env key AND the
@@ -164,6 +206,54 @@ def require_api_key(f):
return decorated
def _legacy_shared_key_refused():
"""True when the site has closed the legacy routes to the shared key.
Absent setting = false = the routes stay open, which is what every site
does today. Read tolerantly because the row can be created by hand through
the settings API, where it may land with valuetype string.
"""
from shopdb.core.models.setting import Setting
value = Setting.get(LEGACY_SHARED_KEY_SETTING, default=False)
if isinstance(value, str):
return value.strip().lower() in ('true', '1', 'yes')
return bool(value)
def require_api_key_legacy(f):
"""require_api_key plus the legacy-route shared-key gate.
These routes take their target hostname from the body, so the shared key is
an any-host write key on them and no per-PC credential narrows it. A site
that has finished the move to per-PC credentials can shut it off here
without waiting for the routes to be retired.
Every shared-key use is logged whether the gate is on or off, so a site can
read the warnings and learn what still depends on the key BEFORE flipping
it - otherwise turning it on is a guess.
"""
from shopdb.utils.clientip import client_ip
@wraps(f)
def decorated(*args, **kwargs):
error = _check_collector_auth(current_app.config.get('COLLECTOR_API_KEY'))
if error is not None:
return error
if getattr(g, 'collectorauth', None) == AUTH_SHARED_KEY:
if _legacy_shared_key_refused():
return error_response(
'SHARED_KEY_REFUSED',
'The shared key is not accepted on the legacy collector '
'routes; use a collector-scoped token',
http_code=401)
current_app.logger.warning(
'Legacy collector route used with the shared key: %s from %s',
request.path, client_ip(request))
return f(*args, **kwargs)
return decorated
def _find_pc(hostname):
"""Find a computer by hostname, falling back to its asset number.
@@ -347,7 +437,7 @@ def collector_schemas():
@collector_bp.route('/pc', methods=['POST'])
@require_api_key
@require_api_key_legacy
def update_pc_info():
"""Update one PC from the collection script (matched by hostname)."""
data = request.get_json()
@@ -385,7 +475,7 @@ def update_pc_info():
@collector_bp.route('/apps', methods=['POST'])
@require_api_key
@require_api_key_legacy
def update_installed_apps():
"""Update installed applications for a PC (matched by hostname)."""
data = request.get_json()
@@ -457,7 +547,7 @@ def update_installed_apps():
@collector_bp.route('/heartbeat', methods=['POST'])
@require_api_key
@require_api_key_legacy
def pc_heartbeat():
"""Record PC online status / heartbeat (single or batch)."""
data = request.get_json()
@@ -492,7 +582,7 @@ def pc_heartbeat():
@collector_bp.route('/bulk', methods=['POST'])
@require_api_key
@require_api_key_legacy
def bulk_update():
"""Bulk update multiple PCs at once."""
data = request.get_json()

69
shopdb/core/api/settings.py
View File

@@ -161,14 +161,50 @@ def _declared_default(key: str) -> dict:
return _plugin_declared_settings().get(key, {})
def _is_secret(key: str) -> bool:
return 'password' in key or 'token' in key or 'secret' in key
def _is_secret(key: str, valuetype=None) -> bool:
"""True when a key's value must never leave the API in plaintext.
The test is the key NAME, which is a heuristic, so it is narrowed by the
stored type when the caller knows it. A boolean, integer or float setting
cannot hold a secret, and masking one breaks the control that renders it:
plugin.endpoints.bootstrapquerytoken is a true/false switch whose name
contains 'token', and the settings page read back '********' instead of
its state, so the checkbox could not show whether it was on.
valuetype omitted means "judge by name alone", which is the safe direction:
an unknown type is treated as a string.
"""
named = 'password' in key or 'token' in key or 'secret' in key
if not named:
return False
return valuetype is None or str(valuetype).lower() == 'string'
def _inferred_valuetype(value) -> str:
"""Valuetype for a key nobody declares, from the JSON type sent.
Only reached on the upsert path in update_setting. JSON already carries the
distinction the row has to keep, so honour it: true/false is a boolean row,
a number is integer or float, everything else is a string. Guessing from
the TEXT (e.g. the string 'true') is deliberately not done - a genuine
string setting whose value happens to read 'true' would then be typed
boolean and coerced out from under its reader.
bool is checked before int because bool subclasses int in Python.
"""
if isinstance(value, bool):
return 'boolean'
if isinstance(value, int):
return 'integer'
if isinstance(value, float):
return 'float'
return 'string'
def _serialize_setting(setting):
"""Serialize a setting, masking secret values so they never leave the API."""
data = setting.to_dict()
if _is_secret(setting.key):
if _is_secret(setting.key, setting.valuetype):
data['value'] = SECRET_MASK if setting.value else ''
return data
@@ -795,7 +831,14 @@ def update_setting(key: str):
setting = Setting(
key=key,
value='',
valuetype=declared.get('valuetype', 'string'),
# An undeclared key used to be born valuetype='string'
# unconditionally, and that was a trap: a caller PUTting JSON true
# got a string row, get_typed_value then handed every reader the
# text 'true', and a reader doing bool(...) or `is True` silently
# disagreed with the UI. Infer from the JSON type the client sent
# instead, so a boolean key is born boolean.
valuetype=declared.get('valuetype') or _inferred_valuetype(
data.get('value')),
category=declared.get('category', 'plugin'),
description=declared.get('description'),
)
@@ -808,7 +851,7 @@ def update_setting(key: str):
# A secret submitted as the mask placeholder means "leave unchanged" - the
# client only ever received the mask, so don't overwrite the real secret.
if _is_secret(key) and value == SECRET_MASK:
if _is_secret(key, setting.valuetype) and value == SECRET_MASK:
return success_response(_serialize_setting(setting), message='Setting unchanged')
# The declared valuetype is a promise about what every reader will get, and
@@ -846,7 +889,7 @@ def update_setting(key: str):
setting.value = str(value) if value is not None else None
# Audit log (mask sensitive values)
is_sensitive = _is_secret(key)
is_sensitive = _is_secret(key, setting.valuetype)
AuditLog.log('updated', 'Setting', entityname=key, changes={
'value': {
'old': '***' if is_sensitive else old_value,
@@ -915,10 +958,13 @@ def create_setting():
else:
value_str = str(value) if value is not None else None
# Same reasoning as the upsert in update_setting: a caller that omits
# valuetype and sends JSON true must not get a string row that every
# boolean reader then misreads.
setting = Setting(
key=data['key'],
value=value_str,
valuetype=data.get('valuetype', 'string'),
valuetype=data.get('valuetype') or _inferred_valuetype(value),
category=data.get('category', 'general'),
description=data.get('description')
)
@@ -1268,7 +1314,16 @@ def build_default_settings():
# Collector pc-type -> ComputerType mapping is computers-plugin domain;
# the plugin seeds pctypemap_<pxetype> settings on install.
# Collector authentication. The wording lives with the code that enforces
# it (shopdb/core/api/collector.py); seeding it here is what makes the row
# exist, and the row is what the settings UI can render a control for.
# Imported inside the function because core.api.__init__ imports collector
# and settings in the same package - a module-level import would couple
# their import order for no gain.
from shopdb.core.api.collector import LEGACY_SHARED_KEY_SETTING_DEFAULT
defaults = sitedefaults + brandingdefaults + printingdefaults + identifierdefaults + searchdefaults + mapdefaults + [
LEGACY_SHARED_KEY_SETTING_DEFAULT,
# ServiceNow ticket links. Each instance points these at its own
# ServiceNow tenant; blank/disabled renders tickets as plain text.
#