Read and write list ordering

Jeremy Allison jra at samba.org
Sat Jul 16 22:42:54 UTC 2016


On Sat, Jul 16, 2016 at 09:55:33PM +0300, Uri Simchoni wrote:
> On 07/16/2016 02:46 AM, Justin Maggard wrote:
> > I'd like to get other people's opinions on this.
> > 
> > Currently, objects in the write list always override the same objects
> > in the read list.
> > 
> > This includes cases where you have an individual user account in the
> > read list, and a group he belongs to in the write list.  This seems
> > unintuitive, as most of us are used to user account settings taking
> > precedence over group account settings.
> > 
> > The attached patch keeps track of the object type, and gives
> > preference to user account objects if they exist.
> > 
> > Thoughts?
> > 
> > Thanks,
> > -Justin
> > 
> If I were to design an authorization system from scratch, I would agree
> with you. If the admin has gone through the trouble of specifying what
> this user's access is, then the SMB server should look no further.
> However, the patch would break compatibility with existing setups, so
> such a change must be brought in a way that doesn't change the behavior
> of any existing setup.

Can we make such a change in a new major release ? i.e. would
such a patch be acceptable in a 4.5.0 release - so long as it
is fully documented in the release notes ?

Note that this change will always add restrictions to access,
never open it up.

IMHO changes that restrict access are much less invasive to existing
setups than changes that losen access.

Personally I'd allow this one with documentation update, as I
really doubt anyone is depending on this.

But as always, opinions may differ :-).

> Please note that this also breaks compatibility with Windows because in
> Windows, everything is a SID, there's no special weight to user-specific
> rules.
> 
> One idea of how to do this is a via a new smb.conf share parameter which
> is a list of user:perm tuples. A perm can be "rw/ro/deny" or something
> like that. This list can be processed before the other lists, and
> whatever "verdict" found there is final (also stops on first match).
> 
> If people object to adding this (e.g. on account of "too many competing
> features"), I also think it's possible to keep this as a private patch
> that only touches upon share_access.c, and hence has little maintenance
> cost, see attached.

I do think that's too many competing features. I'd rather just clarify
the existing one to behave as Justin mentioned.

Having said that your patch looks much less invasive (though I
haven't reviewed it yet).

Cheers,

	Jeremy.


> From dedd4bd56fdf1dafc071646dfc46dfbd7d4193b3 Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <uri at samba.org>
> Date: Sat, 16 Jul 2016 21:48:37 +0300
> Subject: [PATCH] smbd: modify share access rule precedence
> 
> When evaluating share access rules, evaluate user-spcific
> rule first and consider them as "terminal" if they apply.
> ---
>  source3/smbd/share_access.c | 126 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 95 insertions(+), 31 deletions(-)
> 
> diff --git a/source3/smbd/share_access.c b/source3/smbd/share_access.c
> index fa56063..2a5367e 100644
> --- a/source3/smbd/share_access.c
> +++ b/source3/smbd/share_access.c
> @@ -67,6 +67,17 @@ static bool do_group_checks(const char **name, const char **pattern)
>  	return False;
>  }
>  
> +static bool is_principal_group(const char *name)
> +{
> +	if (name[0] == '@' || (name[0] == '+' && name[1] == '&') ||
> +	    name[0] == '+' || (name[0] == '&' && name[1] == '+') ||
> +	    name[0] == '&') {
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool token_contains_name(TALLOC_CTX *mem_ctx,
>  				const char *username,
>  				const char *domain,
> @@ -155,31 +166,51 @@ static bool token_contains_name(TALLOC_CTX *mem_ctx,
>   *
>   * The other use is the netgroup check when using @group or &group.
>   */
> -
> -bool token_contains_name_in_list(const char *username,
> -				 const char *domain,
> -				 const char *sharename,
> -				 const struct security_token *token,
> -				 const char **list)
> +#define PRINCIPAL_TYPE_USER 1
> +#define PRINCIPAL_TYPE_GROUP 2
> +#define PRINCIPAL_TYPE_ALL (PRINCIPAL_TYPE_USER | PRINCIPAL_TYPE_GROUP)
> +
> +static bool token_contains_name_in_list_internal(
> +    const char *username, const char *domain, const char *sharename,
> +    const struct security_token *token, const char **list,
> +    unsigned principals_mask)
>  {
>  	if (list == NULL) {
>  		return False;
>  	}
> -	while (*list != NULL) {
> -		TALLOC_CTX *frame = talloc_stackframe();
> +	for (;*list != NULL; ++list) {
> +		TALLOC_CTX *frame;
>  		bool ret;
> +		bool is_group = is_principal_group(*list);
>  
> +		if (is_group && !(principals_mask & PRINCIPAL_TYPE_GROUP)) {
> +			continue;
> +		}
> +
> +		if (!is_group && !(principals_mask & PRINCIPAL_TYPE_USER)) {
> +			continue;
> +		}
> +
> +		frame = talloc_stackframe();
>  		ret = token_contains_name(frame, username, domain, sharename,
>  					  token, *list);
>  		TALLOC_FREE(frame);
>  		if (ret) {
>  			return true;
>  		}
> -		list += 1;
>  	}
>  	return False;
>  }
>  
> +bool token_contains_name_in_list(const char *username, const char *domain,
> +				 const char *sharename,
> +				 const struct security_token *token,
> +				 const char **list)
> +{
> +	return token_contains_name_in_list_internal(
> +	    username, domain, sharename, token, list, PRINCIPAL_TYPE_ALL);
> +}
> +
>  /*
>   * Check whether the user described by "token" has access to share snum.
>   *
> @@ -197,23 +228,38 @@ bool user_ok_token(const char *username, const char *domain,
>  		   const struct security_token *token, int snum)
>  {
>  	if (lp_invalid_users(snum) != NULL) {
> -		if (token_contains_name_in_list(username, domain,
> -						lp_servicename(talloc_tos(), snum),
> -						token,
> -						lp_invalid_users(snum))) {
> +		if (token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_invalid_users(snum), PRINCIPAL_TYPE_USER)) {
>  			DEBUG(10, ("User %s in 'invalid users'\n", username));
> -			return False;
> +			return false;
>  		}
>  	}
>  
>  	if (lp_valid_users(snum) != NULL) {
> -		if (!token_contains_name_in_list(username, domain,
> -						 lp_servicename(talloc_tos(), snum),
> -						 token,
> -						 lp_valid_users(snum))) {
> -			DEBUG(10, ("User %s not in 'valid users'\n",
> -				   username));
> -			return False;
> +		if (token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_valid_users(snum), PRINCIPAL_TYPE_USER)) {
> +			DEBUG(10, ("User %s in 'valid users'\n", username));
> +			return true;
> +		}
> +	}
> +
> +	if (lp_invalid_users(snum) != NULL) {
> +		if (token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_invalid_users(snum), PRINCIPAL_TYPE_GROUP)) {
> +			DEBUG(10, ("User %s in 'invalid users'\n", username));
> +			return false;
> +		}
> +	}
> +
> +	if (lp_valid_users(snum) != NULL) {
> +		if (!token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_valid_users(snum), PRINCIPAL_TYPE_GROUP)) {
> +			DEBUG(10, ("User %s not in 'valid users'\n", username));
> +			return false;
>  		}
>  	}
>  
> @@ -245,24 +291,42 @@ bool is_share_read_only_for_token(const char *username,
>  	int snum = SNUM(conn);
>  	bool result = conn->read_only;
>  
> +	if (lp_write_list(snum) != NULL) {
> +		if (token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_write_list(snum), PRINCIPAL_TYPE_USER)) {
> +			result = false;
> +			goto done;
> +		}
> +	}
> +
>  	if (lp_read_list(snum) != NULL) {
> -		if (token_contains_name_in_list(username, domain,
> -						lp_servicename(talloc_tos(), snum),
> -						token,
> -						lp_read_list(snum))) {
> -			result = True;
> +		if (token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_read_list(snum), PRINCIPAL_TYPE_USER)) {
> +			result = true;
> +			goto done;
>  		}
>  	}
>  
>  	if (lp_write_list(snum) != NULL) {
> -		if (token_contains_name_in_list(username, domain,
> -						lp_servicename(talloc_tos(), snum),
> -						token,
> -						lp_write_list(snum))) {
> -			result = False;
> +		if (token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_write_list(snum), PRINCIPAL_TYPE_GROUP)) {
> +			result = false;
> +			goto done;
> +		}
> +	}
> +
> +	if (lp_read_list(snum) != NULL) {
> +		if (token_contains_name_in_list_internal(
> +			username, domain, lp_servicename(talloc_tos(), snum),
> +			token, lp_read_list(snum), PRINCIPAL_TYPE_GROUP)) {
> +			result = true;
>  		}
>  	}
>  
> +done:
>  	DEBUG(10,("is_share_read_only_for_user: share %s is %s for unix user "
>  		  "%s\n", lp_servicename(talloc_tos(), snum),
>  		  result ? "read-only" : "read-write", username));
> -- 
> 2.5.5
> 




More information about the samba-technical mailing list