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.

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

AD: Ignore Foreign Security Principals in groups #7596

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

AD: Ignore Foreign Security Principals in groups #7596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3637db8
Nested groups: Handle Foreign Security Principals correctly (i.e. ign…
ondrejv2 Sep 11, 2024
10348f7
Merge branch 'SSSD:master' into references
ondrejv2 Sep 17, 2024
6afcc0b
Precision FSP detection update (optional)
ondrejv2 Sep 17, 2024
ad457a9
AD: Detect and ignore (for now) Foreign Security Principals
ondrejv2 Oct 28, 2024
21e3b85
Merge branch 'SSSD:master' into references
ondrejv2 Dec 10, 2024
d27446b
Merge branch 'SSSD:master' into references
ondrejv2 Dec 13, 2024
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
1 change: 1 addition & 0 deletions src/db/sysdb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
#define SYSDB_DOMAIN_ID_RANGE_CLASS "domainIDRange"
#define SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS "TrustedADDomainRange"
#define SYSDB_CERTMAP_CLASS "certificateMappingRule"
#define SYSDB_AD_FSP_CLASS "foreignSecurityPrincipal"

#define SYSDB_DN "dn"
#define SYSDB_NAME "name"
Expand Down
16 changes: 13 additions & 3 deletions src/lib/idmap/sss_idmap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ const char *idmap_error_string(enum idmap_error_code err)
}
}

bool is_domain_sid(const char *sid)
bool is_str_sid(const char *sid, int count)
{
const char *p;
long long a;
Expand All @@ -228,15 +228,25 @@ bool is_domain_sid(const char *sid)
return false;
}
c++;
} while(c < 3 && *endptr != '\0');
} while(c < count && *endptr != '\0');

if (c != 3 || *endptr != '\0') {
if (c != count || *endptr != '\0') {
return false;
}

return true;
}

bool is_principal_sid(const char *str)
{
return is_str_sid(str, 4);
}

bool is_domain_sid(const char *str)
{
return is_str_sid(str, 3);
}

enum idmap_error_code sss_idmap_init(idmap_alloc_func *alloc_func,
void *alloc_pvt,
idmap_free_func *free_func,
Expand Down
1 change: 1 addition & 0 deletions src/lib/idmap/sss_idmap.exports
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,6 @@ SSS_IDMAP_0.5 {

sss_idmap_ctx_set_extra_slice_init;
sss_idmap_add_auto_domain_ex;
is_principal_sid;

} SSS_IDMAP_0.4;
11 changes: 11 additions & 0 deletions src/lib/idmap/sss_idmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,17 @@ const char *idmap_error_string(enum idmap_error_code err);
*/
bool is_domain_sid(const char *str);

/**
* @brief Check if given string can be used as principal SID
*
* @param[in] str String to check
*
* @return
* - true: String can be used as principal SID
* - false: String can not be used as principal SID
*/
bool is_principal_sid(const char *str);

/**
* @brief Check if a domain is configured with algorithmic mapping
*
Expand Down
1 change: 1 addition & 0 deletions src/providers/ldap/sdap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ struct sdap_domain {
* the base DN which will not be based on the DNS domain of the LDAP
* server. naming_context might be NULL even after connection to an LDAP
* server. */
char *fspdn; /* Foreign security principal search base */
char *naming_context;

struct sdap_search_base **search_bases;
Expand Down
101 changes: 96 additions & 5 deletions src/providers/ldap/sdap_async_nested_groups.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
enum sdap_nested_group_dn_type {
SDAP_NESTED_GROUP_DN_USER,
SDAP_NESTED_GROUP_DN_GROUP,
SDAP_NESTED_GROUP_DN_FSP,
SDAP_NESTED_GROUP_DN_UNKNOWN
};

Expand Down Expand Up @@ -251,6 +252,33 @@ static errno_t
sdap_nested_group_hash_user(struct sdap_nested_group_ctx *group_ctx,
struct sysdb_attrs *user)
{
errno_t ret;
const char *val = NULL;
const char **val_list = NULL;

ret = sysdb_attrs_get_string_array(user, SYSDB_OBJECTCLASS, group_ctx, &val_list);
if (ret == EOK) {
/* if called from sdap_nested_group_single_step_process(), then we have objectclass
* and uid attrs so we can test for Foreign Security principals */
if (string_in_list(SYSDB_AD_FSP_CLASS, discard_const(val_list), false)) {
/* TODO: handle Foreign Security Principal here
* since we don't know how to do it now, then we skip them for now */
sysdb_attrs_get_string(user, SYSDB_ORIG_DN, &val);
DEBUG(SSSDBG_TRACE_ALL, "Ignoring Foreign Principal %s\n",val);
talloc_free(val_list);
return EOK;
}
talloc_free(val_list);

ret = sysdb_attrs_get_string(user, group_ctx->opts->user_map[SDAP_AT_USER_NAME].name, &val);
if (ret != EOK) {
DEBUG(SSSDBG_TRACE_ALL, "Unable to get username for %s\n",val);
return ret;
}
/* we need to populate the sys_name in the user map so the user is recognized later on */
sysdb_attrs_add_string(user, group_ctx->opts->user_map[SDAP_AT_USER_NAME].sys_name, val);
}

return sdap_nested_group_hash_entry(group_ctx->users, user, "users");
}

Expand Down Expand Up @@ -526,6 +554,57 @@ sdap_nested_member_is_group(struct sdap_nested_group_ctx *group_ctx,
return sdap_nested_member_is_ent(group_ctx, dn, filter, false);
}

static bool
sdap_nested_member_is_fsp(struct sdap_nested_group_ctx *group_ctx,
const char *dn)
{
char *fspdn;
size_t fspdn_len, dn_len;
int reti, len_diff;
bool ret = false;

if (group_ctx->opts->sdom->fspdn == NULL) {
char *basedn;

reti = domain_to_basedn(group_ctx, group_ctx->domain->realm, &basedn);
if (reti != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "Unable to obtain basedn\n");
return false;
}

fspdn = talloc_asprintf(group_ctx->opts->sdom, "%s,%s",
"CN=ForeignSecurityPrincipals", basedn);
if (fspdn == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "Unable to run talloc_asprintf\n");
return false;
}
talloc_free(basedn);
group_ctx->opts->sdom->fspdn = fspdn;
} else {
fspdn = group_ctx->opts->sdom->fspdn;
}
sumit-bose marked this conversation as resolved.
Show resolved Hide resolved

fspdn_len = strlen(fspdn);
dn_len = strlen(dn);
len_diff = dn_len - fspdn_len;
if (len_diff < sizeof("CN=S-")) {
return false;
}
ret = (strncasecmp(&dn[len_diff], fspdn, fspdn_len) == 0);

if (ret) { /* looks like FSP, so just double check to be 100% sure */
char *fsp_str = talloc_strdup(group_ctx, dn);

if (fsp_str == NULL)
return false;
fsp_str[len_diff - 1] = '\0'; /* replace comma with NULL */
ret = is_principal_sid(&fsp_str[3]);
talloc_free(fsp_str);
}

return ret;
}

static errno_t
sdap_nested_group_split_members(TALLOC_CTX *mem_ctx,
struct sdap_nested_group_ctx *group_ctx,
Expand All @@ -548,6 +627,7 @@ sdap_nested_group_split_members(TALLOC_CTX *mem_ctx,
bool bret;
bool is_user;
bool is_group;
bool is_fsp;
errno_t ret;
int i;

Expand Down Expand Up @@ -625,7 +705,12 @@ sdap_nested_group_split_members(TALLOC_CTX *mem_ctx,
is_group = sdap_nested_member_is_group(group_ctx, dn,
&group_filter);

if (is_user && is_group) {
is_fsp = sdap_nested_member_is_fsp(group_ctx, dn);

if (is_fsp) {
DEBUG(SSSDBG_TRACE_ALL, "[%s] is Foreign Security principal\n", dn);
type = SDAP_NESTED_GROUP_DN_FSP;
} else if (is_user && is_group) {
/* search bases overlap */
DEBUG(SSSDBG_TRACE_ALL, "[%s] is unknown object\n", dn);
type = SDAP_NESTED_GROUP_DN_UNKNOWN;
Expand Down Expand Up @@ -1500,6 +1585,10 @@ static errno_t sdap_nested_group_single_step(struct tevent_req *req)
state->group_ctx,
state->current_member);
break;
case SDAP_NESTED_GROUP_DN_FSP: /* TODO: Process FSPs */
DEBUG(SSSDBG_TRACE_ALL, "Ignoring Foreign Security Principal [%s]\n",
state->current_member->dn);
return EOK;
}

if (subreq == NULL) {
Expand Down Expand Up @@ -1625,6 +1714,9 @@ sdap_nested_group_single_step_process(struct tevent_req *subreq)
state->nested_groups[state->num_groups] = entry;
state->num_groups++;

break;
case SDAP_NESTED_GROUP_DN_FSP: /* TODO: Handle FSPs */
DEBUG(SSSDBG_TRACE_ALL, "BUG!!! We should never get here\n");
break;
case SDAP_NESTED_GROUP_DN_UNKNOWN:
if (state->ignore_unreadable_references) {
Expand Down Expand Up @@ -1832,8 +1924,8 @@ sdap_nested_group_lookup_user_send(TALLOC_CTX *mem_ctx,
attrs[2] = NULL;

/* create filter */
base_filter = talloc_asprintf(state, "(objectclass=%s)",
group_ctx->opts->user_map[SDAP_OC_USER].name);
base_filter = talloc_asprintf(state, "(|(objectclass=%s)(objectclass=%s))",
group_ctx->opts->user_map[SDAP_OC_USER].name,SYSDB_AD_FSP_CLASS);
if (base_filter == NULL) {
ret = ENOMEM;
goto immediately;
Expand All @@ -1849,8 +1941,7 @@ sdap_nested_group_lookup_user_send(TALLOC_CTX *mem_ctx,
/* search */
subreq = sdap_get_generic_send(state, ev, group_ctx->opts, group_ctx->sh,
member->dn, LDAP_SCOPE_BASE, filter, attrs,
group_ctx->opts->user_map,
group_ctx->opts->user_map_cnt,
NULL, 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

why is it needed to remove the map here?

I think this approach is good as well, but you are doing too many shortcuts with this patch.

Imo it would be better to handle the FSPs in sdap_nested_group_lookup_unknown_send()/recv() by adding a dedicated sdap_nested_group_lookup_fsp_send()/recv() request to handle FSPs in the same way as plain user and group members.

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
We can't use maps for ldapsearch with multiple object classes in the filter due to the design limitation. I wanted to use single ldap search when detecting FSPs/users. To me it's unnecessary to introduce another sdap_nested_group_lookup_fsp_send()/recv() as it would mean another pointless ldapsearch.
Since we only require member uid at this stage, there is no real need for maps and we can squeeze both searches into single one.
Ondrej

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

I see your point. But following this thought would mean to refactor sdap_nested_group_lookup_unknown_send()/recv() to only send a single ldapsearch which would allow all objectclasses (or at least user, group and fsp) and determine the type of the results based on the objectclass. What do you think?

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Well, yes, that would be an ideal case (as the current code is bit "cumbersome") yes.
The problem is, that in order to search for all objectclasses we need to use unmapped search (I've mentioned this in a different PR).
Using unmapped search means sdap_parse_entry() will not map entries for us so we gotta do it manually. Now I did it for the user objectclass as it's not a big deal (single attr uid), but not sure what we need for group objectclasses.

Ondrej

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

you can just request all attributes and run sdap_parse_entry() after the type is known based on the objectclass. There is an example how it can be done in sdap_asq_search_parse_entry().

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, but sdap_parse_entry() needs sdap_msg parameter, where do I get it from group_ctx?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

instead of using sdap_get_generic_send() you can use sdap_get_generic_ext_send() and add your own parser as a callback. This callback will receive sdap_msg.

HTH

bye,
Sumit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Sorry, that's bit too complex for me. Can you suggest a suitable code? I can't figure out myself.
Ideally I would like to parse attributes in sdap_nested_group_lookup_unknown_recv() if that's possible.
Namely I need better replacement of this snippet:

       ret = sysdb_attrs_get_string(*_entry, state->group_ctx->opts->user_map[SDAP_AT_USER_NAME].name, &val);
       if (ret != EOK) {
          DEBUG(SSSDBG_TRACE_ALL, "Unable to get username for %s\n",val);
          goto done;
       }
       /* we need to populate the sys_name in the user map so the user is recognized later on */
       sysdb_attrs_add_string(*_entry, state->group_ctx->opts->user_map[SDAP_AT_USER_NAME].sys_name, val);

Once I have the parsing done, then I could possibly do the rest myself.
Thanks.
Ondrej

dp_opt_get_int(group_ctx->opts->basic,
SDAP_SEARCH_TIMEOUT),
false);
Expand Down
Loading