Add SubjectPublicKeyInfo methods for cert::Cert

[lvkv]

Using types from rustls/pki-types#47, this PR adds the following methods for a cert::Cert's SubjectPublicKeyInfo:

• pub fn subject_public_key_info(&self) -> SubjectPublicKeyInfo
• pub(crate) fn subject_public_key_info_contents(&self) -> &[u8]

As cert::Cert's internal representation of a SubjectPublicKeyInfo (Cert::spki) lacks the ASN.1 SEQUENCE bytes to make it a proper RFC 5280 SubjectPublicKeyInfo, we need to sneakily add them back when someone calls subject_public_key_info(&self). I accomplished this with the addition of der::asn1_wrap which I mercilessly lifted from rustls::x509::asn1_wrap. Not completely mercilessly, though—I did make a few improvements to the signature, use of constants, and comments.

The motivation behind this is to make it possible to verify consistency for public and private keys (rustls/rustls#1918) —which can be accomplished by comparing SPKI values.

[lvkv]

Also, I refrained from pulling in the asn1_wrap/wrap_in_sequence tests from rustls::x509::asn1_wrap because:

1. The best home for this function seems yet to be determined
2. It would inflate the diff for what's currently a draft PR

They do pass, though 🙂 LMK if you'd like me to add them in.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.27%. Comparing base (bd9b7d6) to head (a1768ab).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
+ Coverage   97.21%   97.27%   +0.05%     
==========================================
  Files          19       19              
  Lines        4100     4190      +90     
==========================================
+ Hits         3986     4076      +90     
  Misses        114      114              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cpu]

Looking good 👍

Some of the CI failures are to be expected (e.g. because of the cargo patch). Some are true positives and I've tried to flag a few changes that should fix them.

[lvkv]

BTW—I ported the missing asn1_wrap tests over in ef13aaa just in case. As before, LMK if you guys think there's a better home for this (e.g. pki-types, some other shared DER repo, etc.)

[cpu]

Nice, almost there 🌠

[lvkv]

Side-note: is there a cargo run-whatever-github-actions-runs-but-locally-so-I-don't-waste-your-time-going-back-and-forth? I suppose I can always squint at ci.yml and pick out what I need to too

[lvkv]

Lemme also add a quick assertion for subject_public_key_info(&self) to get code coverage back up to 💯

[cpu]

@lvkv 😆 Unfortunately there isn't anything quite that nice, however you should be able to enable GitHub actions on your fork of the repo.

My workflow is to spin a branch off of the one I'm working on, push that to my fork without opening a PR, watch that branch's CI for failures (the gh command line tool is nice for this), and then iterate as needed. Once the temp branch is green I'll delete my original PR branch, checkout the tmp branch as the PR branch (a rename basically), and then force push the PR branch, now with some confidence CI will pass.

[cpu]

Thanks! Looks great :-)

[ctz]

This looks good to me.

Could we reorder / squash some of these commits? I think ideally we could have three commits without compromising on clarity:

• bump the pki-types version first, and drop the earlier hunks that add/remove the temporary patch. That commit should just be Cargo.toml and Cargo.lock.
• "Add der::asn1_wrap function lifted from rustls::x509" plus the associated tests
• "Add public and crate-private SPKI methods on cert::Cert" plus the associated tests

[djc]

I'd probably prefer to squash all of it? Doesn't make all that much sense to import asn1_wrap() without using it (technically running CI against that commit would fail due to warnings). The pki-types bump is also pointless without actually using the new type.

[lvkv]

@ctz, I've tidied up these commits—LMK if you'd like any other changes!

[ctz]

I'd probably prefer to squash all of it? Doesn't make all that much sense to import asn1_wrap() without using it (technically running CI against that commit would fail due to warnings).

Fair point, but I think the test should be "will this get in the way of git bisect in the future", and warnings won't do that.

I think trying to ensure that --deny warnings works at every commit is a step too far -- it either demands that entire features are in one commit, or that a given PR has a bunch of work to add and then remove allow(dead_code).

The pki-types bump is also pointless without actually using the new type.

I think I prefer this as a separate commit, to isolate the automated changes to Cargo.lock from commits that contain actual human work (in extremis, that means such commits can be just dropped and recreated rather than trying to solve conflicts in a machine-writable-only file).

[djc]

I'd probably prefer to squash all of it? Doesn't make all that much sense to import asn1_wrap() without using it (technically running CI against that commit would fail due to warnings).

Fair point, but I think the test should be "will this get in the way of git bisect in the future", and warnings won't do that.

I think trying to ensure that --deny warnings works at every commit is a step too far -- it either demands that entire features are in one commit, or that a given PR has a bunch of work to add and then remove allow(dead_code).

I think I disagree, for two reasons:

• Apart from just bisection (does it build and pass tests), I think it is also helpful for human reviewers to see the rationale for new code in the same commit, which can be not at all obvious (the blame case). If I bisect a regression and land on a commit that adds a bunch of unused code, that doesn't help me all that much.
• For the specific case of adding new API/code, splitting commits doesn't have the advantage it has in many other cases of making code easier to review. Changing code, in particular, is easier to review when split into chunks that handle one logical/semantic change per commit -- but reviewing new code is O(new lines of code) no matter what.

The pki-types bump is also pointless without actually using the new type.

I think I prefer this as a separate commit, to isolate the automated changes to Cargo.lock from commits that contain actual human work (in extremis, that means such commits can be just dropped and recreated rather than trying to solve conflicts in a machine-writable-only file).

So one solution for this is that we have a Cargo.lock bump in an initial commit and then bump the requirement in Cargo.toml in the commit that introduces code depending on new API.

(cc @cpu to review this discussion when you have time.)

[cpu]

If I bisect a regression and land on a commit that adds a bunch of unused code, that doesn't help me all that much.

I think if someone were to break up commits this way it would be important for the "why is this being added" context to be present in the commit message.

For the specific case of adding new API/code, splitting commits doesn't have the advantage it has in many other cases of making code easier to review. Changing code, in particular, is easier to review when split into chunks that handle one logical/semantic change per commit -- but reviewing new code is O(new lines of code) no matter what.

I find this argument more compelling: I do appreciate refactoring/changes to existing code being done in small increments as opposed to a big all-in-one rewrite, but what you're saying about the advantage of that strategy being reduced for new additions resonates with me.

So perhaps we should consider a policy that differs in its commit granularity recommendation based on whether it's largely new code or largely changes to existing code?

So one solution for this is that we have a Cargo.lock bump in an initial commit and then bump the requirement in Cargo.toml in the commit that introduces code depending on new API.

That sounds like a good compromise for this situation.

If there's consensus on the above we should try to capture some recommendations into the CONTRIBUTING.md in Rustls. I can take that task if folks are in agreement.