Skip to content
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

Merged
merged 8 commits into from
Aug 7, 2023

Conversation

jleightcap
Copy link
Contributor

@jleightcap jleightcap commented Aug 2, 2023

Summary

Note: blocked by #121.
Compiles Rust bindings from JSON schemas.

Release Note

Documentation

Closes #115.

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>
@jleightcap
Copy link
Contributor Author

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 gen/pb-rust.

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 🙃

@jleightcap
Copy link
Contributor Author

jleightcap commented Aug 2, 2023

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:

/// NOTE(jleightcap): a method to include all JSON schemas is not immediately obvious to me:
///
/// - `schemafy!` is a direct 1:1 compilation to Rust structures of each definition in "defintions",
/// - each schema is 'standalone': including definitions of all it's dependencies,
/// as a result, the Rust generated structures have name collisions if included in the same module scope.
///
/// prefixing works,
///
/// ```rust
/// mod CertificateIdentity {
/// schemafy::schemafy!("schemas/CertificateIdentity.shema.json")
/// }
/// mod CertificateIdentities {
/// schemafy::schemafy!("schemas/CertificateIdentities.schema.json")
/// }
/// ```
/// but is clunky to use and manual to generate.
///
/// a more general approach is to 'flatten' each JSON Schema,
/// unioning all of the "defintions" fields.
///
/// Since standardized bundles is the singular motiviation for these Rust bindings,
/// for now we're punting this issue.

(per conversation with @woodruffw)

@woodruffw
Copy link
Member

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 🙃

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 cargo and in-tree resources, which requires two copies.

@jleightcap jleightcap force-pushed the jl/pb-rust-schemafy branch 2 times, most recently from d6bcc8f to 00831d6 Compare August 2, 2023 19:30
@jleightcap
Copy link
Contributor Author

jleightcap commented Aug 2, 2023

there's a chance to investigate #115 as this crystalizes.

gen/pb-rust/tests/unit.rs Outdated Show resolved Hide resolved
@jleightcap
Copy link
Contributor Author

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.

@jleightcap
Copy link
Contributor Author

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.

Comment on lines +24 to +25
use serde::{Deserialize, Serialize};
schemafy::schemafy!("schemas/Bundle.schema.json");
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/sigstore/protobuf-specs/pull/118/files#diff-3e6cc2e74ad94dd29d10a8e8df25be38bd688d5e5f0105720695335005100ec6R1-R5

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>
Comment on lines 27 to 39
"enum": [
"HASH_ALGORITHM_UNSPECIFIED",
0,
"SHA2_256",
1
],
"oneOf": [
{
"type": "string"
},
{
"type": "integer"
}
Copy link
Member

@woodruffw woodruffw Aug 4, 2023

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?

Copy link
Contributor Author

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>
}),
message_signature: Some(MessageSignature {
message_digest: Some(HashOutput{
algorithm: Some(HashAlgorithm::Variant0(String::from("SHA2_256"))),
Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Jack Leightcap <jack.leightcap@trailofbits.com>
@woodruffw
Copy link
Member

LGTM. I'd like it if the doctests also pass, but I'll look at that in a follow-up.

@woodruffw woodruffw merged commit 88fe2fb into sigstore:main Aug 7, 2023
7 checks passed
@woodruffw woodruffw deleted the jl/pb-rust-schemafy branch August 7, 2023 21:25
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.

Optimize the Rust codegen
2 participants