Skip to content

fix(bastion-id): read the bastion id from /pam/whoami, and stop inventing one when that fails - #248

Merged
guimard merged 5 commits into
mainfrom
fix/bastion-id-whoami
Sep 6, 2026
Merged

guimard merged 5 commits into
mainfrom
fix/bastion-id-whoami

Conversation

@guimard

@guimard guimard commented Sep 6, 2026

Copy link
Copy Markdown
Member

Closes #246.

Upstream lemonldap-ng-plugins merged its whole audit round (#76#95) and
nothing is published yet, so this is the window to land the client side.

The rupture

#86 removes
/pam/bastion-token — it signed a JWT even when the user lookup had failed.
ob-bastion-id POSTs {"probe": true} to it and exit 2s on any non-200, and
both lab deployment paths feed its output straight into
/etc/open-bastion/allowed_bastions.

The non-obvious half is that deploy-shell.sh did not fail: it wrote the
literal string ob-bastion into the allowlist. That literal never matches the
bastion=<id> field of a hop certificate's key-id, so the lab refuses every
hop several phases later, with errors that point at certificates rather than at
the allowlist. deploy-ansible.sh did the same, and it also took that path in
Mode E, where the capture is skipped by design (sudo is locked to LLNG
tokens, so the mgmt user cannot read the server token).

The fix

#94 adds POST /pam/whoami, which returns the same portal-assigned device id under the same
bastion_id field, and is a pure read — no JWT, no session write, no heartbeat
stamp, so running ob-bastion-id no longer makes a bastion look freshly seen.

ob-bastion-id asks /pam/whoami, and falls back to the legacy probe only
on 404. Any other status — 403 from the caller gate especially — is reported,
not worked around, because falling back on a 403 would just produce a second
403 and a misleading message. So the command works against plugins 0.5.x and
0.6.0 alike, and can be deployed before the portal is upgraded.

The id is unchanged across the upgrade, which is worth stating plainly:
allowed_bastions files do not need rewriting, and re-enrolling a bastion
would break them by minting a new id.

curl -f had to go

The call carried -f and -w '%{http_code}'. -f makes curl exit non-zero
on any 4xx and discard the body, so every portal-side refusal collapsed into
the same opaque Request failed, and the if [ "$http_status" != "200" ]
branch below it was unreachable. Distinguishing 404 from 403 is the whole basis
of the fallback, so the status has to survive.

Verification

Against a mock portal, all four branches plus both output modes:

Portal Result
/pam/whoami → 200 rc=0, 9f86d081
/pam/whoami → 404, probe → bastion_id rc=0, legacy-id-42
/pam/whoami → 404, probe → bastion_jwt rc=0, jwt-id-7
/pam/whoami → 403 rc=2, no fallback attempted
both → 404 rc=2, reports the last status

The docker integration test now tolerates 404 as well as 403 — but note it
stays a skip in CI either way: docker-demo-cert/sso derives from the
published yadd/lemonldap-ng-portal:latest-non-root, so /pam/whoami will not
exist there until the plugins are released and the image rebuilt.

UPGRADE-NOTES.md

New file, as discussed. It covers the plugins 0.6.0 upgrade: this migration,
unbound vouchers dropping from 12 h to 15 min (a visible outage replacing a
silent weakening
— intended), the now exact-match PAM scope, which catches an
RP granted something like pam-prod, and why
pamAccessRequestSigningMode = required must not be turned on — that one is
#247, and it is the change in this round that can take a fleet down silently,
hours after the switch is flipped.

Docs

doc/bastion-architecture.md, doc/llng-configuration.md,
doc/security/00-architecture.md, doc/design/bastion-cert-vouching.md (its
"open question" about deprecating the endpoint is now settled) and
man/ob-bastion-id.1.

https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN

guimard added a commit that referenced this pull request Sep 6, 2026
…work

The whole audit round landed upstream on 6 September (PRs #76 to #95), so the
three sheets that said "en cours de revue" were wrong within a day of being
written. All nine tickets in scope are merged; none is published — v0.5.2 is
the last tag, these ship in 0.6.0 — so no score moves. What changes is the
nature of the R-P3/R-P7 acceptance: a publication delay with no upstream work
left, which is the weakest form that acceptance can take.

Two merged fixes are not acquired by upgrading, and the sheets now say so:

- pamAccessAllowedRps (#92) only bites if the operator fills it in, which
  means enrolling bastions under their own client_id. R-P1's remediation is
  still a configuration, not a product guarantee.
- request signing (#93) cannot be set to `required`: the client signs two of
  the six /pam/* endpoints, and /pam/heartbeat is one of the unsigned ones, so
  the fleet would fall over hours after the switch. New MT52, issue #247.

R-P6's mitigating factor survives the upgrade after all: ob-bastion-id moves
to POST /pam/whoami in #248, with a fallback for portals that have not
migrated. MT51 goes from open to in review.

Checker: 47 sheets, every matrix cell still derivable.

Claude-Session: https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN
…d endpoint

Upstream lemonldap-ng-plugins 0.6.0 removes /pam/bastion-token (#86): it
signed a JWT even when the user lookup had failed. Its `probe: true` mode was
the only way for an enrolled server to read the per-device id the portal gave
it, and #94 restores exactly that part as POST /pam/whoami, same value under
the same `bastion_id` field.

ob-bastion-id asks /pam/whoami and falls back to the legacy probe when, and
only when, a portal answers 404 — any other status, 403 included, is reported
rather than worked around. So it runs against 0.5.x and 0.6.0 alike and can go
out before the portal moves. The id itself does not change across the upgrade:
allowed_bastions files need no rewriting, and re-enrolling a bastion would
actually break them by minting a new id.

The request also loses its `curl -f`, which exited non-zero on any 4xx *and*
discarded the body, so every refusal became the same opaque "Request failed"
and the HTTP-status branch under it was unreachable. Distinguishing 404 from
403 is what the fallback rests on, so the status has to survive.

Both lab deployment scripts wrote the literal string `ob-bastion` into
allowed_bastions when the capture failed — deploy-ansible.sh including on the
Mode E path, where it is skipped by design. That literal never matches the
bastion=<id> field of a hop certificate's key-id, so every hop was refused
phases later with errors pointing at certificates. They now leave the list
empty and say so.

UPGRADE-NOTES.md is new: what to do before deploying, starting with this
upgrade, the unbound-voucher TTL going from 12h to 15min, the exact-match PAM
scope, and why pamAccessRequestSigningMode=required must wait for #247.

Verified against a mock portal: /pam/whoami 200, fallback to a probe answering
bastion_id, fallback to a probe answering a JWT, 403 refused without falling
back, 404 on both.

Closes #246

Claude-Session: https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN
…ot 404

Caught by the docker integration job, which is the only place a real portal
without the endpoint gets exercised: LemonLDAP::NG has a catch-all that serves
the portal's own login page, with a 200, for any /pam/* path no plugin has
registered. So the endpoint's absence does not look like a 404 in the common
case -- it looks like a web page -- and the fallback never fired.

It now fires on a 200 carrying no identity too. A real refusal (403 from the
caller gate, 5xx) is still reported rather than retried against the other URL,
which is the distinction the fallback exists to make.

Two smaller things the same body exposed:

- emit_id checked --verbose before it checked for an identity, so a verbose run
  against such a portal printed the HTML page as though it were an answer.
- `jwt=$(... | jq ...)` had no failure branch, so under `set -e` with pipefail
  jq exiting non-zero on a non-JSON body killed the script at that assignment,
  exit 5, no message. The final error never got a chance to say what happened.

The docker test recognises the third shape of "no endpoint here" alongside 403
and 404. Mock coverage is now 7 cases: whoami 200; fallback to a probe
answering bastion_id; fallback to a probe answering a JWT; 403 not retried;
catch-all HTML on both, reported as such; catch-all HTML then a working probe;
404 on both.

Claude-Session: https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN
Found while replaying the mock-portal scenarios after the rebase. `die()`
logged `$*`, which joins every argument — including the exit code passed
as `$2` — so each call that set one ended its message with a stray digit:

  [ERROR] this portal implements neither /pam/whoami nor the legacy
  /pam/bastion-token probe: <!DOCTYPE html>
  <html><body>portal login page</body></html> 2

The bug predates this branch, but this branch adds two more call sites
that pass a code and redefines what 2 and 3 mean, so the noise lands
squarely on the paths it introduces.

Also promotes `sshCaAdminRule` in UPGRADE-NOTES.md out of "Optional, and
worth doing" into a required step of its own. The entry contradicted its
own heading — "must be set or /ssh/admin, /ssh/certs and /ssh/revoke
answer 403" is not optional — and #238 established what the upgrade
actually does: on 0.5.x those routes have no check at all, so an upgrade
that leaves the rule unset swaps "anyone can revoke anyone's
certificate" for "nobody can revoke anything", silently, when the portal
restarts.

Verified against the seven mock-portal cases: whoami 200; fallback to a
probe answering bastion_id; fallback to a probe answering a JWT; 403 not
retried; catch-all HTML on both; catch-all HTML then a working probe;
404 on both. All seven behave as designed and no message carries a
trailing digit.

Claude-Session: https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN
@guimard
guimard force-pushed the fix/bastion-id-whoami branch from b37f844 to 3f54c38 Compare September 6, 2026 19:45

@guimard guimard left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revue du 06/09/2026 — verdict : CHANGES_REQUESTED (petits fixs) (posté en COMMENT, PR à toi).

Le cœur est sain, et je l'ai vérifié contre les deux dépôts : les 7 claims amont tiennent (/pam/whoami = PR #94, forme de réponse et 401/403 exacts ; suppression = #86 ; probe v0.5.2 renvoie bastion_id direct et saute bien _pamSeen ; pamAccessBastionVoucherUnboundTtl 900 s ; scope exact pam/pam:server ; le client C signe exactement 2/6 endpoints ; pamAccessAllowedRps conforme). Le catch-all POST sert bien le HTML en 200 (pas 302 — le catch-all qui redirige est GET-only, c'est pour ça que le script POSTe). La sémantique « liste vide = tout bastion vouché accepté » est vraie (ob-ssh-principals saute une liste vide, fichier écrit inconditionnellement). Le design du fallback est correct. Le scénario « redirect suivi par -X POST -L » est vérifié inatteignable (gate = 401/403 JSON pur, catch-all POST = 200).

À corriger (dans l'ordre)

  1. Exit 2 vs 3 sur « implements neither » (inline script:303) — le portail a répondu 200 (deux fois) sans identité : c'est le cas « 3 = answered, no identity » de la nouvelle sémantique, pas « 2 = failed/refused ». Les trois die frères du même cas utilisent bien 3 ; celui-ci est un reste de l'ancienne numérotation.
  2. Le déclencheur du fallback est mal documenté partout (inline CHANGELOG:17 ; aussi man:17 et UPGRADE-NOTES:35) — « when, and only when, a portal answers 404 » contredit le 2e paragraphe du même CHANGELOG et le code : sur LLNG l'absence se présente en 200 HTML (catch-all), c'est le déclencheur usuel ; le 404 est le cas rare.
  3. deploy-ansible.sh : un vrai échec d'ob-bastion-id est indiscernable du skip Mode E (inline :101) — failed_when:false + skip non compté dans le résumé → la panne exacte que cette PR veut rendre diagnostiquable fait un run vert avec « skip », alors que deploy-shell.sh passe au rouge pour la même condition. Au minimum : distinguer les deux chemins (le Mode E a un marqueur connu) et compter le skip de phase 1 dans le verdict.
  4. Silent exit 1 sur échec de base64url-decode (inline script:308) — sous set -e, claims=$(base64url_decode …) tue le script avant le die … 3 ; vérifié empiriquement (payload !! → rc=1 sans message, exit 3 documenté inatteignable).
  5. Commentaires périmés dans deploy-ansible.sh (inline :94) — les lignes 88-89 (« fall back to the enrolling client_id, which is what the bastion_id resolves to here ») et le heredoc deploy.yml:70-71 survivent à 6 lignes du nouveau commentaire qui dit l'inverse, et sont factuellement faux dans ce lab (device-org actif → _deviceId, pas client_id).
  6. deploy-shell.sh supprime les diagnostics au seul point qui signale l'échec (inline :58) — 2>/dev/null des deux côtés : la grammaire exit 2/3 que cette PR construit est jetée là où elle servirait.
  7. Le chemin /pam/whoami n'est exercé par rien (inline test:1104) — l'image démo est pré-0.6.0, le test ne passe que par le fallback ; quand l'image bougera, le fallback (seul chemin testé aujourd'hui) perdra son exercice. Et le grep de skip 40[34] masquerait une régression réelle qui imprime 403/404 à la transition. Ajouter un job/variant avec le plugin main (ou mocker whoami).
  8. local-test/README.md:79,87 n'a pas été mis à jour — il enseigne encore « is ob-bastion (= its client_id) », le modèle exact que cette PR supprime.
  9. man --verbose périmé (man:54, hors diff) — « Print all JWT claims as pretty JSON » alors que --verbose dump maintenant la réponse brute ; idem message du test d'intégration (« JWT claims JSON », test:1137).
  10. man footer « open-bastion 0.6.2 » vs script VERSION=0.3.0 — la page attribue un comportement non publié à une version déjà sortie (0.6.2 expédie ob-bastion-id 0.2.3 sans whoami).
  11. UPGRADE-NOTES §2 renvoie vers un doc qui ne contient pas le check (inline UPGRADE-NOTES:61) — 99-risk-reduce.md ne parle pas du spool /run/open-bastion/ssh-fp ; la procédure vit dans doc/pam-modes.md:398+.
  12. Template Ansible d'ob-builder (admin-builder/…/defaults/main.yml.in:91 + README:52) — enseigne encore « bastion_id = enrolling client_id » ; le même run qui refuse d'inventer un placeholder régénère une doc qui le recommande.
  13. Commentaires du script : le cadre « Telling 404 from 403 » (script:133) contredit la propre analyse 200-HTML de la PR (un portail upgradé ne 404 jamais — d'ailleurs la note « gets a 404 » du UPGRADING.md des plugins est fausse, la PR a raison de ne pas la suivre) ; et « Older portals answer a probe by minting a placeholder JWT » (script:291) décrit une population qui n'a jamais coexisté avec ob-bastion-id (JWT seulement pré-v0.1.17 ; v0.1.17–v0.3.1 = 403 au gate ; v0.3.2+ = JSON direct).
  14. Divers : UPGRADE-NOTES.md n'est lié ni du README ni de doc/README.md (découvrabilité) ; CHANGELOG a désormais deux sections « ### Added » non adjacentes (l.48 vs l.100 — keep-a-changelog) ; « the removed probe » (bastion-architecture.md:624) se lit mal pour les portails < 0.6.0 où il n'est justement pas retiré.

Tout le reste est exact : la valeur du bastion_id est stable à travers l'upgrade, les fichiers allowed_bastions n'ont pas besoin d'être réécrits, le séd vide produit un YAML valide et ob-builder n'émet pas de flag, et le 15-min-drop des vouchers non liés est réel et correctement décrit.

Comment thread scripts/ob-bastion-id
# non-zero on a body that is not JSON (the portal's HTML page, exactly the
# case below) kills the script before it can say anything useful.
jwt=$(printf '%s' "$BODY" | jq -r '.bastion_jwt // empty' 2>/dev/null) || jwt=""
# Neither endpoint answered with an identity. Most often both are simply

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ce die … 2 contredit la sémantique de sortie que la PR redéfinit : le portail a répondu 200 (deux fois : whoami sans identité puis bastion-token en HTML sans bastion_jwt) — c'est le cas « 3 = the portal answered, but with no identity in it » de la man page, du usage() et du CHANGELOG, pas « 2 = failed or refused … HTTP status other than 200 from the endpoint that was tried last » (le dernier endpoint a répondu 200). Les trois die frères du même cas utilisent bien 3 ; celui-ci est un reste de l'ancienne numérotation (l'ancien « No bastion_jwt in response » était 2).

Comment thread scripts/ob-bastion-id
# deployment missing the pam-access plugin looks like from here.
[ -n "$jwt" ] || die "this portal implements neither /pam/whoami nor the legacy /pam/bastion-token probe: $BODY" 2

# JWT is header.payload.signature -- decode the middle segment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sous set -euo pipefail, claims=$(base64url_decode "$b64payload") tue le script silencieusement quand base64 -d échoue (payload hors de l'alphabet base64url, JWT tronqué) : la substitution échoue avant que le die "Failed to base64url-decode JWT payload" 3 puisse s'exécuter — vérifié empiriquement (rc=1, aucun message). Le die 3 n'est atteignable que pour le cas « sortie vide avec rc=0 », et la man page (« also returned when a legacy JWT cannot be base64url-decoded ») décrit un comportement qui n'existe pas. Garder la substitution (|| die … 3 comme pour jwt= plus haut) ou tester le rc.

Comment thread scripts/ob-bastion-id
log_error "LLNG returned HTTP $HTTP_STATUS on /pam/bastion-token: $BODY"
exit 2
fi

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

« Older portals answer a probe by minting a placeholder JWT instead » ne décrit aucune population réelle : le JWT placeholder n'existe que pré-v0.1.17 (avant avril 2026, un mois avant la création d'ob-bastion-id) ; v0.1.17–v0.3.1 répondaient 403 au gate _pamSeen sur l'utilisateur de probe (d'où le commit open-bastion f0c31fa « use probe mode … avoids 403 ») ; v0.3.2+ renvoyaient le JSON direct. Le chemin JWT est un héritage de l'ancien script — le commentaire devrait le dire (et les mainteneurs futurs ne devraient ni le supprimer en le croyant mort ni le garder pour des portails qui n'ont jamais coexisté avec lui).

Comment thread CHANGELOG.md Outdated
Comment thread local-test/deploy-ansible.sh Outdated
wait
BID="$(dssh "${IP[$BASTION_VM]}" 'sudo ob-bastion-id 2>/dev/null' 2>/dev/null | tr -d '\r\n')"
[ -n "$BID" ] && ok "bastion enrolled; ob-bastion-id=$BID" || { bad "ob-bastion-id failed"; BID="ob-bastion"; }
if [ -n "$BID" ]; then

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

La capture supprime les deux couches de stderr (sudo ob-bastion-id 2>/dev/null dans un dssh … 2>/dev/null) : quand le nouveau script échoue, toute la grammaire de diagnostic que cette PR ajoute (2 = refusé, 3 = répondu sans identité, « HTTP 403 on /pam/whoami » vs « implements neither ») est jetée au seul point d'appel qui signale l'échec, et l'opérateur ne voit que « ob-bastion-id failed — allowed_bastions left EMPTY ». Garder le stderr (au moins dans le log de la phase).

if echo "$id" | grep -qE 'HTTP[/ ]403|HTTP_STATUS__:403|"status"[[:space:]]*:[[:space:]]*403'; then
log_warn "/pam/bastion-token returned 403 — endpoint not provisioned in this demo"
# Three ways this demo can legitimately have no endpoint to answer:
#

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trou de couverture sur le changement principal de la PR : le chemin « /pam/whoami répond 200 avec identité » n'est exercé par rien — l'image démo est pré-0.6.0, donc le test ne passe que par le fallback legacy (qui réussit : bastion_id=pam-access). Quand l'image yadd passera à 0.6.0, c'est le fallback — le seul chemin testé aujourd'hui — qui perdra son exercice, et le grep de skip 40[34] masquerait une régression réelle qui imprime 403/404 à la transition (CI verte avec ob-bastion-id entièrement cassé). Ajouter un variant avec le plugin main (ou mocker /pam/whoami).

Comment thread man/ob-bastion-id.1 Outdated
Comment thread UPGRADE-NOTES.md Outdated
Comment thread CHANGELOG.md
and say so: weaker (an empty list accepts any vouched bastion) but honest,
and it keeps the rest of the run diagnosable.

### Added

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deux sections « ### Added » non adjacentes dans [Unreleased] : celle-ci (l.48, UPGRADE-NOTES) et celle de #178 (l.100), séparées par « ### Security ». keep-a-changelog (cité dans l'en-tête du fichier) exige une section par type — joindre l'entrée à la section Added existante, ou déplacer celle-ci en dessous.

The review's central point was that the change this PR is about had no
coverage: the docker integration test only reaches whichever path the
published demo image happens to take, which today is the legacy
fallback, so `/pam/whoami` answering with an identity was exercised by
nothing. Worse, the day that image moves to 0.6.0 the fallback loses its
only exercise too, and the skip-on-403/404 branch there would report a
real regression as "endpoint not provisioned in this demo".

So `tests/test_ob_bastion_id.sh` replays the command against a mock
portal in every shape it has to survive: whoami with an identity, the
legacy probe answering an id, the legacy probe answering a JWT,
LemonLDAP::NG's catch-all HTML on one endpoint or on both, a 403, a 404
on both, and a JWT whose payload is not decodable. CI picks it up from
the existing `tests/test_ob_*.sh` loop.

Writing it found two of the three defects below, and the review found
the third.

Exit code. A portal answering 200 on both endpoints without an identity
exited 2, "the request failed or was refused". Nothing failed or was
refused -- both answered 200. That is 3, "the portal answered, but with
no identity in it", which its three sibling errors already used.

Undecodable JWT. `claims=$(base64url_decode "$b64payload")` dies at the
assignment under `set -e` with `pipefail`, so the `die "Failed to
base64url-decode JWT payload" 3` below it never ran: the caller got rc=1
and no message, and the man page documented a diagnostic that could not
happen. Same `|| claims=""` treatment as the `jwt=""` above it, which
was already load-bearing for the same reason.

The JWT path's comment claimed "older portals answer a probe by minting
a placeholder JWT". No such population: upstream minted a JWT for this
call only up to v0.1.16; v0.1.17 added the `_pamSeen` gate, so those
answer a synthetic probe user 403; probe mode landed before v0.3.2 and
has returned the id as JSON since. It is a legacy of the original
script, and the comment now says so -- keep it, do not extend it, do not
delete it believing it covers something current.

Three texts said the fallback triggers "only when a portal answers 404",
contradicted by the next paragraph of the same CHANGELOG entry and by
the code. Absence usually looks like a 200 carrying the portal's HTML.
Fixed in the CHANGELOG, the man page and UPGRADE-NOTES.

The lab harnesses could not report any of this.
`deploy-shell.sh` discarded stderr twice over -- `sudo ob-bastion-id
2>/dev/null` inside `dssh ... 2>/dev/null` -- at the one call site that
reports the failure, so the 2-versus-3 grammar this PR builds was thrown
away and the operator got "ob-bastion-id failed" and nothing else.
`deploy-ansible.sh` was worse: its capture task carried `failed_when:
false`, so a real failure left `bastion-id.txt` absent and landed in the
Mode E `skip()`, which only increments SKIP while `summary()` returns on
FAIL -- the run went green with the hop allowlist untested, where
`deploy-shell.sh` goes red for the same condition. The playbook now
ships the rc and stderr out even on failure, and the harness tells the
two apart.

While there: the Mode E guard tested `ob_max_security`, a variable set
nowhere in the repository, so it never fired. It is passed from the
scenario now. And two comments describing the removed client_id fallback
-- asserting an equality between client_id and bastion_id that #246's
premise denies -- survived ten lines from the block that replaced them.

Also promotes `sshCaAdminRule` in UPGRADE-NOTES.md out of "Optional, and
worth doing", which contradicted its own text, into a required step; the
spool check now points at doc/pam-modes.md, which documents the spool,
rather than 99-risk-reduce.md, which does not; and the second `###
Added` section this branch created in [Unreleased] is merged into the
existing one. (The other duplicate sections there predate this branch.)

Last, the header of tests/test_ob_llng_location_rules.sh still carried
the two mechanisms #238 corrected everywhere else -- "`^/device$` never
matches" and "restricting that route breaks revocation propagation
fleet-wide". Both false, both fixed.

Verified: the new suite 10/10, with the exit-code and base64 fixes each
mutation-checked to fail with the right symptom (rc=1 and no output;
a message ending in its own exit code); all 23 tests/test_ob_*.sh green;
shellcheck clean on every script touched.

Claude-Session: https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN
Comment thread tests/mock_portal_whoami.py Fixed
github-code-quality flagged do_POST for mixing explicit and implicit
returns. It is right, and the idiom was worse than inconsistent: every
branch read `return self._json(...)`, which returns None because the
helper does, so the `return` conveyed nothing and the one branch without
it looked like an oversight rather than the fall-through it was.

Response selection now lives in `_response_for()`, which returns a
(status, body) pair on every path, and `do_POST` sends it. No branch of
either returns a value that is not used.

Claude-Session: https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN
@guimard

guimard commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

Les onze points tiennent, vérifiés à la source. Corrigés dans fdde95b, plus f... pour le retour du bot. Une imprécision de référence signalée en fin de message.

Le trou de couverture était le vrai sujet

Vous avez raison, et c'était pire : le changement principal de la PR n'était exercé par rien. tests/test_ob_bastion_id.sh rejoue maintenant la commande contre un portail mock dans les huit formes qu'elle doit encaisser — whoami avec identité, probe legacy répondant un id, probe legacy répondant un JWT, catch-all HTML sur un endpoint ou sur les deux, 403, 404 sur les deux, et un JWT au payload indécodable — plus le contrat des codes de sortie et --json. La CI le ramasse via la boucle tests/test_ob_*.sh existante.

Écrire ce test a trouvé deux des trois défauts ci-dessous ; le troisième est le vôtre.

Codes de sortie et JWT

die … 2 → 3. Confirmé : les deux endpoints ont répondu 200, rien n'a échoué ni été refusé. Ses trois erreurs sœurs utilisaient déjà 3. C'était bien un reste de l'ancienne numérotation.

claims=$(base64url_decode …). Confirmé empiriquement — rc=1, aucun message. La fonction se termine par un pipe, donc sous set -e/pipefail l'affectation tue le script avant son propre die. Même traitement || claims="" que le jwt="" au-dessus, qui était déjà porteur pour la même raison. La ligne « also returned when a legacy JWT cannot be base64url-decoded » de la page de manuel décrit désormais un comportement qui existe, et le cas est couvert par le test legacy-badjwt.

Le commentaire JWT. Confirmé dans l'historique amont : e0daf16 introduit le gate _pamSeen (présent en v0.1.17), et le probe mode arrive avec b11251f, avant v0.3.2. La population « JWT » s'arrête donc à v0.1.16. Le commentaire dit maintenant que c'est un héritage du script d'origine, et dit explicitement de ne pas l'étendre ni de le supprimer en le croyant courant.

« 404 seulement »

Confirmé aux trois endroits, et corrigé aux trois. Le paragraphe suivant de la même entrée de CHANGELOG disait déjà l'inverse.

Les harnais de lab

deploy-shell.sh : les deux couches de stderr sont conservées, et le message est affiché sous le bad.

deploy-ansible.sh : confirmé, et c'était pire que « indiscernable ». En corrigeant j'ai trouvé que le garde Mode E testait ob_max_security, une variable définie nulle part dans le dépôtgrep -rn ob_max_security ne renvoie que la ligne du when: elle-même. Le garde n'a donc jamais fonctionné. Il est maintenant passé depuis le scénario. Le playbook exporte le rc et le stderr même en échec, et le harnais distingue « sauté parce que Mode E » (skip, condition connue) de « a tourné et a échoué » (bad, avec le stderr), comme le fait déjà deploy-shell.sh.

Les deux points de doc

sshCaAdminRule sort de « Optional, and worth doing » — qui contredisait son propre texte — vers une section requise décrivant ce que la mise à jour fait réellement.

Sur la référence du spool : votre prémisse est légèrement inexacte, 99-risk-reduce.md mentionne bien ob-ssh-principals (quatre fois, l.160-167). Mais le fond tient entièrement — il n'y a là ni le chemin /run/open-bastion/ssh-fp, ni aucune procédure opérateur. Le lien pointe maintenant vers doc/pam-modes.md pour le mécanisme et doc/security/02-ssh-connection.md pour la justification.

CHANGELOG

Le doublon ### Added était bien créé par cette branche : fusionné. À noter pour plus tard — [Unreleased] sur main porte déjà 3× ### Changed, 2× ### Security et 2× ### Removed ; c'est pré-existant et je l'ai laissé hors périmètre.

Hors revue

L'en-tête de tests/test_ob_llng_location_rules.sh portait encore les deux mécanismes que #238 a corrigés partout ailleurs (« ^/device$ never matches », « breaks revocation propagation fleet-wide »). J'avais corrigé le commentaire du test 3 et manqué le bandeau du fichier. Rattrapé ici.

Et le retour de github-code-quality sur le mock : justifié, et l'idiome était pire qu'incohérent — return self._json(...) ne renvoie rien puisque l'aide ne renvoie rien, donc le return ne portait aucune information et la seule branche qui n'en avait pas ressemblait à un oubli. La sélection de réponse est passée dans _response_for(), qui renvoie un couple (status, body) sur tous les chemins.


Mutation-tests des deux corrections :

M1 (|| claims="" retiré)   -> FAIL: legacy-badjwt - rc=1 expected 3; output:
M2 (die "$*" rétabli)      -> FAIL: no error message ends with its own exit code
                              - <html><body>portal login page</body></html> 3

Vérifications : nouvelle suite 10/10 ; les 23 tests/test_ob_*.sh verts ; python3 tests/ebios_matrix_check.pyOK: 47 risk sheets ; shellcheck -S warning propre sur tous les scripts touchés ; page de manuel rendue sans avertissement.

https://claude.ai/code/session_011q88Fs4nFuUs7JMvmgbBTN

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ob-bastion-id breaks when /pam/bastion-token is removed, and deploy-shell.sh silently writes a wrong bastion id when it does

1 participant