Remove URN support #1930

rousskov · 2024-10-31T20:52:56Z

Squid URN resolution code has been neglected for a very long time and
caused multiple security vulnerabilities. Its existence complicates
planned StoreClient API refactoring. URN resolution feature is rarely
used (if at all). If Squid handling of unknown (to Squid) URI schemes is
enhanced, a similar feature can be implemented externally, using
url_rewrite_program helpers or adaptation services.

Squid now treats URN as any unknown (to Squid) URI scheme, typically
responding with an HTTP 400 (Bad Request) ERR_INVALID_URL.

Prior to these changes, Squid did not forward HTTP requests with URN
scheme targets to cache_peers either. Instead, Squid rewrote request
targets into URLs with an HTTP scheme (and a /uri-res/N2L? path
prefix; see Historic THTTP RFC 2169) and forwarded those adapted
requests. That hard-coded adaptation and forwarding are now gone.

kinkie

YES!

rousskov

This minimal PR implements URN support removal as I requested earlier. It does not add support for forwarding requests with URN scheme (and other unsupported schemes) to cache_peers. I am not sure there is enough need for such forwarding, but that should be settled separately if somebody actually decides to work on adding that new feature.

rousskov · 2024-10-31T21:01:12Z

errors/template.am

@@ -48,6 +48,5 @@ ERROR_TEMPLATES = \
 	templates/ERR_TOO_BIG \
 	templates/ERR_UNSUP_HTTPVERSION \
 	templates/ERR_UNSUP_REQ \
-	templates/ERR_URN_RESOLVE \


This PR does not update PO/POT files based on an earlier recommendation. If those files should be updated to reflect ERR_URN_RESOLVE removal, please let me know, and we will update them (you can even preview most of those changes in earlier branch commit c00c79a that was later reverted).

These templates are used to generate the "langpack" releases which get installed for use by much older versions of Squid. The template file needs to be retained until no supported version of Squid tries to load it on startup.

This is also why the ERR_ESI remains.

The template file needs to be retained

Restored at 0f42fbd.

yadij

Some major flaws to get you started.

yadij · 2024-11-02T02:55:45Z

doc/release-notes/release-7.sgml.in

+<p>If necessary, a similar feature can be implemented externally, using
+url_rewrite_program helpers or adaptation services.


That would be a bug. You have removed the ability for Squid to correctly:
a) parse any request-target with "urn:" scheme, and
b) send a valid URN anywhere (including helpers, ICAP, eCAP, and even cache.log - receive unknown:// at most)

A squid lacking "foo:" support should reject all "foo:" URLs on initial parse/validate. Failure to do that re-opens one of those security vulnerabilities I closed off by fixing the URN NID validation.

You have removed the ability for Squid to ... send a valid URN anywhere (including helpers, ICAP, eCAP...)

Agreed. I have adjusted PR description and release notes (commit 14914f4) to precondition external support on enhancing Squid to handle unknown (to Squid) URI schemes (which should not be limited to URN scheme, of course). Fortunately, we do not need to debate the details of that hypothetical enhancement -- folks implementing it should initiate that debate outside this PR.

yadij · 2024-11-02T03:00:52Z

errors/template.am

@@ -48,6 +48,5 @@ ERROR_TEMPLATES = \
 	templates/ERR_TOO_BIG \
 	templates/ERR_UNSUP_HTTPVERSION \
 	templates/ERR_UNSUP_REQ \
-	templates/ERR_URN_RESOLVE \


These templates are used to generate the "langpack" releases which get installed for use by much older versions of Squid. The template file needs to be retained until no supported version of Squid tries to load it on startup.

This is also why the ERR_ESI remains.

yadij · 2024-11-02T03:18:12Z

doc/release-notes/release-7.sgml.in

+<p>Squid URN resolution code has been neglected for a very long time and caused
+multiple security vulnerabilities. This feature was rarely used (if at all).


Come on be honest. All attempts to update the code were vetoed by you. Otherwise this code would very much have been updated by at least four authors in the past 10 years.

Come on be honest. All attempts to update the code were vetoed by you. Otherwise this code would very much have been updated by at least four authors in the past 10 years.

I hope that any vetoes were correct, but this discussion and implications of dishonesty feel out of scope: PR text describes code state. It does not speculate about the reasons that led to that code state.

yadij · 2024-11-02T03:20:22Z

src/adaptation/ecap/Host.cc

@@ -56,7 +56,6 @@ Adaptation::Ecap::Host::Host()
    libecap::protocolHttps.assignHostId(AnyP::PROTO_HTTPS);
    libecap::protocolFtp.assignHostId(AnyP::PROTO_FTP);
    libecap::protocolWais.assignHostId(AnyP::PROTO_WAIS);
-    libecap::protocolUrn.assignHostId(AnyP::PROTO_URN);


Thus rendering eCAP unable to meet the release notes claimed capability of performing Trivial-HTTP Resolver gateway.
("AnyP::PROTO_UNKNOWN" are passed as static string "unknown", not as the received scheme image)

Host application IDs being configured by this code are an optimization that speeds up string comparison for common cases. eCAP code should function correctly without that optimization. If it does not, it is an out-of-scope bug (in eCAP adapter or host application code).

yadij · 2024-11-02T03:30:25Z

src/anyp/Uri.cc

-            absolute_.append(":", 1);
+        absolute_.append("//", 2);
+        const bool allowUserInfo = getScheme() == AnyP::PROTO_FTP ||
+                                   getScheme() == AnyP::PROTO_UNKNOWN;


This needs to exclude URI with image() of "urn:" (which is now part of AnyP::PROTO_UNKNOWN) or we open a security vulnerability for sensitive data exfiltration.

Also, these changes are what render ICAP and helpers unable to meet the release notes claimed capability of performing Trivial-HTTP Resolver gateway.

This needs to exclude URI with image() of "urn:" (which is now part of AnyP::PROTO_UNKNOWN) or we open a security vulnerability for sensitive data exfiltration.

PR code treats all unknown (to Squid) URI schemes the same. This code had received unknown non-URN schemes prior to PR changes. Thus, the "we open a vulnerability" assertion is false: Either that vulnerability existed before these changes, or these changes do not open it.

Also, these changes are what render ICAP and helpers unable to meet the release notes claimed capability of performing Trivial-HTTP Resolver gateway.

That problem was flagged and addressed in another change request. If necessary, let's continue this part of the discussion there.

Also described how URNs are treated now.

rousskov

I believe all change requests have been addressed. Please re-review.

rousskov · 2024-11-26T14:19:59Z

doc/release-notes/release-7.sgml.in

+<p>Squid URN resolution code has been neglected for a very long time and caused
+multiple security vulnerabilities. This feature was rarely used (if at all).


Come on be honest. All attempts to update the code were vetoed by you. Otherwise this code would very much have been updated by at least four authors in the past 10 years.

I hope that any vetoes were correct, but this discussion and implications of dishonesty feel out of scope: PR text describes code state. It does not speculate about the reasons that led to that code state.

rousskov · 2024-11-26T14:24:17Z

doc/release-notes/release-7.sgml.in

+<p>If necessary, a similar feature can be implemented externally, using
+url_rewrite_program helpers or adaptation services.


You have removed the ability for Squid to ... send a valid URN anywhere (including helpers, ICAP, eCAP...)

Agreed. I have adjusted PR description and release notes (commit 14914f4) to precondition external support on enhancing Squid to handle unknown (to Squid) URI schemes (which should not be limited to URN scheme, of course). Fortunately, we do not need to debate the details of that hypothetical enhancement -- folks implementing it should initiate that debate outside this PR.

rousskov · 2024-11-26T14:28:05Z

src/adaptation/ecap/Host.cc

@@ -56,7 +56,6 @@ Adaptation::Ecap::Host::Host()
    libecap::protocolHttps.assignHostId(AnyP::PROTO_HTTPS);
    libecap::protocolFtp.assignHostId(AnyP::PROTO_FTP);
    libecap::protocolWais.assignHostId(AnyP::PROTO_WAIS);
-    libecap::protocolUrn.assignHostId(AnyP::PROTO_URN);


Host application IDs being configured by this code are an optimization that speeds up string comparison for common cases. eCAP code should function correctly without that optimization. If it does not, it is an out-of-scope bug (in eCAP adapter or host application code).

rousskov · 2024-11-26T14:34:53Z

src/anyp/Uri.cc

-            absolute_.append(":", 1);
+        absolute_.append("//", 2);
+        const bool allowUserInfo = getScheme() == AnyP::PROTO_FTP ||
+                                   getScheme() == AnyP::PROTO_UNKNOWN;


This needs to exclude URI with image() of "urn:" (which is now part of AnyP::PROTO_UNKNOWN) or we open a security vulnerability for sensitive data exfiltration.

PR code treats all unknown (to Squid) URI schemes the same. This code had received unknown non-URN schemes prior to PR changes. Thus, the "we open a vulnerability" assertion is false: Either that vulnerability existed before these changes, or these changes do not open it.

Also, these changes are what render ICAP and helpers unable to meet the release notes claimed capability of performing Trivial-HTTP Resolver gateway.

That problem was flagged and addressed in another change request. If necessary, let's continue this part of the discussion there.

eduard-bagdasaryan and others added 10 commits October 25, 2024 13:50

Remove URN support

79ce64a

Autoformatted

c8f037d

Polished the modified code and removed URN doc

7837f2b

Removed URN from error translations

c00c79a

Revert "Removed URN from error translations"

b069854

Autoformatted

504a826

Cleanup

b6e5413

Updated realease notes

dc94f66

fixup: Polished example wording

98c7e13

fixup: Found one more URN entry

fbe4fde

kinkie previously approved these changes Oct 31, 2024

View reviewed changes

rousskov commented Oct 31, 2024

View reviewed changes

rousskov added the S-waiting-for-more-reviewers needs a reviewer and/or a second opinion label Oct 31, 2024

yadij requested changes Nov 2, 2024

View reviewed changes

kinkie added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-more-reviewers needs a reviewer and/or a second opinion labels Nov 2, 2024

Restored ERR_URN_RESOLVE per a PR request

0f42fbd

eduard-bagdasaryan dismissed kinkie’s stale review via 0f42fbd November 15, 2024 11:42

Docs: External implementation requires Squid enhancements

14914f4

Also described how URNs are treated now.

rousskov commented Nov 26, 2024

View reviewed changes

rousskov requested a review from yadij November 26, 2024 14:39

rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove URN support #1930

Remove URN support #1930

rousskov commented Oct 31, 2024 •

edited

Loading

kinkie left a comment

rousskov left a comment

rousskov Oct 31, 2024

yadij Nov 2, 2024

eduard-bagdasaryan Nov 15, 2024

yadij left a comment

yadij Nov 2, 2024

rousskov Nov 26, 2024

yadij Nov 2, 2024

yadij Nov 2, 2024

rousskov Nov 26, 2024

yadij Nov 2, 2024

rousskov Nov 26, 2024

yadij Nov 2, 2024

yadij Nov 2, 2024

rousskov Nov 26, 2024 •

edited

Loading

rousskov left a comment

rousskov Nov 26, 2024

rousskov Nov 26, 2024

rousskov Nov 26, 2024

rousskov Nov 26, 2024 •

edited

Loading

		<p>If necessary, a similar feature can be implemented externally, using
		url_rewrite_program helpers or adaptation services.

		<p>Squid URN resolution code has been neglected for a very long time and caused
		multiple security vulnerabilities. This feature was rarely used (if at all).

Remove URN support #1930

Are you sure you want to change the base?

Remove URN support #1930

Conversation

rousskov commented Oct 31, 2024 • edited Loading

kinkie left a comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yadij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

rousskov commented Oct 31, 2024 •

edited

Loading

rousskov Nov 26, 2024 •

edited

Loading

rousskov Nov 26, 2024 •

edited

Loading