Support ttl=N from helpers #1662

yadij · 2024-02-06T07:55:48Z

Currently only used for the Basic authentication helpers where it
overwrites the auth_param credentialsttl configuration.

rousskov

I support adding this feature.

src/helper/Reply.h

src/helper/Reply.cc

src/cf.data.pre

src/auth/basic/UserRequest.cc

rousskov · 2024-02-06T15:46:07Z

src/helper/Reply.cc

@@ -232,9 +232,13 @@ Helper::Reply::parseResponseKeys()
 // TODO: Convert the above code to use Tokenizer and SBuf
 const SBuf parsedKey(key);
 const SBuf parsedValue(v); // allow empty values (!v or !*v)
- CheckReceivedKey(parsedKey, parsedValue);
- notes.add(parsedKey, parsedValue);


I see no good reason to make this old code conditional on the annotation not being called "ttl". The official code already handles existing Squid-specific helper annotations like "group" and "tag". Why make the new ttl special in that regard?

This key pertains to the re-use of the exact helper response that it is part of. Other annotations are relevant in higher levels of abstraction and make sense preserving across the potentially multiple helpers a transaction can use.

This key pertains to the re-use of the exact helper response that it is part of. Other annotations are relevant in higher levels of abstraction and make sense preserving across the potentially multiple helpers a transaction can use.

A PR may propose a new from-helper annotation categorization scheme, of course, but this should be done explicitly (e.g., by adding a clear definition of categories using C++ comments), and the categorization should be applied to existing/old supported annotations. Just adding do not do this for annotations named foo code is not good enough because it is not clear whether the new code should apply to some old annotations and whether some future bar annotation would need to be added to that code or excluded from it. Addressing this change request requires PR code modifications.

This change request is about (and is attached to) two independent features, represented by two old code lines:

The alleged "ttl vs. other annotations" differences are not relevant to CheckReceivedKey() at all.

As for (not) adding the new annotation to notes, some of the existing/old annotations have the same "single helper response" scope AFAICT; once you define the new category, please double check whether any of the existing annotations belong to it. Also, if this PR starts excluding some annotation categories from notes, then all notes uses must be checked to make sure the exclusion is valid/desirable. I would not be surprised if that audit results in preservation of the old code updating notes for all annotation categories and/or code to preserve/replicate the old notes functionality for the new category of annotations.

src/helper/Reply.h

src/helper/Reply.cc

src/auth/basic/UserRequest.cc

Co-authored-by: Alex Rousskov <[email protected]>

rousskov

This is as far as I could get without an expires data member description.

rousskov · 2024-02-18T23:35:02Z

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

+ <p>Changed <em>basic</em> scheme <em>credentialsttl</em> option
+ to prefer helper provided <em>ttl=N</em> value (if any).


It is not very clear what "prefer" is in this context. I suggest being more specific, but I do not insist on changing the current PR wording.

Suggested change

Changed basic scheme credentialsttl option

to prefer helper provided ttl=N value (if any).

Ignore basic scheme credentialsttl option

if helper has returned ttl=N.

rousskov · 2024-02-18T23:49:40Z

src/auth/User.h

@@ -50,7 +50,7 @@ class User : public RefCountable
 Auth::SchemeConfig *config;
 dlink_list proxy_match_cache;
 size_t ipcount;
- long expiretime;
+ time_t expires = 0;


Please add a description for the expires field. AFAICT, this PR changes this field semantics. I may be missing something, but I cannot find the meaning of the new/changed field based on PR code alone because PR code seems to treat expiretme/expires replacement inconsistently. I hope the description will confirm that PR code is correct (or expose PR bugs).

rousskov · 2024-02-18T23:53:19Z

src/auth/User.cc

@@ -256,7 +255,7 @@ Auth::User::CredentialsCacheStats(StoreEntry *output)
 Auth::Type_str[auth_user->auth_type],
 CredentialState_str[auth_user->credentials()],
 auth_user->ttl(),
- static_cast<int32_t>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl),
+ static_cast<int32_t>(auth_user->expires - current_time.tv_sec),


For consistency (all other ~4 similar expressions use squid_curtime) and simplicity sake:

Suggested change

static_cast<int32_t>(auth_user->expires - current_time.tv_sec),

static_cast<int32_t>(auth_user->expires - squid_curtime),

rousskov · 2024-02-18T23:59:07Z

src/auth/basic/UserRequest.cc

@@ -56,17 +56,13 @@ Auth::Basic::UserRequest::authenticate(HttpRequest *, ConnStateData *, Http::Hdr
 return;

 /* are we about to recheck the credentials externally? */
- if ((user()->expiretime + static_cast<Auth::Basic::Config*>(Auth::SchemeConfig::Find("basic"))->credentialsTTL) <= squid_curtime) {
+ if (user()->expires <= current_time.tv_sec) {


Similar to the other change request (and please check for similar occurrences):

Suggested change

if (user()->expires <= current_time.tv_sec) {

if (user()->expires <= squid_curtime) {

rousskov · 2024-02-19T00:08:17Z

I have adjusted the following phrase in PR description because we still support credentialsttl configuration, even for Basic authentication, AFAICT:

- replaces the auth_param credentialsttl configuration.
+ overwrites the auth_param credentialsttl configuration.

rousskov requested changes Feb 6, 2024

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Feb 6, 2024

yadij marked this pull request as draft February 7, 2024 06:35

yadij and others added 8 commits February 15, 2024 06:47

Support ttl=N from helpers

7abb546

Review requests

c4c9712

fixup

d40b5da

Update src/helper/Reply.h

0bead34

Co-authored-by: Alex Rousskov <[email protected]>

Log ignored 'ttl=N' received from helpers

8d4f6e1

Revert change to NTLM and Nagotiate auth behaviour

7a34208

typo in debugs

92e8b04

Detect and report ttl=BAD values

4365dcd

yadij force-pushed the arc-helper-ttl branch from 47d75af to 4365dcd Compare February 14, 2024 19:46

yadij marked this pull request as ready for review February 14, 2024 19:54

yadij 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 Feb 14, 2024

yadij requested a review from rousskov February 14, 2024 19:54

yadij added the feature maintainer needs documentation updates for merge label Feb 14, 2024

Release notes for configuration changes

9c95277

rousskov requested changes Feb 19, 2024

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ttl=N from helpers #1662

Support ttl=N from helpers #1662

yadij commented Feb 6, 2024 •

edited by rousskov

rousskov left a comment

rousskov Feb 6, 2024

yadij Feb 7, 2024

rousskov Feb 7, 2024

rousskov left a comment

rousskov Feb 18, 2024

rousskov Feb 18, 2024

rousskov Feb 18, 2024

rousskov Feb 18, 2024

rousskov commented Feb 19, 2024 •

edited

		<p>Changed <em>basic</em> scheme <em>credentialsttl</em> option
		to prefer helper provided <em>ttl=N</em> value (if any).

	static_cast<int32_t>(auth_user->expires - current_time.tv_sec),
	static_cast<int32_t>(auth_user->expires - squid_curtime),

	if (user()->expires <= current_time.tv_sec) {
	if (user()->expires <= squid_curtime) {

Support ttl=N from helpers #1662

Are you sure you want to change the base?

Support ttl=N from helpers #1662

Conversation

yadij commented Feb 6, 2024 • edited by rousskov

rousskov left a comment

Choose a reason for hiding this comment

rousskov Feb 6, 2024

Choose a reason for hiding this comment

yadij Feb 7, 2024

Choose a reason for hiding this comment

rousskov Feb 7, 2024

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

rousskov Feb 18, 2024

Choose a reason for hiding this comment

rousskov Feb 18, 2024

Choose a reason for hiding this comment

rousskov Feb 18, 2024

Choose a reason for hiding this comment

rousskov Feb 18, 2024

Choose a reason for hiding this comment

rousskov commented Feb 19, 2024 • edited

yadij commented Feb 6, 2024 •

edited by rousskov

rousskov commented Feb 19, 2024 •

edited