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

Add certificate field to TrustAnchor struct behind a feature flag? #15

Closed
mspiegel opened this issue Nov 28, 2023 · 16 comments
Closed

Add certificate field to TrustAnchor struct behind a feature flag? #15

mspiegel opened this issue Nov 28, 2023 · 16 comments

Comments

@mspiegel
Copy link

The TrustAnchor struct stores the elements of a trust anchor needed for verifying certificates. I have a use case where I need the entire certificate of each trust anchor. I recognize the reasons against adding a new field to the TrustAnchor struct: (1) requires a semver update and (2) increases the size of the webpki-roots library with data that the common use case does not use.

Would you consider the addition of a field certificate of type CertificateDer to the struct TrustAnchor that was behind a feature flag? I believe that would address the concerns above. The tradeoff is the complexity of the feature flag. I would volunteer to submit the PR. I didn't want to write the code if there's no interest. Thanks.

@djc
Copy link
Member

djc commented Nov 28, 2023

Why do you need the entire certificate? Why couldn't that work while maintaining your own type that contains both the TrustAnchor and the CertificateDer, or some kind of mapping from the TrustAnchor to the CertificateDer?

@mspiegel
Copy link
Author

mspiegel commented Nov 28, 2023

With regards to why we need the entire certificate, the short answer is FIPS compliance. With regards to maintaining our own mapping, I apologize I didn't explain the second step of my proposal. I would also submit a PR to the 'webpki-roots' crate that populated this proposed new field behind a feature flag of the 'webpki-roots' crate. The PR to 'webpki-roots' would update /tests/codegen.rs with the changes guarded by the feature flag.

@briansmith
Copy link

With regards to why we need the entire certificate, the short answer is FIPS compliance.

Could you please be more precise and go into more detail about this? Many of are actively working on FIPS compliance and also NIST SP800-52 compliance and it would be good to find a solution that isn't so expensive as what you are proposing.

@mspiegel
Copy link
Author

I will do my best to provide more detail about FIPS compliance with the caveat that I am not an expert on FIPS compliance. I believe that an implementation is certified as FIPS compliance. I believe the root certificates are out of scope with regards to FIPS compliance. Maybe that last statement is incorrect? My intended use case is to use a different TLS library and use the 'webpki-roots' crate as the source of truth for the root certificates.

I'd like to understand more, what part of my proposal is expensive? I believe the patch to 'pki-types' would be limited to the TrustAnchor struct and the patch to 'webpki-roots' would modify /tests/codegen.rs to populate the new field. These changes would be behind a feature flag is that disabled by default. To rephrase my request: are you amenable to downstream consumers of the webpki-root crate as a source of certificates?

@djc
Copy link
Member

djc commented Nov 28, 2023

If you're looking to use a different TLS library then I think don't think that's a compelling use case for changing pki-types (to make the TrustAnchor type contain a bunch of extra data that webpki doesn't need). You can quite easily fork webpki-roots to generate whatever you need based on the CCADB.

@briansmith
Copy link

When a field is added using #[cfg(feature = "...")] and that feature flag is enabled by any crate, then that field will exist for all uses of the structure, even within crates that don't enable the feature. Any crate that constructs or (completely) pattern matches a TrustAnchor within the entire compilation graph of crates would then need to enable the feature flag, or its compilation will break. Thus many projects (including probably Rustls and others) would need to support this feature for it to realistically have a chance of working. But, this is really against the intent of how feature flags are supposed to work, in terms of not breaking things that don't use the feature flag, even within a given compilation.

Basically, it creates a lot of problems to have a (pub) field that is added/removed based on conditional compilation.

@mspiegel
Copy link
Author

I understand your concerns. With regards to the feedback, "You can quite easily fork webpki-roots to generate whatever you need based on the CCADB." The downside of this approach is that when '/tests/codegen.rs' changes over time, those improvements need to be recognized and transferred to the fork. We know this is a challenge with forking open-source software.

A different proposal is to create a new crate webpki-roots-raw that instead of pub const TLS_SERVER_ROOTS would have a pub const that has an encoding of all the root certificates. And modify the crate webpki-roots to have a dev-dependency on the new create webpki-roots-raw. The existing code generation would be split across the webpki-roots-raw and webpki-roots crates. The interface of the webpki-roots would be unchanged. This would avoid any changes to type signatures. I don't mind doing the work and submitting the pull request.

@djc
Copy link
Member

djc commented Nov 28, 2023

I'd propose instead that we change the webpki-roots repo to be a workspace that includes a crate that pulls the CCADB stuff and exposes some kind of API on that. Then the tests/codegen.rs script will have a path dependency on that crate, and we can get it published for other users so you can generate your own webpki-roots-like crate.

@cpu
Copy link
Member

cpu commented Nov 28, 2023

I think @djc's proposal would be an OK direction. Making a separate webpki-roots-raw crate is less appealing to me for two reasons:

  1. It would be more additional overhead - we would need to publish that crate for each root update in addition to the webpki-roots crate.
  2. (Bikeshedding) the name would be confusing since webpki won't consume that crate.

Making a separate crate for the codegen is also additional maintainer overhead to cater to a usecase that we don't have in the Rustls ecosystem, but it at least wouldn't require as much ongoing maintenance (changes to that code are infrequent compared to root data updates).

@djc
Copy link
Member

djc commented Nov 28, 2023

The downside of this approach is that when '/tests/codegen.rs' changes over time, those improvements need to be recognized and transferred to the fork. We know this is a challenge with forking open-source software.

Yes, getting open source maintainers (often, as in my case, volunteers) to do your maintenance work is helpful to organizations, including VC-funded startups such as your employer's. I'm not sure it's fair to describe this as the forking itself being a problem.

@mspiegel
Copy link
Author

This discussion has prompted another possible design. How about a proposed change only to the webpki-roots crate that would add a feature flag. The feature flag determines whether a new global const exists in the webpki-roots crate that stores the root certificates? I believe that doesn’t break semver compatibility.

@djc
Copy link
Member

djc commented Nov 28, 2023

I don't think you'll convince us that this stuff should appear in the public webpki-roots API.

@mspiegel
Copy link
Author

I'd propose instead that we change the webpki-roots repo to be a workspace that includes a crate that pulls the CCADB stuff and exposes some kind of API on that. Then the tests/codegen.rs script will have a path dependency on that crate, and we can get it published for other users so you can generate your own webpki-roots-like crate.

I like this proposal. Any objections if I make a pull request to webpki-roots with this change?

@djc
Copy link
Member

djc commented Nov 28, 2023

Please closely read the rustls CONTRIBUTING.md before you submit your PR.

@mspiegel
Copy link
Author

I'll close this issue and submit a PR to the webpki-roots repo.

mspiegel added a commit to mspiegel/webpki-roots that referenced this issue Nov 29, 2023
Change the webpki-roots repo to be a workspace that includes a crate
that pulls the CCADB stuff and exposes an API.
References rustls/pki-types#15
@cpu
Copy link
Member

cpu commented Dec 6, 2023

Resolved w/ rustls/webpki-roots#56

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

No branches or pull requests

4 participants