-
Notifications
You must be signed in to change notification settings - Fork 31
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
pb-rust: JSON schema compilation source #118
Conversation
includes the modules structure definitions needed have full package prefixes, like ```rust use sigstore_protobuf_specs::dev::sigstore::verification::v1::Input ``` Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
Note: I believe JIT compiling the JSON schema locally to the Rust crate requires committing the schemas a second time: the packaging requires all of the source data to be self-contained in I've added relevant rules to the Makefile to sync the two, let me know if that automates this enough. Or if I'm totally off-base 🙃 |
0421499
to
e8f2d78
Compare
This PR does not provide complete Rust bindings, instead binding just enough to unblock parent sigstore-rs issue (sigstore/sigstore-rs#280). For future work, see note: protobuf-specs/gen/pb-rust/src/lib.rs Lines 3 to 25 in e8f2d78
(per conversation with @woodruffw) |
I'm fine with that for now -- ideally we'd only have one copy here, but I'd rather we do this the "right" way with |
d6bcc8f
to
00831d6
Compare
there's a chance to investigate #115 as this crystalizes. |
There's currently an annoying naming convention discrepancy when attempting to consume sigstore-python artifacts directly; see #67 (comment). For the moment the integration tests uses a sigstore-python bundle but manually edited to use snake_case. |
I must have just had some vestigital state around, doing a clean rebuild forcing the use of camelCase with both the schema and artifact seems to have resolved compatibility issues with other sigstore artifacts. |
00831d6
to
4baf1ac
Compare
use serde::{Deserialize, Serialize}; | ||
schemafy::schemafy!("schemas/Bundle.schema.json"); |
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.
If I'm understanding correctly, this ends up in the current module namespace and not a nested one, right? Fine if so, just making sure.
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.
Correct, see the use in the hand-written tests for usage.
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.
The names are compiled from the dev.sigstore.{...}
strings into VeryLongCamelCaseStrings -- annoying, but with an as
I don't think it's too prohibitive for use in sigstore-rs.
Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com> pb-rust: schemafy construction Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
adapted from sigstore@b7d905d Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
4baf1ac
to
31c3ad9
Compare
"enum": [ | ||
"HASH_ALGORITHM_UNSPECIFIED", | ||
0, | ||
"SHA2_256", | ||
1 | ||
], | ||
"oneOf": [ | ||
{ | ||
"type": "string" | ||
}, | ||
{ | ||
"type": "integer" | ||
} |
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.
Are we missing enums_as_strings
? Or is this schema out-of-date possibly?
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.
Silly bug. (f21f9ef#diff-fe6f382153307cf3287026377c362727d9ec012c8681090c32a1d93255a652d7R9)
Added the updated schemas to this PR.
Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
gen/pb-rust/tests/integration.rs
Outdated
}), | ||
message_signature: Some(MessageSignature { | ||
message_digest: Some(HashOutput{ | ||
algorithm: Some(HashAlgorithm::Variant0(String::from("SHA2_256"))), |
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.
Is this test running? I'd expect it to fail now.
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.
Yeah, wasn't caught -- root issue here is not running the test suite in CI. (1b22bff)
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.
Noting:
$ cargo test
(cut)
Doc-tests sigstore_protobuf_specs
running 3 tests
test src/lib.rs - Serialize (line 9) ... FAILED
test src/lib.rs - Deserialize (line 9) ... FAILED
test src/lib.rs - (line 9) ... FAILED
The macro expansion seems to result in some failing doc tests? I can look into it if that's important, for the moment I have CI just run the test suite with cargo test --tests
.
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.
Now passing test suite: https://github.com/trail-of-forks/protobuf-specs/actions/runs/5789830447/job/15691699889
Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
LGTM. I'd like it if the doctests also pass, but I'll look at that in a follow-up. |
Summary
Note: blocked by #121.
Compiles Rust bindings from JSON schemas.
Release Note
Documentation
Closes #115.