Skip to content

Commit f562506

Browse files
Krishcalinclaude
andcommitted
Write the platform change for BTP, integration and cryptography findings
THE QUALIFIER IS THE DEFECT IN THIS FAMILY, which is what makes it writable and is the exact inverse of hana_pack. There a qualifier meant the export had not typed the object, so the statement had to be refused. Here it names the thing to change — path=/, TrustAll=true, allowedHosts=['*'], instanceStatus=DELETED — and the object it belongs to is in the same subject entry, so no graph is read at all. The other three packs walk edges because a role change needs a (role, object) pair and an assignment a (user, role) pair; this one does not. BTP-DST-002 Legacy_ECC_SOAP — set TrustAll = false (currently TrustAll=true) BTP-SB-003 old-test-binding — delete the orphaned binding BTP-CC-001 S4H_Production — narrow path=/ to the paths your integrations use CRYPTO-CERT-001 SSL_Server_Cert — remove this expired certificate THREE SHAPES, NOT THE SAME STRENGTH: an exact target value (TrustAll has two settings and one of them verifies the certificate), a pure removal (an orphaned binding needs no replacement), and an exact coordinate whose value is the customer's (narrow path=/ — to WHAT is theirs). BTP IS NOT ONE CONSOLE, so `where` is per prefix: a Cloud Connector resource is narrowed in an on-premise admin UI that is not even in the cockpit, a destination in Connectivity, a queue in Event Mesh, a partner profile in WE20, a certificate in STRUST. One label reading "BTP" sends somebody to the wrong screen. THE FAMILY COUNTS DID NOT SURVIVE READING THE CHECKS, again. INTG showed 300 and CRYPTO 170; CRYPTO carries almost no qualifiers at all, and three INTG checks are the SAME design decision already declined on the BTP side — "an insecure mechanism is in use" is fixed by choosing what to move TO, which is a decision about the integration and not a setting. Seven checks are declined with that reason rather than left silent; a check nobody examined returns None instead, and the plan counts it under not_covered, because claiming a considered refusal for something nobody looked at is the more flattering lie. _NO_QUALIFIER exists for the four whose action needs no value: an unused OAuth client is deleted, an expired certificate removed, and the object's NAME is the whole coordinate. Without it they would decline as "records no setting", which is true and beside the point. It is an allowlist, not a loosening — a check whose action names %(qual)s still declines when the finding records none. CRYPTO-CERT-002 is the one pack with no rollback and says so. Restoring an expiring certificate is not an undo, it is reintroducing the finding. Renamed btp_pack to platform_pack once it handled three families. A name that lies is worse than a long one. PRD's plan goes 42 changes to 89 across five block kinds, with 9 declines each carrying a reason. Fourteen mutations across the two test files; all fourteen fail them. Two had to be rebuilt: replacing only a caveat's opening words left the phrases the assertions check, and slicing the caveat out entirely left a dangling `if`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent ce05f69 commit f562506

3 files changed

Lines changed: 658 additions & 1 deletion

File tree

server/remediation.py

Lines changed: 246 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,55 @@ def _qualified_names(row: Dict[str, Any]) -> set:
8686
return out
8787

8888

89+
def _qualified_subjects(row: Dict[str, Any]) -> List[tuple]:
90+
"""(name, qualifier) for every subject that recorded one, in order.
91+
92+
The pairs `platform_pack` writes from. `_qualified_names` above answers a
93+
different question — WHICH names carry a qualifier, so `hana_pack` can refuse
94+
them — and returns a set, so it cannot be reused here: a set loses both the
95+
qualifier and the order, and the same backend appears twice in one finding
96+
with two different paths.
97+
"""
98+
import json as _json
99+
100+
subject = row.get("subject") or row.get("affected_objects")
101+
if isinstance(subject, str):
102+
try:
103+
subject = _json.loads(subject)
104+
except ValueError:
105+
return []
106+
out = []
107+
for obj in (subject or ()):
108+
if isinstance(obj, dict) and obj.get("name") and obj.get("qualifier"):
109+
out.append((str(obj["name"]), str(obj["qualifier"])))
110+
return out
111+
112+
113+
def _named_subjects(row: Dict[str, Any]) -> List[str]:
114+
"""Every subject name, qualifier or not, in order and without duplicates.
115+
116+
For the checks whose action needs no setting — an unused OAuth client is
117+
deleted, an expired certificate removed — where the object's NAME is the
118+
whole coordinate. Deduplicated because two subject entries for one object
119+
would otherwise produce the same deletion step twice.
120+
"""
121+
import json as _json
122+
123+
subject = row.get("subject") or row.get("affected_objects")
124+
if isinstance(subject, str):
125+
try:
126+
subject = _json.loads(subject)
127+
except ValueError:
128+
return []
129+
out: List[str] = []
130+
for obj in (subject or ()):
131+
if isinstance(obj, dict) and obj.get("name"):
132+
name = str(obj["name"])
133+
if name not in out:
134+
out.append(name)
135+
return out
136+
137+
89138
def _detail(row: Dict[str, Any]) -> Dict[str, Any]:
90139
detail = row.get("details") or row.get("latest_details") or {}
91140
return detail if isinstance(detail, dict) else {}
@@ -551,6 +600,201 @@ def assignment_pack(row: Dict[str, Any],
551600
}
552601

553602

603+
BTP_SETTING = "btp_setting"
604+
PLATFORM_SETTING = "platform_setting"
605+
606+
#: Which console the change is made in, by check prefix. BTP is not one product:
607+
#: a Cloud Connector resource is narrowed in an on-premise admin UI, a
608+
#: destination in the cockpit, a queue in Event Mesh. A pack that said only "BTP"
609+
#: would send somebody to the wrong screen.
610+
_BTP_CONSOLE = {
611+
"BTP-CC": "Cloud Connector admin UI — Cloud To On-Premise > Access Control",
612+
"BTP-DST": "BTP cockpit — Connectivity > Destinations",
613+
"BTP-EM": "BTP cockpit — Event Mesh > Queues",
614+
"BTP-SB": "BTP cockpit — Instances and Subscriptions > Service Bindings",
615+
"INTG-IDOC": "WE20 — partner profiles",
616+
"INTG-OAUTH": "SOAUTH2 — OAuth 2.0 clients",
617+
"INTG-WH": "the webhook registry of the system that publishes them",
618+
"CRYPTO-CERT": "STRUST — the system PSE",
619+
}
620+
621+
#: check -> (what to do, how to undo it). `%(obj)s` is the named object and
622+
#: `%(qual)s` the qualifier the finding recorded, which for this family IS the
623+
#: defect: `path=/`, `TrustAll=true`, `allowedHosts=['*']`.
624+
_BTP_ACTIONS = {
625+
"BTP-CC-001": ("narrow %(qual)s to the specific paths your integrations use",
626+
"restore %(qual)s"),
627+
"BTP-CC-002": ("remove %(qual)s, or restrict it to the sub-paths actually "
628+
"consumed",
629+
"re-add %(qual)s"),
630+
"BTP-CC-004": ("replace %(qual)s with the specific hosts the subaccount needs",
631+
"restore %(qual)s"),
632+
# The one with a target value rather than a coordinate: TrustAll has exactly
633+
# two settings and only one of them verifies the certificate.
634+
"BTP-DST-002": ("set TrustAll = false (currently %(qual)s)",
635+
"set TrustAll = true"),
636+
"BTP-EM-001": ("narrow %(qual)s to the specific events this queue consumes",
637+
"restore %(qual)s"),
638+
"BTP-EM-002": ("define an access policy for this queue (currently %(qual)s)",
639+
"remove the access policy"),
640+
# Pure removal: an orphaned binding to a deleted instance grants nothing and
641+
# needs no replacement value.
642+
"BTP-SB-003": ("delete the orphaned binding (%(qual)s)",
643+
"re-create the binding"),
644+
645+
# ── Integration and cryptography ──────────────────────────────────────
646+
# Same two shapes as above, in different consoles. The four below take no
647+
# `%(qual)s` because their findings record none — the object's NAME is the
648+
# whole coordinate, and the action needs no value: an unused client is
649+
# deleted, an expired certificate removed. `_NO_QUALIFIER` says so, because
650+
# otherwise the missing qualifier reads as a finding too thin to act on.
651+
"INTG-IDOC-003": ("narrow %(qual)s to the message types this partner "
652+
"actually exchanges",
653+
"restore %(qual)s"),
654+
"INTG-OAUTH-001": ("remove %(qual)s from this client, leaving only the "
655+
"scopes it calls",
656+
"restore %(qual)s"),
657+
"INTG-OAUTH-003": ("delete this OAuth client — nothing has used it in the "
658+
"window the export covers",
659+
"re-create the client and re-issue its secret"),
660+
"INTG-WH-004": ("delete this webhook registration",
661+
"re-register the webhook"),
662+
"CRYPTO-CERT-001": ("remove this expired certificate from the trust store",
663+
"re-import the certificate"),
664+
"CRYPTO-CERT-002": ("renew this certificate before it expires",
665+
"no rollback: a renewal replaces an expiring certificate "
666+
"and the old one should not be restored"),
667+
}
668+
669+
#: Checks whose action names no `%(qual)s`, because the finding records none and
670+
#: the object's name is the whole coordinate. Kept as an explicit set rather than
671+
#: inferred from the template: a template that loses its `%(qual)s` in an edit
672+
#: would otherwise start silently accepting findings with no setting recorded.
673+
_NO_QUALIFIER = frozenset({
674+
"INTG-OAUTH-003", "INTG-WH-004", "CRYPTO-CERT-001", "CRYPTO-CERT-002",
675+
})
676+
677+
#: Examined and refused, with the reason. Separate from "no pack at all" because
678+
#: a reader looking for a fix is better served by "this one is a decision, and
679+
#: here is why" than by silence.
680+
_BTP_DECLINED = {
681+
"BTP-DST-001": "moving off basic authentication means choosing what to move "
682+
"TO — OAuth, a client certificate, a principal-propagation "
683+
"flow — and which one fits is a design decision about the "
684+
"integration, not a setting this tool can name",
685+
"BTP-CPI-002": "the same choice as BTP-DST-001, on the Integration Suite side",
686+
"BTP-CPI-004": "an iFlow with no sender authentication needs an authentication "
687+
"mechanism chosen for it, and which one depends on who calls it",
688+
"BTP-MIG-001": "moving an application from XSUAA to IAS is a migration with a "
689+
"cutover, not a property to set",
690+
"INTG-WS-001": "exposing a BAPI over basic authentication is fixed by choosing "
691+
"a stronger mechanism, and which one depends on what calls it",
692+
"INTG-WS-003": "the same choice, for a web service endpoint with no "
693+
"authentication at all",
694+
"INTG-OAUTH-002": "the password grant is deprecated, and moving off it means "
695+
"choosing the flow that replaces it — authorization code, "
696+
"client credentials — which is a decision about the client",
697+
}
698+
699+
700+
def platform_pack(row: Dict[str, Any],
701+
neighbourhood: Optional[Dict[str, Any]] = None) -> Optional[Dict[str, Any]]:
702+
"""The BTP-side change for a platform finding, or None.
703+
704+
THE QUALIFIER IS THE DEFECT HERE, which is what makes this family writable at
705+
all and is the opposite of `hana_pack`, where a qualifier meant the object
706+
could not be typed and the statement had to be refused. A Cloud Connector
707+
backend mapped at `path=/` and a destination carrying `TrustAll=true` both
708+
name the exact thing to change in the exact place to change it.
709+
710+
NO GRAPH NEEDED, and that is the difference from the other three packs. A
711+
role change needs a (role, object) pair and an assignment needs a (user,
712+
role) pair, so both read edges. Here the object and its defect are one
713+
subject entry — `{"name": "S4H_Production", "qualifier": "path=/"}` — so the
714+
finding is sufficient on its own.
715+
716+
NOT EXECUTABLE. Every one of these is a form in an admin console.
717+
"""
718+
check_id = str(row.get("check_id") or "")
719+
if not check_id.startswith(("BTP-", "INTG-", "CRYPTO-")):
720+
return None
721+
kind = BTP_SETTING if check_id.startswith("BTP-") else PLATFORM_SETTING
722+
723+
owner = str(row.get("remediation_owner") or "").strip().lower()
724+
if owner and owner != _CUSTOMER_FIXABLE:
725+
return {"kind": kind, "applicable": False, "owner": owner,
726+
"why": "this is not the customer's to change under the contract; "
727+
"raise a service request instead",
728+
"apply": [], "rollback": []}
729+
730+
if check_id in _BTP_DECLINED:
731+
return {"kind": kind, "applicable": False,
732+
"owner": owner or _CUSTOMER_FIXABLE,
733+
"why": _BTP_DECLINED[check_id], "apply": [], "rollback": []}
734+
735+
action = _BTP_ACTIONS.get(check_id)
736+
if action is None:
737+
return None
738+
739+
if check_id in _NO_QUALIFIER:
740+
# The object's name is the whole coordinate. `_qualified_subjects` would
741+
# return nothing here and the guard below would decline a finding that is
742+
# perfectly actionable.
743+
targets = [(n, "") for n in _named_subjects(row)]
744+
else:
745+
targets = _qualified_subjects(row)
746+
if not targets:
747+
# The check is one we can write for, and THIS finding did not record the
748+
# qualifier — so there is no defect to name. Declining beats emitting
749+
# "narrow to the specific paths", which is the blank-where-the-value-goes
750+
# failure `parameter_pack` refuses.
751+
return {"kind": kind, "applicable": False,
752+
"owner": owner or _CUSTOMER_FIXABLE,
753+
"why": "this finding records no setting for the objects it names, "
754+
"so there is nothing precise to change",
755+
"apply": [], "rollback": []}
756+
757+
verb, undo = action
758+
where = _BTP_CONSOLE.get(check_id.rsplit("-", 1)[0], "BTP cockpit")
759+
apply_steps, rollback_steps = [], []
760+
for name, qual in targets[:_MAX_STATEMENTS]:
761+
fields = {"obj": name, "qual": qual}
762+
apply_steps.append("%s — %s" % (name, verb % fields))
763+
rollback_steps.append("%s — %s" % (name, undo % fields))
764+
765+
caveats = [
766+
"Review and apply through your normal change control. This tool holds "
767+
"no connection to SAP and has changed nothing.",
768+
]
769+
if check_id.startswith("BTP-CC"):
770+
caveats.append(
771+
"A Cloud Connector resource change takes effect immediately for new "
772+
"requests and does not disturb established connections, so an "
773+
"integration that is currently working can start failing without an "
774+
"obvious cause. Narrow one backend at a time and watch it.")
775+
if check_id.startswith(("BTP-DST", "BTP-SB")):
776+
caveats.append(
777+
"Destinations and bindings are read at application start in some "
778+
"runtimes and per request in others; confirm which applies before "
779+
"assuming a change has taken effect.")
780+
if len(targets) > _MAX_STATEMENTS:
781+
caveats.append("%d of %d shown." % (_MAX_STATEMENTS, len(targets)))
782+
783+
return {
784+
"kind": kind,
785+
"applicable": True,
786+
"owner": owner or _CUSTOMER_FIXABLE,
787+
"where": where,
788+
"executable": False,
789+
"apply": apply_steps,
790+
"rollback": rollback_steps,
791+
"verify": "Re-run the scan; %s closes when these settings no longer "
792+
"report the defect." % check_id,
793+
"source": "",
794+
"caveats": caveats,
795+
}
796+
797+
554798
#: The states a finding must be in to belong in a change window. `accepted` is
555799
#: excluded deliberately: somebody decided to tolerate it, and putting it into a
556800
#: script would undo that decision without asking.
@@ -665,4 +909,5 @@ def pack(row: Dict[str, Any],
665909
return (parameter_pack(row)
666910
or hana_pack(row, neighbourhood)
667911
or role_pack(row, neighbourhood)
668-
or assignment_pack(row, neighbourhood))
912+
or assignment_pack(row, neighbourhood)
913+
or platform_pack(row, neighbourhood))

0 commit comments

Comments
 (0)