Simplify NameIterator

[djc]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 98.73950% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.42%. Comparing base (c32bfb9) to head (8ec1f76).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/subject_name/mod.rs	98.71%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
+ Coverage   97.30%   97.42%   +0.11%     
==========================================
  Files          20       20              
  Lines        4343     4345       +2     
==========================================
+ Hits         4226     4233       +7     
+ Misses        117      112       -5     

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

[cpu]

This makes sense to me as a way to tidy the existing logic +1.

The broader purpose/pre-existing logic still feels off to me in a way that's tricky to articulate. Here's my analysis. WDYT?

By my read, the bool on the NameIterator (and its Option<..> predecessor) only influences whether the iterator yields GeneralName::DirectoryName after yielding each of the SANs. Previously it didn't seem to consider whether the option-wrapped untrusted::Input was empty or not, just whether it was assigned Some(_), so a bool (or a more descriptive two-value enum) seems like a sensible replacement.

The GeneralName::DirectoryName variant is a content-less variant that seems to serve no deeper purpose than to make sure we fall in to a specific rejection match arm from check_presented_id_conforms_to_constraints() when we have the cert's subject GeneralName::DirectoryName from the NameIter in hand while also looking at a GeneralName::DirectoryName base general name from a name constraint extension.

A couple questions that come to mind:

• Does this work the right way if there's an empty cert subject? It seems like both the prev & current code set the NameIterator's directory_name control to true for the name constraint evaluation in check_name_constraints() without deeper analysis of the path node's subject. Is that the right thing to do?
• Could we remove this logic from NameIterator entirely, and somehow arrange for an extra call to check_presented_id_conforms_to_constraints() with a hardcoded GeneralName::DirectoryName for name be made after processing the SANs from NameIterator? Is that more brittle in a way that matters?

I don't know that we need to block this small improvement on the broader topic but it feels like there's something left to pull at here. Name constraints are super gross so it's also very possible I'm overlooking some fundamental issue that makes the current approach superior/necessary.

[djc]

Fair questions.

• Could we remove this logic from NameIterator entirely, and somehow arrange for an extra call to check_presented_id_conforms_to_constraints() with a hardcoded GeneralName::DirectoryName for name be made after processing the SANs from NameIterator? Is that more brittle in a way that matters?

Implemented this, which seems like a nice simplification.

[cpu]

LGTM, thanks!

[cpu]

Does this work the right way if there's no cert subject?

@ctz Do you have any memories on this subject from the name constraint work that happened around the time of the fork? No worries if not, just curious.

[ctz]

Does this work the right way if there's no cert subject?

@ctz Do you have any memories on this subject from the name constraint work that happened around the time of the fork? No worries if not, just curious.

I don't. But what do you mean by "no cert subject"? I don't think it can be entirely absent, but it could be just empty?

[cpu]

but it could be just empty?

That's what I meant. Sorry, agreed that "no cert subject" is ambiguous.