Skip to content

Add -g option for custom group attribute #2395

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.

Sign up for GitHub

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

Open
ankor2023 wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add -g option for custom group attribute #2395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
78d14c1
Update negotiate_kerberos.h
ankor2023 Mar 26, 2026
92b0049
Add files via upload
ankor2023 Mar 26, 2026
f6d7cca
Add files via upload
ankor2023 Mar 27, 2026
d8d5231
Add files via upload
ankor2023 Mar 27, 2026
94d93ab
Add files via upload
ankor2023 Mar 27, 2026
987a6dd
Add files via upload
ankor2023 Mar 27, 2026
34375ad
Update negotiate_kerberos_auth.cc
ankor2023 Mar 27, 2026
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
31 changes: 25 additions & 6 deletions src/auth/negotiate/kerberos/negotiate_kerberos_auth.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ main(int argc, char *const argv[])
#else
gss_buffer_desc type_id = GSS_C_EMPTY_BUFFER;
#endif
const char* group_attribute_name = "group";
#endif /* HAVE_KRB5_PAC_SUPPORT */
krb5_context context = nullptr;
krb5_error_code ret;
Expand Down Expand Up @@ -366,7 +367,7 @@ main(int argc, char *const argv[])
setbuf(stdout, nullptr);
setbuf(stdin, nullptr);

while (-1 != (opt = getopt(argc, argv, "dirs:k:c:t:"))) {
while (-1 != (opt = getopt(argc, argv, "dirs:k:c:t:a:"))) {
switch (opt) {
case 'd':
debug_enabled = 1;
Expand Down Expand Up @@ -462,19 +463,37 @@ main(int argc, char *const argv[])
exit(EXIT_FAILURE);
}
break;
#if HAVE_KRB5_PAC_SUPPORT
case 'a':
if (optarg)
group_attribute_name = optarg;
else {
fprintf(stderr, "ERROR: '-a' value not given\n");
exit(EXIT_FAILURE);
}
break;
#endif
default:
fprintf(stderr, "Usage: \n");
fprintf(stderr, "squid_kerb_auth [-d] [-i] [-s SPN] [-k keytab] [-c rcdir] [-t rctype]\n");
fprintf(stderr, "squid_kerb_auth [-d] [-i] [-s SPN] [-k keytab] [-c rcdir] [-t rctype]");
#if HAVE_KRB5_PAC_SUPPORT
fprintf(stderr, " [-a name]");
#endif
fprintf(stderr, "\n");
fprintf(stderr, "-d full debug\n");
fprintf(stderr, "-i informational messages\n");
fprintf(stderr, "-r remove realm from username\n");
fprintf(stderr, "-s service principal name\n");
fprintf(stderr, "-k keytab name\n");
fprintf(stderr, "-c replay cache directory\n");
fprintf(stderr, "-t replay cache type\n");
#if HAVE_KRB5_PAC_SUPPORT
fprintf(stderr, "-a annotation name for reporting user groups (e.g., 'clt_conn_tag'); defaults to '%s'\n",
group_attribute_name);
#endif
fprintf(stderr,
"The SPN can be set to GSS_C_NO_NAME to allow any entry from keytab\n");
fprintf(stderr, "default SPN is HTTP/fqdn@DEFAULT_REALM\n");
"\nThe SPN can be set to GSS_C_NO_NAME to allow any entry from keytab\n");
fprintf(stderr, "default SPN is HTTP/fqdn@DEFAULT_REALM\n\n");
exit(EXIT_SUCCESS);
}
}
Expand Down Expand Up @@ -786,7 +805,7 @@ main(int argc, char *const argv[])

rfc_user = rfc1738_escape(user);
#if HAVE_KRB5_PAC_SUPPORT
fprintf(stdout, "OK token=%s user=%s %s\n", token, rfc_user, ag?ag:"group=");
fprintf(stdout, "OK token=%s user=%s %s=%s\n", token, rfc_user, group_attribute_name, ag?ag:"");
#else
fprintf(stdout, "OK token=%s user=%s\n", token, rfc_user);
#endif /* HAVE_KRB5_PAC_SUPPORT */
Expand Down Expand Up @@ -828,7 +847,7 @@ main(int argc, char *const argv[])
}
rfc_user = rfc1738_escape(user);
#if HAVE_KRB5_PAC_SUPPORT
fprintf(stdout, "OK token=%s user=%s %s\n", "AA==", rfc_user, ag?ag:"group=");
fprintf(stdout, "OK token=%s user=%s %s=%s\n", "AA==", rfc_user, group_attribute_name, ag?ag:"");
#else
fprintf(stdout, "OK token=%s user=%s\n", "AA==", rfc_user);
#endif /* HAVE_KRB5_PAC_SUPPORT */
Expand Down
65 changes: 23 additions & 42 deletions src/auth/negotiate/kerberos/negotiate_kerberos_pac.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,28 +114,23 @@ get1byt(void)
return var;
}


static char *
pstrcpy( char *src, const char *dst)
pstrcat( char *dst, const char *src)
{
if (dst) {
if (strlen(dst)>MAX_PAC_GROUP_SIZE)
if (src) {
if ( strlen(dst) + strlen(src) + 1 >= MAX_PAC_GROUP_SIZE )
return nullptr;
else
return strcpy(src,dst);
return strcat(dst, src);
} else
return src;
return dst;
}

static char *
pstrcat( char *src, const char *dst)
{
if (dst) {
if (strlen(src)+strlen(dst)+1>MAX_PAC_GROUP_SIZE)
return nullptr;
else
return strcat(src,dst);
} else
return src;
append_comma(char *buffer){
if ( buffer && buffer[0] ) return pstrcat(buffer, ",");
return buffer;
}

int
Expand Down Expand Up @@ -236,17 +231,8 @@ getdomaingids(char *ad_groups, uint32_t DomainLogonId, char **Rids, uint32_t Gro
ag[1] = ag[1]+1;
memcpy((void *)&ag[2],(const void*)&p[bpos+2],6+nauth*4);
memcpy((void *)&ag[length],(const void*)Rids[l],4);
if (l==0) {
if (!pstrcpy(ad_groups,"group=")) {
debug((char *) "%s| %s: WARN: Too many groups ! size > %d : %s\n",
LogTime(), PROGRAM, MAX_PAC_GROUP_SIZE, ad_groups);
}
} else {
if (!pstrcat(ad_groups," group=")) {
debug((char *) "%s| %s: WARN: Too many groups ! size > %d : %s\n",
LogTime(), PROGRAM, MAX_PAC_GROUP_SIZE, ad_groups);
}
}

append_comma(ad_groups);

@rousskov rousskov Apr 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replacing group=g1 group=g2 with group=g1,g2 codifies comma as the delimiter for group values. AFAICT, Helper::Reply::parseResponseKeys() does not treat comma specially today. This raises many red flags, including:

  • parseResponseKeys() treats group=g1,g2 annotation as a single key=value pair, adding a single NotePairs::Entry. Adding two groups via one Entry violates the spirit of NotePairs API that uses one Entry per Squid-perceived annotation value.
  • Helper::Reply::parseResponseKeys() method treats "quoted" values specially. If we codify the special meaning of commas, then that treatment would need to be adjusted to support group=g1,"g2" and similar input.
  • Individual group names with embedded commas may be treated incorrectly. The helper must either quote them (to give parseResponseKeys() a chance to handle them correctly) or reject them.

Please note that we should not assume that all helpers or even all connection authentication helpers are going to do exactly what shipped-with-Squid helpers are doing. We should define an interface and supply compliant implementation in Squid. If we change that interface, like this PR may be doing, we must evaluate whether that change may break existing helpers/deployments and whether the benefits of our changes outweigh that breakage (if any). If they do not outweigh, we may need to version helper output format, so that Squid can reject no-longer-compatible helpers.

I am not sure whether authentication code uses stored group values. I suspect that it does not! Please point me to group-using authentication code if that suspicion is wrong.

I know that general code (e.g., note ACL) lets an admin determine whether values can be delimited and what that delimiter is. A comma is usually the default for delimited values, but it is not the default that matters here. I believe the changes in this PR will break deployments that use a non-comma delimiter for group values because Acl::NoteCheck::matchNotes() calls NotePairs::expandListEntries() and the latter does not treat commas specially. For example, I speculate that, after this PR changes , acl badGuys note -m: group bad will stop matching requests that belong to active and bad groups because the new helper response -- group=active,bad -- is not going to match while the old response -- group=active group=bad -- did match.

Similarly, I believe the changes in this PR will break deployments that do not use a delimiter for group values. For example, I speculate that acl badGuys note group bad will stop matching requests that belong to active and bad groups.

struct base64_encode_ctx ctx;
base64_encode_init(&ctx);
const uint32_t expectedSz = base64_encode_len(length+4) +1 /* terminator */;
Expand Down Expand Up @@ -281,6 +267,12 @@ getdomaingids(char *ad_groups, uint32_t DomainLogonId, char **Rids, uint32_t Gro
char *
getextrasids(char *ad_groups, uint32_t ExtraSids, uint32_t SidCount)
{
if (!ad_groups) {
debug((char *) "%s| %s: ERR: No space to store groups\n",
LogTime(), PROGRAM);
return nullptr;
}

if (ExtraSids!= 0) {
uint32_t ngroup;
uint32_t *pa;
Expand Down Expand Up @@ -323,19 +315,8 @@ getextrasids(char *ad_groups, uint32_t ExtraSids, uint32_t SidCount)
size_t length = 1+1+6+nauth*4;
ag = (char *)xcalloc((length)*sizeof(char),1);
memcpy((void *)ag,(const void*)&p[bpos],length);
if (!ad_groups) {
debug((char *) "%s| %s: ERR: No space to store groups\n",
LogTime(), PROGRAM);
xfree(pa);
xfree(ag);
return nullptr;
} else {
if (!pstrcat(ad_groups," group=")) {
debug((char *) "%s| %s: WARN: Too many groups ! size > %d : %s\n",
LogTime(), PROGRAM, MAX_PAC_GROUP_SIZE, ad_groups);
}
}

append_comma(ad_groups);
struct base64_encode_ctx ctx;
base64_encode_init(&ctx);
const uint32_t expectedSz = base64_encode_len(length) +1 /* terminator */;
Expand Down Expand Up @@ -465,11 +446,7 @@ get_resource_groups(char *ad_groups, uint32_t ResourceGroupDomainSid, uint32_t
uint32_t sauth;
memcpy((void *)&st[group_domain_sid_len], (const void*)&p[bpos], 4); // rid concatenation

if (!pstrcat(ad_groups, " group=")) {
debug((char *) "%s| %s: WARN: Too many groups ! size > %d : %s\n",
LogTime(), PROGRAM, MAX_PAC_GROUP_SIZE, ad_groups);
}

append_comma(ad_groups);
struct base64_encode_ctx ctx;
base64_encode_init(&ctx);
const uint32_t expectedSz = base64_encode_len(length) + 1 /* terminator */;
Expand Down Expand Up @@ -526,6 +503,8 @@ get_ad_groups(char *ad_groups, krb5_context context, krb5_pac pac)
return nullptr;
}

ad_groups[0] = '\0';

ad_data = (krb5_data *)xcalloc(1,sizeof(krb5_data));

#define KERB_LOGON_INFO 1
Expand Down Expand Up @@ -612,6 +591,8 @@ get_ad_groups(char *ad_groups, krb5_context context, krb5_pac pac)
if (checkustr(&LogonDomainName)<0)
goto k5clean;
ad_groups = getdomaingids(ad_groups,LogonDomainId,Rids,GroupCount);
if (!ad_groups)
goto k5clean;

// https://learn.microsoft.com/en-us/previous-versions/aa302203(v=msdn.10)?redirectedfrom=MSDN#top-level-pac-structure
if ((UserFlags&LOGON_EXTRA_SIDS) != 0) {
Expand Down
Loading