Remove URN support #1930
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -329,13 +329,4 @@ TODO: get RFCs linked from ietf | |
| we have made almost all of the cachemgr information available | ||
| via SNMP. | ||
|
|
||
| */ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ The Squid-@SQUID_RELEASE@ change history can be <url url="https://github.com/squ | |
| <item>Removed purge tool | ||
| <item>Remove deprecated languages | ||
| <item>Remove Ident protocol support | ||
| <item>Remove URN protocol support | ||
| </itemize> | ||
|
|
||
| <p>Most user-facing changes are reflected in squid.conf (see further below). | ||
|
|
@@ -123,6 +124,14 @@ in the position of what used to be a %ui record field. | |
| <p>If necessary, an external ACL helper can be written to perform Ident transactions | ||
| and deliver the user identity to Squid through the **user=** annotation. | ||
|
|
||
| <sect1>Removed URN protocol support | ||
|
|
||
| <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). | ||
|
|
||
| <p>If necessary, a similar feature can be implemented externally, using | ||
| url_rewrite_program helpers or adaptation services. | ||
|
There was a problem hiding this comment. That would be a bug. You have removed the ability for Squid to correctly: 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. There was a problem hiding this comment.
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. |
||
|
|
||
| <sect>Changes to squid.conf since Squid-@SQUID_RELEASE_OLD@ | ||
| <p> | ||
| This section gives an account of those changes in three categories: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,5 @@ ERROR_TEMPLATES = \ | |
| templates/ERR_TOO_BIG \ | ||
| templates/ERR_UNSUP_HTTPVERSION \ | ||
| templates/ERR_UNSUP_REQ \ | ||
|
There was a problem hiding this comment. 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). There was a problem hiding this comment. 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. There was a problem hiding this comment.
Restored at 0f42fbd. |
||
| templates/ERR_WRITE_ERROR \ | ||
| templates/ERR_ZERO_SIZE_OBJECT | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,6 @@ typedef enum { | |
| #if USE_HTCP | ||
| PROTO_HTCP, | ||
| #endif | ||
| PROTO_WHOIS, | ||
| PROTO_ICY, | ||
| PROTO_TLS, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -325,11 +325,6 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) | |
| if (scheme == AnyP::PROTO_NONE) | ||
| return false; // invalid scheme | ||
|
|
||
| // URLs then have "//" | ||
| static const SBuf doubleSlash("//"); | ||
| if (!tok.skip(doubleSlash)) | ||
|
|
@@ -531,48 +526,6 @@ AnyP::Uri::parse(const HttpRequestMethod& method, const SBuf &rawUrl) | |
| } | ||
| } | ||
|
|
||
| /// Extracts and returns a (suspected but only partially validated) uri-host | ||
| /// IPv6address, IPv4address, or reg-name component. This function uses (and | ||
| /// quotes) RFC 3986, Section 3.2.2 syntax rules. | ||
|
|
@@ -695,23 +648,18 @@ AnyP::Uri::absolute() const | |
|
|
||
| absolute_.append(getScheme().image()); | ||
| absolute_.append(":",1); | ||
| absolute_.append("//", 2); | ||
| const bool allowUserInfo = getScheme() == AnyP::PROTO_FTP || | ||
| getScheme() == AnyP::PROTO_UNKNOWN; | ||
|
There was a problem hiding this comment. This needs to exclude URI with image() of "urn:" (which is now part of There was a problem hiding this comment. Also, these changes are what render ICAP and helpers unable to meet the release notes claimed capability of performing Trivial-HTTP Resolver gateway. There was a problem hiding this comment.
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.
That problem was flagged and addressed in another change request. If necessary, let's continue this part of the discussion there. |
||
|
|
||
| if (allowUserInfo && !userInfo().isEmpty()) { | ||
| static const CharacterSet uiChars = CharacterSet(UserInfoChars()) | ||
| .remove('%') | ||
| .rename("userinfo-reserved"); | ||
| absolute_.append(Encode(userInfo(), uiChars)); | ||
| absolute_.append("@", 1); | ||
| } | ||
| absolute_.append(authority()); | ||
| absolute_.append(path()); // TODO: Encode each URI subcomponent in path_ as needed. | ||
| } | ||
|
|
||
|
|
@@ -723,15 +671,15 @@ AnyP::Uri::absolute() const | |
| * and never copy the query-string part in the first place | ||
| */ | ||
| char * | ||
| urlCanonicalCleanWithoutRequest(const SBuf &url, const HttpRequestMethod &method) | ||
| { | ||
| LOCAL_ARRAY(char, buf, MAX_URL); | ||
|
|
||
| snprintf(buf, sizeof(buf), SQUIDSBUFPH, SQUIDSBUFPRINT(url)); | ||
| buf[sizeof(buf)-1] = '\0'; | ||
|
|
||
| // CONNECT method and non-stripped URIs can go straight out | ||
| if (Config.onoff.strip_query_terms && method != Http::METHOD_CONNECT) { | ||
| // strip anything AFTER a question-mark | ||
| // leaving the '?' in place | ||
| if (auto t = strchr(buf, '?')) { | ||
|
|
@@ -814,10 +762,6 @@ urlIsRelative(const char *url) | |
| void | ||
| AnyP::Uri::addRelativePath(const char *relUrl) | ||
| { | ||
| // TODO: Handle . and .. segment normalization | ||
|
|
||
| const auto lastSlashPos = path_.rfind('/'); | ||
|
|
@@ -962,7 +906,6 @@ urlCheckRequest(const HttpRequest * r) | |
| /* does method match the protocol? */ | ||
| switch (r->url.getScheme()) { | ||
|
|
||
| case AnyP::PROTO_HTTP: | ||
| return true; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.