Kristian Rickert created OPENNLP-1901:
-----------------------------------------
Summary: Verify and pin teacher model downloads in
opennlp-embeddings
Key: OPENNLP-1901
URL: https://issues.apache.org/jira/browse/OPENNLP-1901
Project: OpenNLP
Issue Type: Improvement
Reporter: Kristian Rickert
{panel:title=Status|borderStyle=solid}
The implementation is already written and pushed to the OPENNLP-1877 branch (PR
#1152); this ticket records the change rather than requesting it. Nothing here
is a defect in a released version: {{opennlp-embeddings}} does not exist in
{{main}} and shipped in neither 3.0.0-M4 nor 3.0.0-M5.
{panel}
h2. Why
The distiller added in OPENNLP-1877 can name its teacher by hub id, in which
case {{HuggingFaceModelCache}} downloads the teacher's {{tokenizer.json}} and
{{onnx/model.onnx}} and the distiller then executes that ONNX graph. As first
written it fetched from {{/resolve/main/}}, a mutable ref, and verified nothing.
That is out of step with the project on two counts.
{{DownloadUtil}} has always fetched a {{.sha512}} sidecar, computed SHA-512
over the download, and thrown {{IOException}} both on mismatch and when the
checksum cannot be retrieved at all. OpenNLP already refuses to use a model it
cannot verify.
The security model published in OPENNLP-1900 states the same thing as a
property of the project: under "Downloaded models" it records the trust level
as "trusted, integrity-checked in transit," and it names as reportable
weaknesses "a checksum that is not actually checked," "a redirect that bypasses
verification," and "a path traversal in how a downloaded artifact is named or
cached." It also says ONNX models should be treated exactly as native
libraries, since OpenNLP adds no sandbox of its own.
An unverified download path in {{opennlp-embeddings}} would have made that
document inaccurate on the day the module shipped.
h2. What changed
* The teacher's ref is resolved to a commit sha once, from the hub's
{{x-repo-commit}} header, and every file is then requested at
{{/resolve/<sha>/}}. A force-push midway can no longer mix two revisions into
one cache directory. A teacher may now name a revision explicitly as
{{org/model@revision}}.
* Every file is verified against the digest the hub publishes in
{{x-linked-etag}}, choosing the algorithm by hex length: 40 is the git blob
SHA-1 over {{"blob <len>\\0"}} and the bytes, 64 is the SHA-256 of the content.
The digest is computed over the bytes written to disk. A mismatch deletes the
partial file and throws, in {{DownloadUtil}}'s wording.
* A file whose digest the hub does not publish is refused rather than used,
which is the point of the change and matches {{DownloadUtil}}'s posture.
* The resolved commit is recorded in {{.opennlp-revision}} in the cache
directory, and carried into the distilled model's {{config.json}} as
{{teacher_revision}}, so a table can name the teacher revision it came from.
Two findings from the review of the change are worth recording, because both
map onto weaknesses OPENNLP-1900 explicitly asks to hear about:
* *A redirect that bypassed verification.* With {{Redirect.NORMAL}} the final
CDN response carries neither the commit nor the etag header; only the
redirecting hub response does. Headers are now read by walking
{{HttpResponse.previousResponse()}} back to the origin hop. A test fails if
that walk is removed.
* *Cache naming that was not injective.* The directory name replaced {{/}},
{{.}} and {{@}} without distinguishing them, so {{acme/model@v1}},
{{acme/model.v1}} and {{acme/model_v1}} all mapped to one directory. Because a
complete cache directory is reused without contacting the hub, one teacher
could have been served another repository's files. The name now carries a
digest of the exact reference.
h2. Verification
Tests need no network: a loopback {{com.sun.net.httpserver.HttpServer}} serves
canned replies and records the requests made. Coverage includes both digest
forms, a corrupted body, a missing and six malformed etags, optional and
required 404s, the redirect path, the zero request cached path, revision
pinning, and the directory collision above. The git blob helper is pinned
against a digest produced by {{git hash-object}} over the same bytes, and a
separate test proves the plain SHA-1 of that content is rejected.
Module tests: 310, none skipped.
h2. Not addressed here
Two related decisions on the same PR are deliberately left open, since they are
design calls rather than integrity gaps:
* {{onnxruntime}} is a compile-scope, non-optional dependency, so consumers who
only load a distilled table pull a 42 MB native runtime they never call.
* The {{embeddings}} launcher hardcodes {{-Xmx1024m}}, which cannot run the
{{BAAI/bge-m3}} example the training guide opens with.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
