[U-Boot] [PATCH 10/12] env: acl: Add environment variable access control list

[Joe Hershberger]

Hi Wolfgang,

I guess you didn't see this the last time I sent it to you off list...

On Thu, Sep 13, 2012 at 3:13 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Joe Hershberger,
>
> In message <1345236586-19076-11-git-send-email-joe.hershberger at ni.com> you wrote:
>> Currently just validates variable types as decimal, hexidecimal,
>> boolean, ip address, and mac address.  Call
>> env_acl_validate_setenv_params() from setenv() in both cmd_nvedit.c
>> and fw_env.c.
>>
>> If the entry is not found in the env ACL, then look in the static one.
>> This allows the env to override the static definitions, but prevents
>> the need to have every definition in the environment distracting you.
>
> I have to admid that I haven't tested your code at all, but here are
> a few comments anyway:
>
> - It appears you will store all access related information in the ACL
>   variable (or a copy of it), i. e. in linearized form.  Are you not
>   concerned that this will make variable access slow?

At least in my uses so far, there are not so many variables that it is
noticeably slower.

> - Encoding type information in a variable named "acl" doesn;t look
>   really elegant to me.  Of course, chosing another name is a trivial
>   thing to do.

For sure.  I accept that this is only one of the things it does and
it's not really access control in the typical sense (with users and
what not).

> - There have been discussions that it would be nice to have a
>   "volatile" variable type, for example for variables like "filesize"
>   or "loadaddr" - such variables would not be stored to the persistent
>   storage by the "saveenv" command.  Thi sis something that should be
>   fairly wasy to add to your code.

I was one of the proponents of this when we last discussed this some
years ago.  I left it out because you convinced me I should just use
other variables and overwrite the special ones in a script when I need
to.  I still think supporting a volatile variable would be cleaner
than that.  I'm glad you changed your mind.

> - It would be nice if we could group variables; for example, assume we
>   could define a group "network" which includes things like ethaddr,
>   ipaddr, netmask, hostname etc. - and we could then do something like
>   "env print -g network" to print all variables of such a group.

I agree that would be nice.

> - We have the situation that certain variables need specific handling
>   when they get assigned to - baudrate, ethaddr etc. come to mind.
>   But the code to implement this is scattered all over the place, nd
>   doesn't really scale.

I agree wholeheartedly.

> I've been thinking about similar features for some time, but the
> implementation I had in mind was way different.  This is what I had in
> mind:
>
> - Like you, I wanted to use plain environment variables for
>   persistent storage of all such information. The format you chose
>   looks fine for me, too.
>
> - I would like to flag these variables as special by using special
>   names for them. All such special vailables would start with a dot,
>   i. e. instead of "acl" I had planned to call this ".flags".

OK...  I like the prepended '.'.  I'm not sure that "flags" is a very
good name for something so specific from a user point of view, but I
see why you chose it based on how you plan to store it in the hash.

>   Normally "env print" etc. would not show such specal variables, but
>   "env print -a" would.

That makes sense.

>   Similarly, grous of variables could be defined in a ".groups"
>   variable, etc.

OK.

> - Instead of parsing the ".flags" variable again and again when
>   accessing variables, my idea was to extend the hash table: so far,
>   the struct entry (see "include/search.h") holds only pointers to the
>   key (variable name) and data (variable value).  I suggest to
>   extend this struct:

This seems like a good idea for the most part.  Are you concerned that
the hast table will take twice the space?  Should this be optional?

>   * By adding a field "flags" (u32) we could easily encode access
>     permission type information (like read-only, write-once,
>     volatile, ...), and of course also type information (hex, dec,
>     string, IP addr, MAC addr, ...).
>
>   * By adding a field "callbacks" (int foo(const char *)) we could
>     register pointers to a function which gets called whenever the
>     respective variable value gets changed.
>
>     We would still need a compile time mapping from strings to
>     pointers, but at least this would allow to generalize and unify
>     the existing code. For example, setting up a mapping of string
>     "ethaddr" to function env_set_ethaddr() would allow to create
>     something like a special variable
>
>     .callbacks=ethaddr:ethaddr,eth1addr,ath2addr;baudrate:baudrate;...

One of the things I've run into with baudrate and silent specifically
is wanting to control if the special callback is made on set and/or on
env relocation, and/or on initial load, and/or on import.  I think
there should be a field in the flags for when to check the callback.

>     This way we could even enale user specific callbacks without need
>     to modify any common code.

Would the user be able to define other callbacks somehow?

>     And any "env set" would just have to check if the callback
>     function pointer is non-NULL ...
>
>
> What do you think?

I think is sounds very nice.  And it's not too far from what I've
done.  I'll look into implementing some of it.

Cheers,
-Joe