Skip to content

krb5_child: fix enterprise principal parsing in keep-alive sessions #8351

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

Merged
alexey-tikhonov merged 1 commit into from
Jan 24, 2026
Merged

krb5_child: fix enterprise principal parsing in keep-alive sessions #8351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 42 additions & 9 deletions src/providers/krb5/krb5_child.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ static errno_t k5c_attach_passkey_msg(struct krb5_req *kr, struct sss_passkey_ch
static errno_t k5c_attach_keep_alive_msg(struct krb5_req *kr);
static errno_t k5c_recv_data(struct krb5_req *kr, int fd, uint32_t *offline);
static errno_t k5c_send_data(struct krb5_req *kr, int fd, errno_t error);
static int k5c_setup(struct krb5_req *kr, uint32_t offline);

static krb5_error_code set_lifetime_options(struct cli_opts *cli_opts,
krb5_get_init_creds_opt *options)
Expand Down Expand Up @@ -877,6 +878,12 @@ static errno_t krb5_req_update(struct krb5_req *dest, struct krb5_req *src)
talloc_free(dest->pd);
dest->pd = talloc_steal(dest, src->pd);

/* Update settings that may change between commands */
dest->use_enterprise_princ = src->use_enterprise_princ;
dest->validate = src->validate;
dest->posix_domain = src->posix_domain;
dest->send_pac = src->send_pac;
Comment on lines +881 to +885
Copy link

@gemini-code-assist gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

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

critical

The update of use_enterprise_princ, validate, posix_domain, and send_pac in krb5_req_update is critical for ensuring that subsequent Kerberos operations in a keep-alive session use the correct configuration. Without this, the principal parsing logic in k5c_setup might operate with stale flags, leading to incorrect behavior, especially in dynamic AD environments.


return EOK;
}

Expand Down Expand Up @@ -944,6 +951,13 @@ static krb5_error_code k5c_send_and_recv(struct krb5_req *kr)
goto done;
}

ret = k5c_setup(kr, offline);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "k5c_setup failed during keep-alive [%d]: %s\n",
ret, sss_strerror(ret));
goto done;
Comment on lines +954 to +958
Copy link

@gemini-code-assist gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

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

high

Calling k5c_setup after krb5_req_update is essential. This ensures that any updated settings received from the backend (like use_enterprise_princ) are immediately applied to the krb5_req structure before further processing, which is crucial for correct principal parsing and credential handling.

}

done:
talloc_free(tmpkr);
return ret;
Expand Down Expand Up @@ -3924,6 +3938,7 @@ static void k5c_ccache_check(struct krb5_req *kr, uint32_t offline)

static int k5c_setup(struct krb5_req *kr, uint32_t offline)
{
krb5_principal princ;
krb5_error_code kerr;
int parse_flags;

Expand Down Expand Up @@ -3951,28 +3966,46 @@ static int k5c_setup(struct krb5_req *kr, uint32_t offline)
}

parse_flags = kr->use_enterprise_princ ? KRB5_PRINCIPAL_PARSE_ENTERPRISE : 0;
kerr = sss_krb5_parse_name_flags(kr->ctx, kr->upn, parse_flags, &kr->princ);
kerr = sss_krb5_parse_name_flags(kr->ctx, kr->upn, parse_flags, &princ);
if (kerr != 0) {
KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
return kerr;
Comment on lines +3969 to 3972
Copy link

@gemini-code-assist gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

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

critical

The introduction of a temporary princ and the subsequent comparison with kr->princ before updating is a robust way to handle changes in the principal. This prevents unnecessary re-initialization if the principal remains the same and correctly updates it when needed, which is vital for the PR's objective.

}
if (kr->princ == NULL || !krb5_principal_compare(kr->ctx, kr->princ, princ)) {
DEBUG(SSSDBG_TRACE_FUNC, "Updating principal\n");
if (kr->princ != NULL) {
krb5_free_principal(kr->ctx, kr->princ);
}
kr->princ = princ;
} else {
DEBUG(SSSDBG_TRACE_FUNC, "Principal unchanged, keeping existing\n");
krb5_free_principal(kr->ctx, princ);
}
Comment on lines +3974 to +3983
Copy link

@gemini-code-assist gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

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

high

The change to free kr->princ only when it's different from the newly parsed princ (or if kr->princ was initially NULL) is a good memory management practice. It avoids potential memory leaks if the principal changes during a keep-alive session.


kerr = krb5_parse_name(kr->ctx, kr->upn, &kr->princ_orig);
if (kerr != 0) {
KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
return kerr;
if (kr->princ_orig == NULL) {
kerr = krb5_parse_name(kr->ctx, kr->upn, &kr->princ_orig);
if (kerr != 0) {
KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
return kerr;
}
}
Comment on lines +3985 to 3991
Copy link

@gemini-code-assist gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

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

high

Moving the krb5_parse_name call for kr->princ_orig inside an if (kr->princ_orig == NULL) block improves efficiency by preventing redundant parsing of the original principal if it has already been set.


sss_krb5_free_unparsed_name(kr->ctx, kr->name);
kr->name = NULL;
Comment on lines +3993 to +3994
Copy link

@gemini-code-assist gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

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

high

The explicit freeing of kr->name before unparsing a new one is good practice to prevent memory leaks and ensure kr->name always reflects the current kr->princ.

kerr = krb5_unparse_name(kr->ctx, kr->princ, &kr->name);
if (kerr != 0) {
KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
return kerr;
}

kr->creds = calloc(1, sizeof(krb5_creds));
if (kr->creds == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "calloc failed.\n");
return ENOMEM;
if (kr->creds != NULL) {
krb5_free_cred_contents(kr->ctx, kr->creds);
} else {
kr->creds = calloc(1, sizeof(krb5_creds));
if (kr->creds == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "calloc failed.\n");
return ENOMEM;
}
}
Comment on lines +4001 to 4009
Copy link

@gemini-code-assist gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

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

high

The conditional freeing of kr->creds contents before potential reallocation ensures that the creds structure is always in a clean state, preventing memory leaks or data corruption if it was previously used.


#ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_RESPONDER
Expand Down