8000 fix(cbio): require injected vendor material instead of compiled constants by shroominic · Pull Request #166 · shroominic/OpenGlucose · GitHub
[go: up one dir, main page]

Skip to content

fix(cbio): require injected vendor material instead of compiled constants - #166

Closed
shroominic wants to merge 1 commit into
feature/cbio-gs1-glucose-livefrom
fix/cbio-credential-redaction
Closed

shroominic wants to merge 1 commit into
feature/cbio-gs1-glucose-livefrom
fix/cbio-credential-redaction

Conversation

@shroominic
@shroominic shroominic commented Sep 18, 2026 •
Copy link
Copy Markdown
Owner

Fixes #164, addresses #158. Base is feature/cbio-gs1-glucose-live, the branch that introduced the material and the head of #136.

What was wrong

packages/cgm_cbio/lib/src/cbio_crypto.dart carried the vendor GS1 per-frame RC4 stream key, the 16-byte link credential, and the five-byte authentication prompt as plaintext List<int> constants. The repository is public, the file is on origin/feature/cbio-gs1-glucose-live, origin/feature/cbio-gs1-app-live, and the 0.4.0 aggregates, and it compiles into the artifacts built from those refs — the tree scan finds it in app.dill and kernel_blob.bin as well as in source. No byte is reproduced here.

What this changes

  • No compiled material. cbio_credentials.dart adds CbioCredentials (validated, immutable, toString renders nothing) and a CbioCredentialSource boundary with CbioMapCredentialSource and CbioDefineCredentialSource. cbioRc4Keystream, maskCbioFrame, unmaskCbioFrame, and every builder in cbio_vendor_frames.dart now take the key and material as required arguments, so a caller that has not resolved material cannot compile a call.
  • A clear failure mode. CbioCredentialUnavailable names what is missing, and the authenticated integration harness aborts with CBIO-A abort=vendor-material-missing before it touches the radio.
  • A documented, redacted loading path. packages/cgm_cbio/README.md documents --dart-define-from-file against a git-ignored local file. No byte is recorded in the package or in this body.
  • The tests no longer carry key-shaped literals. The fixture that trips the generic ruleset is derived from synthetic material instead of being written out, and the two inline hex spellings are composed at run time.
  • The branch-local scan config is gone. This branch no longer adds a .gitleaks.toml. The canonical history policy stays on main (fix(security): scope the scheduled secret sweep to a real finding #150) and the tip guard lives in security/gitleaks-vendor-material.toml (fix(security): prove and scope the cgm_cbio vendor-material guard #173), so there is one policy rather than three competing ones.

Verification

Command Result
dart test in packages/cgm_cbio All tests passed! (62 passing, 2 skipped)
gitleaks detect --no-git --source . --redact (default ruleset, this tree) no leaks found
gitleaks detect --no-git --source <this tree> --config security/gitleaks-vendor-material.toml exit 0, 0 findings
Same guard rule against the pre-fix tree (feature/cbio-gs1-glucose-live) exit 2, 31 findings, including the source constant and compiled app.dill / kernel_blob.bin
gitleaks detect (whole clone) against the same commit reported _authMasked and a streamKey: literal, not the material constants. Reproduces why this gate was never the guard it looked like.

Decisions taken

  • Injection over redaction. A masked or encoded constant is not a fix, so the material is removed from the tree rather than re-encoded.
  • Fail closed, with a named reason. A build without material cannot authenticate a link and says so, instead of falling back to a compiled default.
  • Environment and defines are both supported, and the define file is the recommended path, because it keeps the values out of a shell history entry and out of a committed file.
  • Rewrote this branch rather than layering a fix on top. The first version of this commit reintroduced a key-shaped literal in its own tests. That commit is superseded and its tip is clean, so the literal no longer exists as this lane's tip.
  • Deleted the lane's own scan config. Guard policy belongs on main where every branch inherits it.

Decision request

Whether vendor-derived authentication material may ship at all is an owner decision. The vendor's
interoperability credential is not rotatable by us, the vendor application is public, and the
material has already been published on a public branch. Two consequential options are (a) treat it
as a support-keying problem and keep the injected-material path for signed bench and support builds
only, or (b) keep it out of shipped builds entirely and let the authenticated GS1 link remain a
bench-only capability until the vendor exposes a per-device or per-account binding. This pull request
is deliberately neutral between them: it removes the compiled material and fails closed either way.

…ants

The vendor GS1 stream key, link credential, and authentication prompt were
plaintext constants in packages/cgm_cbio/lib/src/cbio_crypto.dart. The
repository is public, the file is on the 0.4.0 release aggregate, and the
Secret Scan job is red on the same lane for a fixture derived from it.

Remove the constants and make the material an explicit input:

* cbio_credentials.dart adds CbioCredentials (validated, immutable, redacting
  toString), CbioCredentialSource, CbioMapCredentialSource,
  CbioDefineCredentialSource, and the fail-closed CbioCredentialUnavailable.
* cbioRc4Keystream, maskCbioFrame, unmaskCbioFrame, and every builder in
  cbio_vendor_frames.dart now take the key and material as required arguments,
  so a caller that has not resolved material cannot compile a call.
* the device harness reads the material from the process environment or
  --dart-define and aborts with vendor-material-missing before touching radio.
* tests use clearly synthetic fixtures; the real-material checks skip unless
  the values are injected.
* .gitleaks.toml adds a rule for this exact class of literal and a narrow
  allowlist for synthetic fixtures only.

Refs #164
@shroominic
shroominic force-pushed the fix/cbio-credential-redaction branch from 3be16fc to 7fa477c Compare September 18, 2026 04:37
shroominic added a commit that referenced this pull request Sep 18, 2026
Absorbs feature/cbio-gs1-app-live at 05785b0, which carries the redacted
credential API (#166 verbatim as a7c21bc) and the live lane's adoption of an
injected CbioCredentialSource (PR #192, issue #191).

Conflict resolution:
- packages/cgm_cbio/README.md: union. The aggregate's FF31 reply-decode
  paragraph (#132) and the lane's new injected-material section are both kept;
  neither lineage had the other.
- packages/cgm_cbio/test/cbio_crypto_test.dart: take the lane's version. It
  supersedes the masked-vector fixture from #148 by removing the published
  vectors entirely and testing the mask with synthetic material.
shroominic added a commit that referenced this pull request Sep 18, 2026
…edential API

Merges fix/bounded-automatic-reconnect (#182). The branch forked at 17db5b4, so
its session conflict re-introduced the compiled credential provider that #166
removed. Resolution keeps the aggregate's injected-credential API and ports
only the new logic:

- cbio_FailureAllowsAutomaticReconnect is added; the provider classes stay out.
- The session's reconnect-allowed field, metadata marker, and _fail gate apply
  unchanged.
- app_controller.dart and the persistence test are unions: the aggregate's
  sync-stage deadline tracking and the wedged-scan test both stay.

No credential, address, or vendor material is added by this merge.
shroominic added a commit that referenced this pull request Sep 18, 2026
…andidate

Reverts the merge of fix/cbio-archive-unit-claim (#168).

#168 removes CbioRawGlucoseRecord.derivedMilligramsPerDecilitre and
.derivedMillimolesPerLitre and leaves only an explicitly unitless
rawCurrentScaled. The app-live surface still fills CgmReading.valueMgdl from the
removed mg/dL accessor, so the merge leaves six analyzer errors in cgm_cbio and,
fixing them, would change the number the live surface renders.

That is a unit-reference decision on the product surface, not a mechanical
merge, and it is the owner's to make. The lane needs an adoption change (as
#192 was for #166) before this can land. Recorded as excluded in the PR body.
@shroominic
Copy link
Copy Markdown
Owner Author

Merge-order note for this stack, from the evidence lane (2026-09-18).

#185 should land first, then #166 rebases onto it. Neither order is conflict-free — git merge-tree --write-tree conflicts on openhealth/integration_test/cbio_glucose_authenticated_test.dart either way — so the decision is which rebase is smaller and safer, and that is not symmetric:

Landing #185 first leaves #166 a mechanical re-application — add the guarded credential block, then thread the required key: through the call sites. Landing #166 first makes #185 re-derive its entire rewrite on top of the credential plumbing. Both end with the same tree; only the cost and the review burden differ.

One ordering constraint that does not depend on this: #166 removes a default parameter from unmaskCbioFrame, so it must not land on a branch where any caller still uses the old signature. Whichever PR goes second has to update the call sites in the same commit.

If the security fix has to land sooner than #185 can be reviewed, #166 can go first — that is a deliberate trade of a larger rebase in #185 for earlier removal of the committed vendor material, not a correctness problem.

shroominic added a commit that referenced this pull request Sep 18, 2026
The Secret Scan gate is red on this branch's review range with one
generic-api-key finding at 36a0b2a: the 48-character masked-frame fixture
in packages/cgm_cbio/test/cbio_crypto_test.dart.

The driver no longer publishes or compiles vendor material, so the literal
exists only in that commit's history (#166 replaced the fixture with
obviously synthetic bytes and an injected credential source). The default
ruleset stays in force and the allowlist is scoped to that commit, that file,
and that assignment, so the sweep still reports every other finding.
@shroominic
Copy link
Copy Markdown
Owner Author

Carried by the GS1 consolidation in PR #209 (branch feature/cbio-gs1, head 3eca4f4).

This PR is contained in the consolidated line; the merge that carries it is a7c21bc.

Closing as subsumed by the consolidation - not as won't fix. The per-defect evidence stays in this thread, and the carrying commit above is where the change now lives.

@shroominic shroominic closed this Sep 18, 2026
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.

1 participant

0