Bug 5363: Handle IP-based X.509 SANs better

[walkert]

Most X.509 Subject Alternate Name extensions encountered by Squid are
based on DNS domain names. However, real-world servers (including
publicly available servers that use vanity IP addresses) also use
IP-based SANs. Squid mishandled IP-based SANs in several ways:

• When generating certificates for servers targeted by their IP
  addresses, addAltNameWithSubjectCn() used that target IP as a
  DNS-based SAN, resulting in a frankenstein DNS:[ip] SAN value that
  clients ignored when validating a Squid-generated certificate.

• When validating a received certificate, Squid was ignoring IP-based
  SANs. When Subject CN did not match the requested IP target, Squid
  only looked at DNS-based SANs, incorrectly failing validation.

• When checking certificate-related ACLs like ssl::server_name,
  matchX509CommonNames() ignored IP-based SANs, not matching
  certificates containing ACL-listed IP addresses.

Squid now recognizes and generates IP-based SANs.

Squid now attempts to match IP-based SANs with ACL-listed IP addresses,
but the success of that attempt depends on whether ACL IP parameters are
formatted the same way inet_ntop(3) formats those IP addresses: Matching
is still done using c-string/domain-based ::matchDomainName() (for
ssl::server_name) and string-based regexes (for ssl::server_name_regex).
Similar problems affect dstdomain and dstdomain_regex ACLs. A dedicated
fix is needed to stop treating IPs as domain names in those contexts.

This change introduces partial support for preserving IP-vs-domain
distinction in parsed/internal Squid state rather than converting both
to a string and then assuming that string is a DNS domain name.

[squid-prbot]

Can one of the admins verify this patch?

[rousskov]

@walkert, thank you for sharing these changes and detailing the logic behind them! I have not reviewed PR code changes yet, but I do have one higher-level question: In your opinion, does this PR address (or at least relates to) Bug 5363? If this PR addresses the same problem, then which solution is better, the patch posted for that bug report or this PR?

[walkert]

Thanks @rousskov. So, as far as I can see, the first commit in this PR fixes the bug you're referring to. That bug however does not resolve the second problem we saw which is fixed in the most-recent commit. In terms of which solution is better I would say it may depend on your appetite for changing existing code. My commit modifies the behaviour of the existing addAltNameWithSubjectCn to deal appropriately with IPs and DNS names. The patch from that bug introduces a new function addAltNameWithIpCn. Further, my first commit (which fixes the bug) introduces a helper function which is relied upon by the most recent commit. Looking again at the patch, the new addAltNameWithIpCn contains a fair amount of duplicative code from addAltNameWithSubjectCn and I'm not sure whether that's really necessary. Needless to say I'm open to whatever your preference is.

[rousskov]

@walkert, thank you for that analysis. We will proceed with your PR as the primary solution, at least for now. I hope to be able to review it, at least superficially next week.

[walkert]

Thanks @rousskov. I just noticed a failure due to my forgetting to declare one of the functions as static. Just fixed and pushed that change (then force-pushed a rebase of origin/master).

[rousskov]

(then force-pushed a rebase of origin/master).

FYI: There is no need to rebase (and force push) unless there are merge conflicts or other rare complications. Anubis, the merge bot used by Squid repository, will squash-merge all the changes into master (using PR title/description as the commit message) at the end of the merge process.

[rousskov]

I have left a few high-level change requests. Once those are addressed, I can help polish this code to meet various Squid code style guidelines.

[walkert]

Required to allow gadgets.cc to refer to ip/Address.h.

[rousskov]

You have made significant progress, thank you! I left a few change requests for the next iteration. I continue to ignore most of the secondary issues that we will handle later.

[rousskov]

Give me a couple of days to see if I can solve XXX in Ssl::OneNameMatcher::matchIp()

Done. This PR is now free of known in-scope XXXs and TODOs.

I also updated PR title/description, primarily to better reflect the (longer) list of fixed bugs and to disclose unresolved (out of scope) issues. @walkert, when you get a chance, please review the current PR title/description and let me know if I missed or misrepresented anything important.

My concerns were addressed.

[walkert]

Thanks again for all your work @rousskov. I'm not sure this can even be considered my PR anymore! Regardless, I've reviewed your changes to the title and description and it all looks good to me.

[rousskov]

I think this PR is ready for review by others (CC: @yadij, @kinkie). The original triage, initial fix implementation, and testing belong to @walkert. All bugs are mine. I left a few comments to assist with reviews. If you want any of this PR changes factored out into prerequisite PRs (without increasing their breadth/scope), please let me know, and I will help with that.

Once the dust settles, we can merge from master to fix staged CI tests and rename a few functions as detailed in one of my comments. These pending changes should not affect any reviews.

If there are no objections, I plan to clear this PR for merging in ten calendar days.

[rousskov]

This class is at the core of this PR changes. In official code, a single address that may be either an IP address or a DNS domain name is either isolated into two or more co-existing objects (e.g., host information inside AnyP::Uri) or morphed into one opaque SBuf object (e.g., server in ssl_verify_cb()). Neither approach works well, for obvious reasons. This PR uses the new Host class to accurately and as-safely-as-possible represent this naming duality in several X.509 SAN-related contexts. Eventually, more code should be converted to use this class. I have left a few related TODOs.

The proposed "host" name works well enough IMO, but I would not be surprised if there are better alternatives our there.

[walkert]

Hi @rousskov - do you know if there's anything else that needs to be done for the review to continue? Thanks.

[rousskov]

@walkert, unfortunately, the answer to your question is "no": I do not know how to convince @yadij to review PRs awaiting his review. IMO, this persistent problem is one of the existential threats faced by the Squid Project, but all my attempts to find a solution have failed. I can only hope that we will resume making progress with this PR soon.

[walkert]

@rousskov understood. Thanks for taking the time to respond. I'll keep watching and hoping.

[walkert]

Hi @rousskov, is there scope to hand this off to another reviewer if @yadij is unavailable? Thanks.

[rousskov]

@walkert, there is no formal procedure to overwrite, move, or clear a "this PR needs to be reviewed by me" flag set by a core developer. Project core developers hold a lot of power, and multiple attempts to establish timeouts (or otherwise restructure the governing rules) have failed so far. FWIW, I plan to bring this PR up again during the next meeting which should happen on October 22, 2024.

[walkert]

@rousskov understood. Thanks for the info. 🙏

[rousskov]

For now, this alias allows us to document Dns::DomainName properties/invariants without changing a lot of code to cope with a new dedicated "real" type. It might stay that way, but I would not be surprised if we will eventually convert this into a class to reduce chances of wrong information (e.g., an IP address or even address:port authority) sneaking into this object and screwing up ACL checks and such.

Technically, this type alias declaration is not a true C++ forward declaration. I think we should make an exception for this forward.h entry, at least for now, because there is no better place to put it. However, if others insist, we can add something like src/dns/elements.h and put it there.

[rousskov]

This C-style function was one of the primary sources of problems fixed in this PR. As you can see, it was using essentially two void* pointers (ASN1_STRING can be nearly anything) and expected the passed callback (i.e. check_func) to figure out what X.509 name it is being called with. The replacement GeneralNameMatcher API (declared below) provides strong visual clues for developers backed by compile-time type checks to make sure we are not trying to compare a domain name with an IP address or simply forget to handle an IP address case.

[rousskov]

Alex: If there are no objections, I plan to clear this PR for merging in ten calendar days.

Status update: Amos has self-requested a review, so the ball is on his side, and the timeout I penciled in above is now irrelevant.

[walkert]

Hi @yadij. I just wanted to check-in and see if there's anything I can do to help with this review? Thanks.

[rousskov]

I accidentally noticed that Amos has removed his request for review and cleared this PR for merging. I now finished last in-scope TODO, merged from master to get the current set of CI tests (that have changed quite a bit while this PR was waiting), and approved this PR so that it can be auto-merged by Anubis (if those tests succeed).

@walkert, please ping me if you notice any failed tests.

[walkert]

Thanks again for all your work @rousskov. I'll keep my eyes peeled.

[rousskov]

@walkert, you may be interested in #1967 that attempts to fix a bug I introduced in this PR. If you can test that fix, please do.