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

validation: add Rust-side trust store APIs #9744

Merged
merged 2 commits into from Oct 22, 2023

Conversation

woodruffw
Copy link
Contributor

Breakout from #8873.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Contributor Author

Documenting here since the context is probably lost elsewhere: the value of a separate Store type here is twofold:

  • Misuse resistance: a newtype here means that users can't accidentally confuse the intermediate and store chain sets
  • Optimizability: the current implementation does a linear search through the trusted set, but having a newtype gives us flexibility needed to optimize lookups in the future (e.g. keying off of subjects)

#[macro_export]
macro_rules! cert {
($pem:literal) => {{
let parsed = Box::leak(Box::new(pem::parse($pem).unwrap()));
Copy link
Member

Choose a reason for hiding this comment

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

I have questions. Namely: Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this was a convenience macro while I was writing path validation tests. It's pretty ugly though and there's no repetition here, so I can just drop it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind a convenience macro, but there's no need to be leaking things :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if I can work around it, but I think the leak is necessary to get the lifetimes to work -- either a macro or a full function here will have local ownership of the Pem that's being borrowed from, so there's no way to get a lifetime shorter than 'static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've rewritten this as a fixture that returns a pem::Pem and a cert(...) helper -- that avoids the need to a leak, at the cost of a more explicit lifetime relationship between the two + binding for the pem::Pem at each callsite 🙂

Signed-off-by: William Woodruff <william@trailofbits.com>
@alex alex merged commit a04865b into pyca:main Oct 22, 2023
57 checks passed
@woodruffw woodruffw deleted the tob-trust-store branch October 22, 2023 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants