Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Name type used for Authentication TTL #1990

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/auth/User.cc
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ Auth::User::CredentialsCacheStats(StoreEntry *output)
Auth::Type_str[auth_user->auth_type],
CredentialState_str[auth_user->credentials()],
auth_user->ttl(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This storeAppendPrintf() function call secretly assumes that ttl() returns something that can be "printed "using %d. I recommend refactoring CredentialsCacheStats() internals to use std::ostream instead, preferably in a separate PR. If that is not done, let's at least add an XXX:

Suggested change
auth_user->ttl(),
auth_user->ttl(), // XXX: Here and below, we assume that Ttl can be printed using %d

static_cast<int32_t>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl),
static_cast<Auth::Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where possible, please avoid Foo:: inside Foo:

Suggested change
static_cast<Auth::Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl),
static_cast<Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl),

or even

Suggested change
static_cast<Auth::Ttl>(auth_user->expiretime - squid_curtime + Auth::TheConfig.credentialsTtl),
static_cast<Ttl>(auth_user->expiretime - squid_curtime + TheConfig.credentialsTtl),

auth_user->username(),
SQUIDSBUFPRINT(auth_user->userKey())
);
Expand Down
11 changes: 8 additions & 3 deletions src/auth/User.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,15 @@ class User : public RefCountable
const SBuf userKey() const {return userKey_;}

/**
* How long these credentials are still valid for.
* Negative numbers means already expired.
* Credentials are not permitted to be re-used from a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we say that credentials are not permitted to be cached "unless the scheme defines..."? In other words, caching code should not cache credentials for schemes that do not override this method. That would be a stronger rule/claim than the proposed "not permitted to be re-used".

* credentials cache unless the authentication scheme
* defines a way to determine a TTL with which to bound
* the scope of their valid re-use.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expired credentials reuse is also not permitted, right? Please add that critical information to the method description. Without that restriction, the method API does not really work because the method caller cannot distinguish "scheme did not define ..." cases from "credentials expired" cases.

*
* \returns How long these credentials are still valid for.
* Negative numbers means already expired.
*/
virtual int32_t ttl() const = 0;
virtual Ttl ttl() const { return -1; }

/* Manage list of IPs using this username */
void clearIp();
Expand Down
8 changes: 4 additions & 4 deletions src/auth/basic/User.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ Auth::Basic::User::~User()
safe_free(passwd);
}

int32_t
Auth::Ttl
Auth::Basic::User::ttl() const
{
if (credentials() != Auth::Ok && credentials() != Auth::Pending)
return -1; // TTL is obsolete NOW.
return Auth::User::ttl(); // TTL is obsolete NOW.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and and in similar contexts, please either continue to return -1 or introduce a named Auth::Ttl constant and return that. Either would be more readable and safe than the proposed "use TTL computed by Auth::User::ttl() while assuming that that value is a negative TTL" change.


int32_t basic_ttl = expiretime - squid_curtime + static_cast<Auth::Basic::Config*>(config)->credentialsTTL;
int32_t global_ttl = static_cast<int32_t>(expiretime - squid_curtime + Auth::TheConfig.credentialsTtl);
const Ttl basic_ttl = expiretime - squid_curtime + static_cast<Auth::Basic::Config*>(config)->credentialsTTL;
const auto global_ttl = static_cast<Ttl>(expiretime - squid_curtime + Auth::TheConfig.credentialsTtl);

return min(basic_ttl, global_ttl);
}
Expand Down
2 changes: 1 addition & 1 deletion src/auth/basic/User.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ class User : public Auth::User

/** Update the cached password for a username. */
void updateCached(User *from);
int32_t ttl() const override;

/* Auth::User API */
Ttl ttl() const override;
static CbcPointer<Auth::CredentialsCache> Cache();
void addToNameCache() override;

Expand Down
11 changes: 5 additions & 6 deletions src/auth/digest/User.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,11 @@ Auth::Digest::User::~User()
}
}

int32_t
Auth::Ttl
Auth::Digest::User::ttl() const
{
int32_t global_ttl = static_cast<int32_t>(expiretime - squid_curtime + Auth::TheConfig.credentialsTtl);

/* find the longest lasting nonce. */
int32_t latest_nonce = -1;
Ttl latest_nonce = -1;
dlink_node *link = nonces.head;
while (link) {
digest_nonce_h *nonce = static_cast<digest_nonce_h *>(link->data);
Expand All @@ -52,9 +50,10 @@ Auth::Digest::User::ttl() const
link = link->next;
}
if (latest_nonce == -1)
return min(-1, global_ttl);
return Auth::User::ttl();

int32_t nonce_ttl = latest_nonce - current_time.tv_sec + static_cast<Config*>(Auth::SchemeConfig::Find("digest"))->noncemaxduration;
const auto global_ttl = static_cast<Ttl>(expiretime - squid_curtime + Auth::TheConfig.credentialsTtl);
const Ttl nonce_ttl = latest_nonce - current_time.tv_sec + static_cast<Config*>(Auth::SchemeConfig::Find("digest"))->noncemaxduration;

return min(nonce_ttl, global_ttl);
}
Expand Down
2 changes: 1 addition & 1 deletion src/auth/digest/User.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ class User : public Auth::User
public:
User(Auth::SchemeConfig *, const char *requestRealm);
~User() override;
int32_t ttl() const override;

/* Auth::User API */
Ttl ttl() const override;
static CbcPointer<Auth::CredentialsCache> Cache();
void addToNameCache() override;

Expand Down
2 changes: 2 additions & 0 deletions src/auth/forward.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ typedef std::vector<Auth::SchemeConfig *> ConfigVector;

class UserRequest;

using Ttl = int32_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new name is missing a description. The description must document what time unit this integer is using (e.g., seconds or time_t unit) and that negative values reflect "expired" entries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried that the suggested name -- "Auth::Ttl" -- is too general because authentication code may need several different types for TTL. For example, authenticate_ttl uses a different type today. However, I assume you consider such worries unfounded and am not requesting any related changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying type -- int32_t -- is wrong on several (probably obvious?) levels. However, I assume that this PR scope is limited to naming existing int32_t uses and excludes fixing the underlying type. Thus, I am not requesting those fixes in this PR.


} // namespace Auth

#endif /* USE_AUTH */
Expand Down
6 changes: 0 additions & 6 deletions src/auth/negotiate/User.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ Auth::Negotiate::User::~User()
debugs(29, 5, "doing nothing to clear Negotiate scheme data for '" << this << "'");
}

int32_t
Auth::Negotiate::User::ttl() const
{
return -1; // Negotiate cannot be cached.
}

CbcPointer<Auth::CredentialsCache>
Auth::Negotiate::User::Cache()
{
Expand Down
1 change: 0 additions & 1 deletion src/auth/negotiate/User.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ class User : public Auth::User
public:
User(Auth::SchemeConfig *, const char *requestRealm);
~User() override;
int32_t ttl() const override;

/* Auth::User API */
static CbcPointer<Auth::CredentialsCache> Cache();
Expand Down
6 changes: 0 additions & 6 deletions src/auth/ntlm/User.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ Auth::Ntlm::User::~User()
debugs(29, 5, "doing nothing to clear NTLM scheme data for '" << this << "'");
}

int32_t
Auth::Ntlm::User::ttl() const
{
return -1; // NTLM credentials cannot be cached.
}

CbcPointer<Auth::CredentialsCache>
Auth::Ntlm::User::Cache()
{
Expand Down
1 change: 0 additions & 1 deletion src/auth/ntlm/User.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class User : public Auth::User
public:
User(Auth::SchemeConfig *, const char *requestRealm);
~User() override;
int32_t ttl() const override;

/* Auth::User API */
static CbcPointer<Auth::CredentialsCache> Cache();
Expand Down