Skip to content

Conversation

@andrewjstone
Copy link
Contributor

Fixes #85

This PR makes two changes that allow this code to act as a library and better support testing in other software like Omicron and Sprockets.

  1. It moves all the logic for validating a Document and writing files out of main.rs and into ValidDocument.rs.
  2. It provides support for programatically generating sprockets configs. Correctness is checked via a test that compares the example config copied over from the sprockets repo with one generated by the new sprockets::generate_config API. To make this work, I modified the serial numbers in the example to be match the generation scheme.

I made some other small changes to better support a library like allowing a path to be input in certain places. Relatedly, I changed to use camino rather than std::path like most of our other software.

Fixes #85

This PR makes two changes that allow this code to act as a library and
better support testing in other software like Omicron and Sprockets.

  1. It moves all the logic for validating a `Document` and writing
files out of `main.rs` and into `ValidDocument.rs`.
  2. It provides support for programatically generating sprockets
configs. Correctness is checked via a test that compares the example
config copied over from the sprockets repo with one generated by the
new `sprockets::generate_config` API. To make this work, I modified the
serial numbers in the example to be match the generation scheme.

I made some other small changes to better support a library like
allowing a path to be input in certain places. Relatedly, I changed to
use `camino` rather than `std::path` like most of our other software.
@andrewjstone andrewjstone requested a review from flihp October 2, 2025 00:21
Cargo.toml Outdated
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
camino = { version = "1.2.0", features = ["serde1"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the configs that we deserialize include any file paths so the serde1 feature probably isn't required. This commit builds fine w/o it and the "sprockets" config you've added can be used to generate keys / certs / cert lists without it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 588a6da

/// In order to prevent mutation, all fields on a `ValidDocument` are private.
///
/// https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/
#[derive(Debug, PartialEq, Eq)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the equality derives here necessary? I got here by following these same derive macros that are added to the ValidDocuments members but I can't find any place where we're comparing any of these types for equality. I can't imagine that adding this is harmful so it's not like I object to deriving these traits, just trying to cut through the noise I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are used in the tests to check that the constructed documents match the parsed kdl.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just came back to correct my comment after getting to that code, thanks for clarifying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though I removed the test, I left the derives because I'm going to put the test in sprockets.

Copy link
Collaborator

@flihp flihp left a comment

Choose a reason for hiding this comment

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

There's lots to like in this PR: I'd gladly merge the Path -> Utf8Path and the ValidDocument refactor immediately. I'm less certain about the sprockets specific test code.

@andrewjstone
Copy link
Contributor Author

There's lots to like in this PR: I'd gladly merge the Path -> Utf8Path and the ValidDocument refactor immediately. I'm less certain about the sprockets specific test code.

I removed all the sprockets related stuff and will put it in sprockets as we discussed. I think this is ready to go.

andrewjstone added a commit to oxidecomputer/sprockets that referenced this pull request Oct 9, 2025
This PR allows certs to be generated for multiple nodes so that we can
spin up arbitrary test clusters for trust quorum testing in Omicron.

A small change was made to the example `config.kdl` to better enable
code based generation as used in the test.

Depends upon oxidecomputer/pki-playground#126
Copy link
Collaborator

@flihp flihp left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewjstone andrewjstone merged commit 7600756 into main Oct 9, 2025
7 checks passed
andrewjstone added a commit to oxidecomputer/sprockets that referenced this pull request Oct 9, 2025
This PR allows certs to be generated for multiple nodes so that we can
spin up arbitrary test clusters for trust quorum testing in Omicron.

A small change was made to the example `config.kdl` to better enable
code based generation as used in the test.

Depends upon oxidecomputer/pki-playground#126
andrewjstone added a commit to oxidecomputer/sprockets that referenced this pull request Oct 10, 2025
This PR allows certs to be generated for multiple nodes so that we can
spin up arbitrary test clusters for trust quorum testing in Omicron.

A small change was made to the example `config.kdl` to better enable
code based generation as used in the test.

Depends upon oxidecomputer/pki-playground#126
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.

library?

3 participants