Fix and improve Ident (RFC 1413) code #245
Conversation
This field looks unnecessary because it basically duplicates Connection::rfc931. For example, ACLIdent::LookupDone() copied the received ident into both fields. Conceptually, ident is an attribute of client connection: it has no sense in situations here the client connection is gone (ACLIdent::match() returns ACCESS_DUNNO in such cases).
src/acl/FilledChecklist.cc
Outdated
| showDebugWarning("IDENT"); | ||
| al->cache.rfc931 = xstrdup(rfc931); | ||
| al->cache.rfc931 = xstrdup(rfc931()); |
There was a problem hiding this comment.
It looks like al->cache.rfc931 is never freed.
Keep ident name in Comm::Connection only.
src/adaptation/icap/ModXact.cc
Outdated
| @@ -1341,8 +1341,7 @@ void Adaptation::Icap::ModXact::finalizeLogInfo() | |||
| // XXX: This reply (and other ALE members!) may have been needed earlier. | |||
| al.reply = adapted_reply_; | |||
|
|
|||
| if (h->rfc931.size()) | |||
| al.cache.rfc931 = h->rfc931.termedBuf(); | |||
| al.tcpClient = h->clientConnection; | |||
There was a problem hiding this comment.
We need to decide, in ICAP context, whether ALE fields (outside of ALE::icap) represent
- the current ICAP transaction
- the corresponding master transaction
- the corresponding HTTP client-to-Squid transaction
- the corresponding HTTP Squid-to-peer transaction.
Commit bd59d61 says that http::>h and http::<h fields are for HTTP messages embedded in ICAP request and response. This does does not directly answer the question for other non-icap ALE fields, but strongly implies that other non-icap fields are meant for the ICAP transaction, not client-Squid or Squid-peer HTTP transactions or the master transaction.
Official code is inconsistent; for example, Adaptation::Icap::ModXact::finalizeLogInfo():
- sets al.cache.caddr to
virgin_request_->client_addr(i.e. HTTP client address) - sets al.cache.code to
h->logType(i.e. ICAP transaction outcome?) - calls HTTP-only (or, more precisely, ICAP-unaware) prepareLogWithRequestDetails() with adapted_request_ that probably supplies some ICAP transaction statistics to not-ICAP-specific ALE fields
I would like us to answer the primary question the following way. Long term:
- ALE should have no master transaction fields. Those fields belong to MasterXaction. One MasterXaction may correspond to many individual transaction ALEs (e.g., one HTTP client-Squid ALE, one ICAP REQMOD ALE, and one eCAP RESPMOD ALE).
- In ICAP context, ALE is for the ICAP transaction (REQMOD, RESPMOD, or OPTIONS).
- ALE::tcpClient is the primary connection of the corresponding transaction. For HTTP client-Squid transaction, that is the connection accepted by Squid. For HTTP Squid-peer transactions (that do not have a dedicated ALE today but probably should have it!), that is the connection opened by Squid. For ICAP transactions, that is the connection to the ICAP service opened by Squid.
- ALE::cache.rfc931 is the identity of the user that has connected to Squid -- a MasterXaction-level info. In the future, it will not be stored in ALE::cache but will be extracted from via client-Squid transaction ALE, where it will be found in ALE::tcpClient::rfc931. Until then, it will continue to live in ALE because MasterXaction does not have access to client-Squid transaction ALE and ACL-checking code does not have access to MasterXaction (all are complex out-of-scope TODOs).
If the above answer is the best we can give, then I suggest:
- Removing ALE::cache.rfc931
- Adding ALE::acceptedClientConnection as a temporary storage for MasterXaction-level info that future code will extract from ALE::tcpClient of a client-Squid transaction.
- Adding Adaptation::Icap::History::acceptedClientAle to remember the client-Squid transaction ALE in ICAP transactions.
- Setting ALE::acceptedClientConnection to
h->httpClientAle->acceptedClientConnectionin this ICAP code. - Documenting this answer in PR description.
There was a problem hiding this comment.
Ok, let's follow your plan.
Adding ALE::acceptedClientConnection as a temporary storage
Do you suggest creating a new type for this field or reusing Comm::Connection? In the second case, ALE::acceptedClientConnection and ALE::tcpClient will be equal for the client-Squid ALE, right?
In ICAP context, ALE is for the ICAP transaction
Does it mean that all ALE fields should represent ICAP transaction? E.g., should ALE::url and ALE::icap::reqUri be the same in this case?
There was a problem hiding this comment.
Adding ALE::acceptedClientConnection as a temporary storage
Do you suggest creating a new type for this field or reusing Comm::Connection? In the second case, ALE::acceptedClientConnection and ALE::tcpClient will be equal for the client-Squid ALE, right?
I suggest using Comm::Connection for both. During the transition, either the two fields will be equal for client-Squid ALE OR ALE code will consult both fields to extract the right information ("the primary connection of this protocol transaction" vs. "client-Squid connection of the master transaction"). The latter feels like a better option to me, but I cannot be sure without trying to implement it.
In ICAP context, ALE is for the ICAP transaction
Does it mean that all ALE fields should represent ICAP transaction? E.g., should ALE::url and ALE::icap::reqUri be the same in this case?
Eventually, yes, all ALE fields will represent the ICAP transaction and there will be no ALE::icap (but we may get protocol-specific Http::ALE and Adaptation::Icap::ALE classes!). During the transition (e.g., in this PR), we should make those decisions one (affected) field at a time, like we are doing with ALE::acceptedClientConnection above. In this PR, we do not want to start working on upgrading ALE fields that we do not have to touch.
This field should be a storage of MasterTransaction-related info (such as ident) until we split ALE into several transaction-specific ALEs, for MasterXaction, ICAP, etc., cases.
src/format/Quoting.cc
Outdated
| char * | ||
| Format::QuoteUrlEncodeUsername(const Ident::User &ident) | ||
| { | ||
| return ident ? QuoteUrlEncodeUsername(SBuf(ident.value()).c_str()) : nullptr; |
There was a problem hiding this comment.
I think the SBuf(...).c_str() construction should be safe in regard to SBuf() lifetime:
Temporary objects are destroyed as the last step in evaluating the full-expression. Expressions are defined here, for example:
if (S(3).v()) // full-expression includes lvalue-to-rvalue and int to bool conversions,
// performed before temporary is deleted at end of full-expression
Here is the rule for return statement:
(6.11)
The lifetime of a temporary bound to the returned value in a function return statement ([stmt.return]) is not extended; the temporary is destroyed at the end of the full-expression in the return statement..
So in our case the SBuf() temporary should be destroyed after the entire return value is constructed (i.e., after QuoteUrlEncodeUsername() finishes).
One should be mindful of casts applied to temporary variables.
There was a problem hiding this comment.
I agree that the destruction rules for the full-expression allow us to do what PR code is doing. Thank you for finding them.
together with USE_IDENT conditional compilation and now unused USER_IDENT_SZ.
Also added a special meaning for '-' in ident ACLs.
previously added at e701d15. Now we keep to the old master Squid behavior. ACLUserData::match() does attempt to match against an empty (or dash) value - it simply returns false in this case.
src/cf.data.pre
Outdated
| # including cases where Authentication Server returned an invalidd | ||
| # (e.g., empty) user-id. | ||
| # - A connection for which Authentication Server returned a dash character | ||
| # as a user-id. |
There was a problem hiding this comment.
I posted an adjusted version at 9841f32 without the 1st and 2nd of your listed '-' cases. In both cases checklist->ident() is undefined and it does not attempt to match().
A transaction that lacks access to a client-to-Squid connection. This case results in level 1 message"ERROR: Cannot start ident lookup" and ACCESS_DUNNO.
A connection for which Squid has not obtained user identity yet. A fast ACL makes hecklist->goAsync() fail, resulting in level 3 message "cannot start ident lookup" and ACCESS_DUNNO.
IMO the both cases indicate some general failure and we should not adjust the code to make them match '-'.
I am OK with keeping these two cases outside '-' reach, at least for now. However, I think we should add them to this documentation, as a separate list of problematic cases where this ACL does not match. The admin should be aware of all these cases when writing security-sensitive rules.
When an ACL foo returns ACCESS_DUNNO, does !foo return ACCESS_DUNNO as well? What about an any-of and all-of ACLs? Will they return ACCESS_DUNNO if at least one of their nodes returns ACCESS_DUNNO?
There was a problem hiding this comment.
!foo return ACCESS_DUNNO as well?
Yes.
What about an any-of and all-of ACLs?
any-of: ACCESS_DUNNO if ACCESS_DUNNO is returned by the first ACL. It will be ALLOWED, e.g., if the first ACL returns ALLOWED and the second ACL returns ACCESS_DUNNO.
all-of: ACCESS_DUNNO if one of the ACLs returns ACCESS_DUNNO.
There was a problem hiding this comment.
any-of: ACCESS_DUNNO if ACCESS_DUNNO is returned by the first ACL.
OK.
It will be ALLOWED, e.g., if the first ACL returns ALLOWED and the second ACL returns ACCESS_DUNNO.
If the first any-of ACL node returns ALLOWED, do we really evaluate the second node? I would expect the evaluation to stop once the result is known.
If we are only looking at allow/deny/dunno outcomes, then I would expect any-of to result in
- ALLOWED if at least one ACL node returned ALLOWED,
- DENIED if all ACL nodes returned DENIED,
- DUNNO otherwise (i.e. at least one node returned DUNNO and no nodes returned ALLOWED)
Do my expectations match reality?
There was a problem hiding this comment.
we should add them to this documentation, as a separate list of problematic cases
Done (42e71c0)
There was a problem hiding this comment.
If the first any-of ACL node returns ALLOWED, do we really evaluate the second node?
No. Also, my test showed that if the first any-of ACL returns DUNNO and the second - ALLOWED, we also do not evaluate the second rule and return DUNNO.
ALLOWED if at least one ACL node returned ALLOWED,
Yes for ALLOWED/DENIED and No for ALLOWED/DUNNO, e.g., ALLOWED,DUNNO results in ALLOWED and DUNNO,ALLOWED results in DUNNO (the second ACL in both cases is not considered).
DUNNO (i.e. at least one node returned DUNNO and no nodes returned ALLOWED)
Yes, both DENIED,DUNNO and DUNNO,DENIED result in DUNNO.
There was a problem hiding this comment.
I sense a terminology conflict in our last few exchanges:
-
When I say "ACL X returned Y", I mean "Evaluation of ACL X resulted in Y". In other words, I am talking about what is actually happening during Checklist application to some Squid directive.
-
When you read my "ACL X returned Y", you are probably thinking "An evaluation of ACL X would result in Y". In other words, you are thinking about how that ACL X is written/configured.
For example, my "ALLOWED if at least one ACL node returned ALLOWED" means "ALLOWED if at least one evaluated ACL node returned ALLOWED" rather than "ALLOWED if at least one ACL node would have returned ALLOWED if it were evaluated".
Here is the same expectation adjusted to make the above clarification explicit:
I expect ACLs evaluation to stop once the result is known. If we are only looking at allow/deny/dunno outcomes, then I would expect any-of evaluation to result in:
- ALLOWED if an evaluated ACL node returned ALLOWED,
- DENIED if all evaluated ACL nodes returned DENIED,
- DUNNO otherwise (i.e. an evaluated ACL returned DUNNO and no previously evaluated ACLs returned ALLOWED)
Do you agree that my bullets match the current official code?
There was a problem hiding this comment.
I did not re-run the tests, but IIRC once a node results in ALLOWED or DUNNO, the entire any-of ACL returns that result:
ALLOWED,DUNNO results in ALLOWED and DUNNO,ALLOWED results in DUNNO (the second ACL in both cases is not considered).
This goes in line with your (1) and (3) bullets.
i.e. an evaluated ACL returned DUNNO and no previously evaluated ACLs returned ALLOWED
or "an evaluated ACL returned DUNNO and all previously evaluated ACLs returned DENIED".
It looks like (2) bullet is the product of (1) and (3).
There was a problem hiding this comment.
It looks like (2) bullet is the product of (1) and (3).
Yes, it is. Moreover, here, any one bullet is the product of the other two because these three mutually exclusive bullets cover all possible in-scope cases. We can move that "X otherwise" shortcut phrase (with the right X) from the third bullet to any other bullet.
... and explain why neither is necessary today. This change keeps similar strOrNull() and getClientIdent() code in sync. P.S. strOrNull() has been unnecessary since inception in 2011 commit bd85ea1.
Developers forget to call isEmpty() and reviewers often miss problems
they cannot see (e.g., when Squid reports an empty string instead of an
Ident lookup error). Compilers detect mismatching types very well.
Also clarify type/member meaning and provide more/better visual clues to
distinguish "no information about ident lookup" from "no information
about user identity" conditions by naming information types rather than
their std::optional wrappers. For example, Ident::User should be about
the user info, not about whether that info exists; different Ident::User
storage contexts may interpret "does not exist" differently!
The above clarification needs more work to better expose bugs like the
following branch-added bug fixed in this commit (creating an infinite
goAsync() loop when all Ident lookups fail):
if (!ch->ident())
ch->goAsync(ACLIdent::StartLookup, *this);
TODO: Consider using a custom Ident::UserLookup type, so that we can
provide a dedicated printing operator for it and easily report ident
information in more contexts. One the other hand, the three reports
we already have differ quite a bit:
* Comm::Connection::setIdent()
* Comm::operator << (std::ostream &os, const Connection &conn)
* LFT_USER_NAME and LFT_USER_IDENT in Format::assemble() and friends
When code needs an Ident user ID, we need to decide whether to call ACLIdent::StartLookup() to obtain that ID. That decision logic is far from trivial and decision mistakes have led to bugs: * StartLookup() asserts if certain conditions are not satisfied. * Checklist code loops if StartLookup() is called when user ID is known. * Checklist code loops if StartLookup() cannot store obtained user ID. We now encapsulate that logic in a dedicated ACLIdent method.
Closely related code used inconsistent sources of client Connection objects when inspecting or setting Ident::Lookup: * ACLIdent::match() checked mgr or ALE Connection * ACLIdent::ShouldStartLookup() checked mgr Connection * Ident.cc updated mgr Connection Now all three use mgr or ALE Connection.
AccessLogEntry::acceptedClientConnection was added based on #245 (comment) I believe that framework reasoning and long-term plans were overall correct except for tcpClient interpretation/plans. And with that interpretation corrected, there is a better way to address the immediate problem at hand than adding acceptedClientConnection hack to ALE. I described (future) ALE::tcpClient as eventually representing "the primary connection of the corresponding transaction. ... For HTTP Squid-peer transactions, that is the connection opened by Squid". Given tcpClient use in current Squid code (where ICAP does not set it at all and all users [can be interpreted as if they] extract accepted-connection information like %ui from it), that was not the best model. The field should be viewed as representing the connection accepted by Squid. It is a MasterXaction-level info (duplicated in ALE). A connection opened by FwdState or by an ICAP RESPMOD transaction should not be stored in tcpClient, even if/when those transactions gain their own ALEs (those future ALE-like types might have something like a primaryConnection field for their connections). If ALE::tcpClient represents the connection accepted by Squid (at one of its proto_ports), then ICAP code should use that field to extract Ident user identity (for the purposes of %ui logging and ident_regex matching) because all related %codes and ACLs are defined in terms of the user opening a connection to a Squid port (and _not_ the user of a connection Squid opens to an HTTP peer or an ICAP service!).
I do not know whether this code is needed in some cases, but it feels wrong to silently remove it, especially when a comment at the beginning of ACLFilledChecklist::verifyAle() implies that this code may be useful. Code details have changed because ALE no longer stores cache.rfc931 directly, but tcpClient in new code is needed to extract that same Ident info.
rousskov
changed the title
Repeated updateIdent() calls for the same Connection are probably relatively rare, but, when they happen, they ought to carry the same lookup value. Do not complicate this code for now, at least until we discover specific reasons to do more than just assign the new value. TODO: Ideally, global ident_hash should be replaced with a FIFO array of lookup awating clients inside a new PendingIdent class, with Ident::Lookup adjusted to add a "pending lookup" state, so that an Comm::Connection object can keep all queued lookup recipients for that specific object. The new Ident::PendingLookup state will keep the corresponding Comm::Connection pointer, so that Ident code updates Connection::identLookup once. Due to significant code changes and lack of automated Ident testing, this low-priority improvement/optimization deserves a dedicated project/branch.
XXX: The new printing code causes linking error in several unit tests.
This reverts previous branch commit c992599. Removed code was on the right track IMO, but I am not sure that improving Ident debugging should be done on this branch, especially since it requires adjusting unit test linking instructions.
| const auto clientConnection = cl.acceptedConnection(); | ||
| // caller should use ShouldStartLookup() to check these preconditions | ||
| assert(clientConnection); | ||
| assert(Comm::IsConnOpen(clientConnection)); // TODO: Remove as unused/unnecessary |
There was a problem hiding this comment.
Don't we need also to assert(!clientConnection.identLookup)?
There was a problem hiding this comment.
We could, but I would not: IIRC, that invariant is not used by (and is not technically necessary for) the Ident transaction code initiated below these assertions. That Ident transaction does not even call setIdent()/updateIdent() -- it does not know where the lookup outcome will be stored!
FWIW, I have added "Remove as unused/unnecessary" TODO for the other assertion for similar reasons -- we should not assert unused/unnecessary/inapplicable conditions, even if they are likely to be true in current code.
Yes, Ident transaction code may lookup identity of an already identified user, but some future hypothetical caller may have a reason for doing that (e.g., an identity cache refresh for long-lived connections?).
If we do add such an assertion now, then we must also add a ShouldStartLookup() check to ConnStateData::whenClientIpKnown(). Perhaps we should do that even if we do not add that assertion. My primary worry there is that it might trigger requests to merge ShouldStartLookup() and ACLIdent::StartLookup() calls as far as public API is concerned -- it may be difficult to merge them correctly (and to convince others that it is too difficult). Adding this assert may also trigger requests to move updateIdent() calls from callbacks to Ident transaction code. Again, such requests may not be wrong in principle, but they will increase this PR complexity further. I was hoping that those improvements can be done later if somebody wants to polish this rarely used code further.
I do realize that the code above implies that ShouldStartLookup() should be called. I think that misleading C++ comment should be removed (instead of adding an assertion). I was probably trying to fix inappropriate match() reference in the official comment, but my replacement was problematic.
Please let me know if any of my assumptions are false. If you agree with my reasoning, let's remove "caller should use..." comment and add a ShouldStartLookup() check to ConnStateData::whenClientIpKnown().
There was a problem hiding this comment.
let's remove "caller should use..." comment and add a ShouldStartLookup() check
Agreed, done at 7e707a5. Should we also remove the Comm::IsConnOpen(...) assertion?
There was a problem hiding this comment.
Should we also remove the Comm::IsConnOpen(...) assertion?
Unless this removal is requested by others, I would keep this assertion (and the corresponding check in ShouldStartLookup()) in this PR to emphasize that we are not changing that precondition. My primary concern here is whether Ident lookup should be done for already closed connections (from general Ident principles unrelated to Squid Ident implementation). I do not know enough about these Ident services to address that concern.
| // requires a lookup) is evaluated while a lookup attempt is still pending. | ||
| // Ident::IdentStateData::notify() makes calls with the same lookup value. | ||
| static const Ident::User lookupError("[ident-error]"); | ||
| if (identLookup) |
There was a problem hiding this comment.
a4857e9 : Repeated updateIdent() calls for the same Connection are probably relatively rare
These repeated calls are always performed in configurations with configured ident_lookup_access (and ident-using ACLs). The initial ident request is performed via ConnStateData::start()->ConnStateData::whenClientIpKnown().
There was a problem hiding this comment.
Agreed. That "rare" assertion in commit a4857e9 message was wrong/sloppy -- these repeated calls are frequent in certain configurations, and those configurations may not be so rare if we only look at configurations where Ident is actively used.
Fortunately, the "when they happen..." part is correct/accurate. I still think that code simplification in that commit was the right thing to do overall.
... instead of RFC 931 "Authentication Server" terminology.
|
This work became official PR 1815 and official PR 1818. |
The above assertion was caused by ACLFilledChecklist constructor calling
setIdent() twice: via ACLFilledChecklist::setRequest() and directly. The
bug was added in 2021 commit e227da8.
Also fixed a memory leak created by ACLFilledChecklist::verifyAle() that
allocated a never-freed ALE::cache.rfc931 memory if ALE was missing an
available user ID. Configurations with external ACLs leaked a user ID
for each transaction that had one. Squid logged up to 99 "ALE missing
IDENT" messages in those cases. Squid leaked since 2015 commit fbbea66.
These rfc931 field maintenance bugs were caused by duplication of its
storage across three different classes: AccessLogEntry::cache,
Comm::Connection, and Icap::History. Now, this info is only kept in the
corresponding Comm::Connection object which is already accessible from
most contexts via ACLFilledChecklist::conn() and ALE::tcpClient.
ICAP code was missing access to ALE::tcpClient of the originating HTTP
transaction, so we added one more Icap::History field to pass such info.
Squid recognizes or treats specially several Ident-related states:
To improve our chances of correctly fixing the above bugs and preventing
similar ones, we upgraded Ident-related information storage from a
fixed-size 64-byte array (with magic first character values to signal
Ident states) to two new named types in Ident namespace.
Also improved "ident" and "ident_regex" ACL documentation that did not
disclose many matching quirks and dangerously stated that ident_regex
ACL treats REQUIRED parameter value specially, like "ident" ACL does
(ident_regex has not done that since inception in 2000 commit 145cf92).
Also removed --disable-ident-lookups ./configure option and related
macros. Ident code was already compiled by default, and the option to
disable that code lost its (only) optimization purpose when we removed
hard-coded 64-byte Connection::rfc931 arrays and their copies.
Also improved error-reporting code when parsing user-id in ident-reply,
but problematic parsing code logic remains essentially unchanged.
Obsoleted Ident RFC 931 uses stale terminology and is less detailed than
its RFC 1413 replacement. For example, the requirement for user-id to
contain at least one octet comes from RFC 1413. Squid has followed that
particular RFC 1413 requirement already. We updated obsoleted RFC
references. Most of the affected text had to be modified anyway.
TODO: Fix REQUIRED in