Remove Ident protocol support #1827

eduard-bagdasaryan · 2024-05-30T16:21:31Z

Ident protocol (RFC 931 obsoleted by RFC 1413) has been considered
seriously insecure and broken since at least 2009 when SANS issued an
update recommending its removal from all networks. Squid Ident
implementation suffered from assertions (since 2021 commit e227da8) and
memory leaks (since 2015 commit fbbea66). Ident implementation design
increased ACLChecklist::goAsync() design complexity.

An external ACL helper can be written to perform Ident transactions.

Configurations using ident/ident_regex ACLs, %ui logformat codes, %IDENT
external_acl_type format code, or ident_lookup_access/ident_timeout
directives are now rejected, leading to fatal startup failures:

FATAL: Invalid ACL type 'ident_regex'    
FATAL: Invalid ACL type 'ident'
ERROR: configuration failure: Unsupported %code: '%ui'
ERROR: configuration failure: Unsupported %code: '%IDENT'
squid.conf(6): unrecognized: 'ident_lookup_access'
squid.conf(7): unrecognized: 'ident_timeout'

To avoid inconveniencing admins that do not use Ident features, access
logs with "common" and "combined" logformats now always receive a dash
in the position of what used to be a %ui record field.

Co-authored-by: Amos Jeffries squid3@treenet.co.nz

and other fixes.

…layers

src/peer_select.cc

rousskov

@yadij, I asked Eduard to list you as this PR co-author (see current PR description footer) because this PR is using your justification for Ident support removal. Beyond that justification, this PR bugs are not your responsibility, of course.

src/cf.data.pre

yadij

Thank you. Just some polish.

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

yadij · 2024-05-30T21:49:42Z

src/acl/FilledChecklist.h

@@ -117,7 +114,7 @@ class ACLFilledChecklist: public ACLChecklist
    err_type requestErrorType;

 private:
-    ConnStateData * conn_;          /**< hack for ident and NTLM */
+    ConnStateData * conn_; ///< client-to-Squid connection manager (if any)


Per @rousskov guidelines about AsyncJob usage we should not retain pointers to them. Please preserve the comment clarifying that this is a hack

Suggested change

ConnStateData * conn_; ///< client-to-Squid connection manager (if any)

ConnStateData * conn_; ///< client-to-Squid connection manager (if any). hack for NTLM

we should not retain pointers to them

But what should we retain? A CbcPointer to that job instead? Could you please provide a direct link to those guidelines? I do not mind adjusting this comment but would prefer something more meaningful than keeping that vague 'hack for...' phrasing.

Amos: Per @rousskov guidelines about AsyncJob usage we should not retain pointers to them.

There is no such guideline -- it would make communicating with a job impossible.

One of the actual guidelines is not to communicate with a started AsyncJob using synchronous means. This guideline is primarily targeting use cases where we want to change that started job state. Such changes should be done via asynchronous calls. Extracting information from a running job using a weak job pointer is a bit of a gray area -- ideally, it should be avoided, but the true risks/problems are much lower there.

Amos: Please preserve the comment clarifying that this is a hack

I hope my branch commit a0f1b74 addresses your concern. Please keep in mind that this hack is used for a lot more than NTLM these days.

Eduard: I ... would prefer something more meaningful than keeping that vague 'hack for...' phrasing.

Let's not block on polishing that wording in this PR. Preserving out-of-scope difficult-to-fix problems is fine!

My understanding is that a) state that needs to be shared outside the Job itself is to be stored in a separate RefCounted object, and b) Job actions that need to be triggered or responses to pass back to a Job should use AsyncCall with a Pointer (or Subscription). Why? because the AsyncJob may self-delete/deallocate at any time due to its own reasons.

I do not insist on a change of the raw-pointer here. Just to keep the "hack" wording. Which is done. Thank you.

My understanding is that a) state that needs to be shared outside the Job itself is to be stored in a separate RefCounted object

Yes, RefCount is one way to do that, but not the only one. Depending on the state and sharing needs, other information sharing mechanisms may be appropriate (e.g., copying or cbdata). There are too many different use cases to cover everything with RefCount.

and b) Job actions that need to be triggered or responses to pass back to a Job should use AsyncCall

Yes, and to call a job, one needs to store a pointer¹ to that job. This is why there cannot be a "should not retain pointers to jobs" guideline.

with a Pointer (or Subscription).

Yes (or other proper means). Again, there are too many different use cases to cover everything with one approach (or two).

Why? because the AsyncJob may self-delete/deallocate at any time due to its own reasons.

Indeed. Moreover, a job might be destroyed for reasons outside job's direct control -- we already have examples of jobs killing other (unsuspecting) jobs.

Someday, this conn_ hack (that violates several guidelines!) will be gone because the information we extract via conn_ today will be available in MasterXaction and ALE.

Thank you for updating your review and clearing this PR!

Footnotes

That job pointer may be hidden inside a stored AsyncCall, but there are other possibilities as well. ↩

src/cf.data.pre

src/peer_select.cc

src/ssl/PeekingPeerConnector.cc

eduard-bagdasaryan

Polished at 989b9c0.

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

eduard-bagdasaryan · 2024-05-31T11:19:44Z

src/acl/FilledChecklist.h

@@ -117,7 +114,7 @@ class ACLFilledChecklist: public ACLChecklist
    err_type requestErrorType;

 private:
-    ConnStateData * conn_;          /**< hack for ident and NTLM */
+    ConnStateData * conn_; ///< client-to-Squid connection manager (if any)


we should not retain pointers to them

But what should we retain? A CbcPointer to that job instead? Could you please provide a direct link to those guidelines? I do not mind adjusting this comment but would prefer something more meaningful than keeping that vague 'hack for...' phrasing.

src/peer_select.cc

src/ssl/PeekingPeerConnector.cc

rousskov · 2024-05-31T13:21:04Z

src/acl/FilledChecklist.h

@@ -117,7 +114,7 @@ class ACLFilledChecklist: public ACLChecklist
    err_type requestErrorType;

 private:
-    ConnStateData * conn_;          /**< hack for ident and NTLM */
+    ConnStateData * conn_; ///< client-to-Squid connection manager (if any)


Amos: Per @rousskov guidelines about AsyncJob usage we should not retain pointers to them.

There is no such guideline -- it would make communicating with a job impossible.

One of the actual guidelines is not to communicate with a started AsyncJob using synchronous means. This guideline is primarily targeting use cases where we want to change that started job state. Such changes should be done via asynchronous calls. Extracting information from a running job using a weak job pointer is a bit of a gray area -- ideally, it should be avoided, but the true risks/problems are much lower there.

Amos: Please preserve the comment clarifying that this is a hack

I hope my branch commit a0f1b74 addresses your concern. Please keep in mind that this hack is used for a lot more than NTLM these days.

Eduard: I ... would prefer something more meaningful than keeping that vague 'hack for...' phrasing.

Let's not block on polishing that wording in this PR. Preserving out-of-scope difficult-to-fix problems is fine!

src/cf.data.pre

src/peer_select.cc

kinkie · 2024-06-01T09:33:33Z

Looks good enough to me; approving so it can be fast-landed as soon as @yadij has a chance to review

Ident protocol (RFC 931 obsoleted by RFC 1413) has been considered seriously insecure and broken since at least 2009 when SANS issued an update recommending its removal from all networks. Squid Ident implementation suffered from assertions (since 2021 commit e227da8) and memory leaks (since 2015 commit fbbea66). Ident implementation design increased ACLChecklist::goAsync() design complexity. An external ACL helper can be written to perform Ident transactions. Configurations using ident/ident_regex ACLs, %ui logformat codes, %IDENT external_acl_type format code, or ident_lookup_access/ident_timeout directives are now rejected, leading to fatal startup failures: FATAL: Invalid ACL type 'ident_regex' FATAL: Invalid ACL type 'ident' ERROR: configuration failure: Unsupported %code: '%ui' ERROR: configuration failure: Unsupported %code: '%IDENT' squid.conf(6): unrecognized: 'ident_lookup_access' squid.conf(7): unrecognized: 'ident_timeout' To avoid inconveniencing admins that do _not_ use Ident features, access logs with "common" and "combined" logformats now always receive a dash in the position of what used to be a %ui record field. Co-authored-by: Amos Jeffries <squid3@treenet.co.nz>

eduard-bagdasaryan and others added 8 commits May 23, 2024 16:09

Removed Ident support

3f8bce2

Merged from master

f03a2fe

Removed an extra space

ba92a12

Polished with autos

5373289

Added list of changes to release-notes

744f5b2

and other fixes.

Removed %IDENT from doc/manuals and 'time_quota' helper

728759f

fixup: Polished release notes

d344847

fixup: Removed dropped ./configure option from test-suite/buildtests/…

5bfd9f9

…layers

eduard-bagdasaryan commented May 30, 2024

View reviewed changes

src/peer_select.cc Show resolved Hide resolved

eduard-bagdasaryan mentioned this pull request May 30, 2024

Fix and improve Ident (RFC 1413) code #1815

Closed

rousskov previously approved these changes May 30, 2024

View reviewed changes

src/cf.data.pre Show resolved Hide resolved

kinkie previously approved these changes May 30, 2024

View reviewed changes

yadij requested changes May 30, 2024

View reviewed changes

yadij added the S-waiting-for-author author action is expected (and usually required) label May 30, 2024

Polished release notes per review comment

989b9c0

eduard-bagdasaryan dismissed stale reviews from kinkie and rousskov via 989b9c0 May 31, 2024 12:09

eduard-bagdasaryan commented May 31, 2024

View reviewed changes

rousskov added 2 commits May 31, 2024 09:12

fixup: Preserve "hack" annotation for ACLFilledChecklist::conn_

a0f1b74

fixup: Wrapped excessively long line

94562a3

rousskov approved these changes May 31, 2024

View reviewed changes

rousskov requested a review from yadij May 31, 2024 13:44

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 May 31, 2024

kinkie approved these changes Jun 1, 2024

View reviewed changes

yadij approved these changes Jun 2, 2024

View reviewed changes

yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jun 2, 2024

squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jun 2, 2024

squid-anubis closed this Jun 2, 2024

kinkie mentioned this pull request Jun 9, 2024

coverity cid 1461163 #1838

Closed

rousskov mentioned this pull request Jul 8, 2024

Removed effectively unused acl_checklist members #1860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Ident protocol support #1827

Remove Ident protocol support #1827

eduard-bagdasaryan commented May 30, 2024

rousskov left a comment

yadij left a comment

yadij May 30, 2024

eduard-bagdasaryan May 31, 2024

rousskov May 31, 2024

yadij Jun 2, 2024

yadij Jun 2, 2024 •

edited

Loading

rousskov Jun 2, 2024

eduard-bagdasaryan left a comment

eduard-bagdasaryan May 31, 2024

rousskov May 31, 2024

kinkie commented Jun 1, 2024

	ConnStateData * conn_; ///< client-to-Squid connection manager (if any)
	ConnStateData * conn_; ///< client-to-Squid connection manager (if any). hack for NTLM

Remove Ident protocol support #1827

Remove Ident protocol support #1827

Conversation

eduard-bagdasaryan commented May 30, 2024

rousskov left a comment

Choose a reason for hiding this comment

yadij left a comment

Choose a reason for hiding this comment

yadij May 30, 2024

Choose a reason for hiding this comment

eduard-bagdasaryan May 31, 2024

Choose a reason for hiding this comment

rousskov May 31, 2024

Choose a reason for hiding this comment

yadij Jun 2, 2024

Choose a reason for hiding this comment

yadij Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

rousskov Jun 2, 2024

Choose a reason for hiding this comment

Footnotes

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

eduard-bagdasaryan May 31, 2024

Choose a reason for hiding this comment

rousskov May 31, 2024

Choose a reason for hiding this comment

kinkie commented Jun 1, 2024

yadij Jun 2, 2024 •

edited

Loading