-
Notifications
You must be signed in to change notification settings - Fork 54
feat: add support for signing and verifying OCI image manifests #571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/model_signing/_cli.py
Outdated
|
|
||
| def _detect_path_type( | ||
| path: pathlib.Path, | ||
| ) -> tuple[Optional[pathlib.Path], Optional[pathlib.Path]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think introducing a path type constant would make sense?
Example: path_type="MODEL_DIR" or "OCI" ; path=/something/xyz
Trade Off:
Possible downside: Maintain different constants ;
Alternatively thinking from extensibility point of view, if signing and verifying another kind of model path is introduced in future, it would be easy to segregate the model path specific logic.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple enum for the type of the path would work here. Then we can make this function return the detected type.
We can also just wrap the path into one of two classes (one for real paths, one for the manifest path) and move the reading behavior into a method in the class.
Right now we're doing json.load twice for the same file, maybe we should not do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, looks much cleaner now using Enum instead. Thank you for the suggestion!
| with open(path, "r", encoding="utf-8") as f: | ||
| data = json.load(f) | ||
| if isinstance(data, dict) and ( | ||
| "layers" in data or "schemaVersion" in data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pydantic schema validation would be a good alternative solution.
This ensures that we maintain a schema that we can always validate against and easy for maintaining config versioning against model transparency versioning.
Again, in future if the underlying schema changes - model transparency can validate against specific schema versions and still go through with the signing/verifying process.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to migrate to pydantic, +1.
For the 1.0 release we wanted to get to a stable API, but for the next stable release we should migrate to pydantic and to our own error hierarchy, rather than just reusing the Python one (to make it easier to distinguish between errors in the signing/verification flows versus errors due to incorrect usage)
src/model_signing/_cli.py
Outdated
|
|
||
| def _detect_path_type( | ||
| path: pathlib.Path, | ||
| ) -> tuple[Optional[pathlib.Path], Optional[pathlib.Path]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple enum for the type of the path would work here. Then we can make this function return the detected type.
We can also just wrap the path into one of two classes (one for real paths, one for the manifest path) and move the reading behavior into a method in the class.
Right now we're doing json.load twice for the same file, maybe we should not do that?
Add the ability to sign and verify model artifacts directly from their manifest JSON files without requiring the model files on disk. This enables signing model artifacts in registries without pulling them, and supports cross-verification between OCI manifests and local model directories. Signed-off-by: SequeI <[email protected]>
Signed-off-by: SequeI <[email protected]>
Summary
This feature introduces support for handling OCI model manifests (obtained via tools like
skopeo inspect --raw) directly in the signing and verification process.I had 2 reasons for adding this:
Closes #568
Thank you :)
Checklist