flimflam: add support for multiple profiles

flimflam: add support for multiple profiles

Add a stack of profiles and support for per-user profiles (stored in the
cryptohome).  This also adds support for operation without a profile so 
no persistent state is recorded (meant mostly for testing).

Changes include:
o add PushProfile, PopProfile, and PopAnyProfile D-bus directives to
  the Manager interface
o on object load (Service, Device, etc.) walk the stack of profiles
  until an entry is found for the object (replace direct storage
  load/store of objects by calls to the profile code)
o object save always write to the active profile which is the top of
  stack; if there is no active profile do nothing
o have Service objects track the active profile where state was last saved;
  this is used when pop'ing a profile to invalidate in-memory state for
  the service (e.g. security credentials)
o include Services in a Profile's properties only if it is the active
  profile
o more correctly implement the notion that OfflineMode is accessible only
  in the Default profile (e.g. don't return it in a Profile's properties,
  disallow setting the property directly)
o drop setting the ActiveProfile property; it does not make sense with the
  new model (use Push/Pop instead)
o drop reading in all global profiles are startup; the old connman model
  does not make a lot of sense for us
o change the storage layer to better support multiple profiles:
  - add a connman_storage_ident structure for file names which may include
    a user name; per-user storage is placed in a subdirectory of the user's
    home directory so creating files will fail unless the directory exists
  - revamp storage <object>_load/save methods so the storage_open/close
    work is done in the storage layer; this consolidates this logic in the
    storage code and better assures consistent error returns to the profile
    code so it can decide when to stop walking the profile stack on load
  - stop passing the profile name to the crypto routines; this was not
    being used; if we want to enable/disable crypto based on the profile
    this looks to be better done one layer up

BUG=chromium-os:12948
TEST=manual:using test scripts; automated tests TBA

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=99c34cc

[Sam Leffler, 2011-03-09 20:44:29]

There are a couple of bug fixes I need to pull out into separate CL's.  Can also
pull out the crypto api changes separately.

Currently using these with mods to login.conf/logout.conf to push/pop the
per-user profile.  Those changes will go up separately.  These changes should
stand on their own and not perturb the status quo.

There are still two pending issues to resolve before giving everyone push/pop:
the ui's "forget" mechanism only looks at the ActiveProfile so the full set of
entries isn't presented, and if flimflamd crashes nothing pushes the user
profile so system state is out of date (not sure how cleanly handle that).

I also need to fixup the hidden ssid code to bounce through the profile code so
it can walk the stack.

We could improve the storage layer by abstracting the keyfile stuff but will
leave that to another time (e.g. in shill).

[Sam Leffler, 2011-03-09 20:48:18]

Add rginda for crypto mods and cmasone for general security review.

[rginda, 2011-03-09 21:12:01]

On 2011/03/09 20:48:18, Sam Leffler wrote:
> Add rginda for crypto mods and cmasone for general security review.

I forgot to mention last time we spoke that the profile name was intended to
allow an alternate crypto plugin to take over for non-default profiles.  So
crypto_descbc might opt out an let rot47 take over, or crypt_descbc might use
something other than the owner key for non-default profiles.  If you decided one
level higher, I imagine it would make crypto a on/off decision instead.

That being said, the profile name isn't actually do anything right now, and if
the functionality ends up being required later I'm sure it could be easily
re-added.

So, crypto related changes LGTM.

[Nathan Williams, 2011-03-09 21:28:10]

A few little things, but it mostly looks good. My only concern is that I don't
see how we would permit the UI to set something globally - "Allow other users to
use this network". Do you have thoughts on how that might be addressed?

http://codereview.chromium.org/6659006/diff/1/doc/profile-api.txt
File doc/profile-api.txt (right):

http://codereview.chromium.org/6659006/diff/1/doc/profile-api.txt#newcode40
doc/profile-api.txt:40: Timesamp entry was last written.
Typo.

http://codereview.chromium.org/6659006/diff/1/src/profile.c
File src/profile.c (right):

http://codereview.chromium.org/6659006/diff/1/src/profile.c#newcode280
src/profile.c:280: * Handle changes to the profile.  Generate PropertChanged
signals
Typo.

http://codereview.chromium.org/6659006/diff/1/src/profile.c#newcode295
src/profile.c:295: services_changed(NULL);
Now that services_changed() takes a profile argument, shouldn't this be
services_changed(active_profile())?

http://codereview.chromium.org/6659006/diff/1/src/storage.c
File src/storage.c (right):

http://codereview.chromium.org/6659006/diff/1/src/storage.c#newcode96
src/storage.c:96: #include <stdio.h>
What's this doing all the way down here?

[Sam Leffler, 2011-03-09 22:31:10]

You also reminded me I needed to pull STORAGE_HOMEDIR out into configure which I
did.

http://codereview.chromium.org/6659006/diff/1/doc/profile-api.txt
File doc/profile-api.txt (right):

http://codereview.chromium.org/6659006/diff/1/doc/profile-api.txt#newcode40
doc/profile-api.txt:40: Timesamp entry was last written.
Done

http://codereview.chromium.org/6659006/diff/1/src/profile.c
File src/profile.c (right):

http://codereview.chromium.org/6659006/diff/1/src/profile.c#newcode280
src/profile.c:280: * Handle changes to the profile.  Generate PropertChanged
signals
Done

http://codereview.chromium.org/6659006/diff/1/src/profile.c#newcode295
src/profile.c:295: services_changed(NULL);
Yup, updated.

http://codereview.chromium.org/6659006/diff/1/src/storage.c
File src/storage.c (right):

http://codereview.chromium.org/6659006/diff/1/src/storage.c#newcode96
src/storage.c:96: #include <stdio.h>
Moved to the top.  It was added for snprintf use in __getpath which I waffled
about removing (since it's just pretty-printing data for debug msgs).

[Nathan Williams, 2011-03-10 20:54:47]

The code LGTM. I still have a couple of questions about the model.

1. This doesn't have a way to write to the non-top profile, which in our
profile-per-user model, prevents sharing auth info among users. This is a
user-visible change in behavior from what we did previously, and affects, for
example, the "house laptop" model. Is there agreement that this is OK? Is there
a reasonable way to add writing to layers other than the top if we decide we
need to?

2. How does this behave in the presence of the existing authentication
information in the default profile? It seems like the global information will
remain global until a user modifies it, at which point they will have a private
copy of the information. Other users will still see the previously-existing
information, and there's no clear way to get rid of it.

[Sam Leffler, 2011-03-10 21:36:58]

On 2011/03/10 20:54:47, Nathan Williams wrote:
> The code LGTM. I still have a couple of questions about the model.
> 
> 1. This doesn't have a way to write to the non-top profile, which in our
> profile-per-user model, prevents sharing auth info among users. This is a
> user-visible change in behavior from what we did previously, and affects, for
> example, the "house laptop" model. Is there agreement that this is OK? Is
there
> a reasonable way to add writing to layers other than the top if we decide we
> need to?

There is no support currently for writing to anything but the top of stack.  If
you want to add something shared you have to configure it pre-login.  I
considered tagging services with the associated profile instead of writing to
TOS but that got too complicated.

Note certain properties must also be controlled where they are stored. 
OfflineMode is forced to the default profile because having it per-user doesn't
make sense.  ProxyConfig is an example of something that cannot go in the
default/global profile (but isn't enforced yet).

> 
> 2. How does this behave in the presence of the existing authentication
> information in the default profile? It seems like the global information will
> remain global until a user modifies it, at which point they will have a
private
> copy of the information. Other users will still see the previously-existing
> information, and there's no clear way to get rid of it.

What you describe about COW/inheritance is correct.  To clear out old entries
we'd need the Settings panel available pre-login.  I think it's reasonable to do
a one-time flush of the global/default profile for this.  Long-term the UI may
want to expose the notion of private vs shared settings.  For that we'd need a
mechanism to tag objects w/ a profile (already done internally but would require
dbus support).

-Sam

[Sam Leffler, 2011-03-11 22:10:18]

PTAL

While testing I found we crash on shutdown which made me look more carefully at
the cleanup process.  The crash was because we tried to free the default profile
string which is statically allocated.  But I also noticed we need to clear the
pending change timer which made me notice a potential race with passing a
reference to a profile as an arg.  To deal with these I changed the handling of
services_changed to take the current active profile when the timer fires.

[Sam Leffler, 2011-03-12 17:28:47]

It appears this work is not ready.  If you've got any pending comments
feel free to push, otherwise I'll be back in a bit.

-Sam

On Fri, Mar 11, 2011 at 2:10 PM,  <sleffler@chromium.org> wrote:
> PTAL
>
> While testing I found we crash on shutdown which made me look more carefully
> at
> the cleanup process.  The crash was because we tried to free the default
> profile
> string which is statically allocated.  But I also noticed we need to clear
> the
> pending change timer which made me notice a potential race with passing a
> reference to a profile as an arg.  To deal with these I changed the handling
> of
> services_changed to take the current active profile when the timer fires.
>
> http://codereview.chromium.org/6659006/
>

[Sam Leffler, 2011-03-22 21:37:14]

PTAL; I think it's time to get this work in.  I've been running it for a while
w/o obvious problems but given the Google-A breakage I haven't been able to
re-test that bit.  My plan is to commit w/o enabling push/pop, wait for Google-A
to work, do more testing, and then enable.

Most recent changes added kicking auto-connect on push/pop so connectivity is
re-established on login/logout as needed.  I also fixed the cmd line --push op.

[stevenjb (google-dont-use), 2011-03-22 21:50:48]

Hey Sam. I spoke to Kan and Zel yesterday about multiple profile support.
Their feeling is that we need:
1. The ability to forget networks in the shared profile while logged in.
Since the "DeleteEntry" is already associated with a specific profile path,
is there any reason why this shouldn't work for any currently active
profile?
2. The ability to make a network commonly available ("shared") while logged
in. It seems like this would require a new API to move the entry from one
profile to another.

That said, I have a CL pretty much ready to go that will support multiple
profiles in Chrome. I will test it with your changes when I have a chance;
at the moment I am buried in VPN UI.

-Steven


On Tue, Mar 22, 2011 at 2:37 PM, <sleffler@chromium.org> wrote:

> PTAL; I think it's time to get this work in.  I've been running it for a
> while
> w/o obvious problems but given the Google-A breakage I haven't been able to
> re-test that bit.  My plan is to commit w/o enabling push/pop, wait for
> Google-A
> to work, do more testing, and then enable.
>
> Most recent changes added kicking auto-connect on push/pop so connectivity
> is
> re-established on login/logout as needed.  I also fixed the cmd line --push
> op.
>
> http://codereview.chromium.org/6659006/
>

[Sam Leffler, 2011-03-22 22:59:38]

On Tue, Mar 22, 2011 at 2:50 PM, Steven Bennetts <stevenjb@google.com> wrote:
> Hey Sam. I spoke to Kan and Zel yesterday about multiple profile support.
> Their feeling is that we need:
> 1. The ability to forget networks in the shared profile while logged in.
> Since the "DeleteEntry" is already associated with a specific profile path,
> is there any reason why this shouldn't work for any currently active
> profile?

No, it works (didn't include in TEST stanza).

> 2. The ability to make a network commonly available ("shared") while logged
> in. It seems like this would require a new API to move the entry from one
> profile to another.

Correct; a mechanism would need to be added.  I'm not sure this is
necessarily a good idea though.  Global entries are already available
when a user is logged in so moving global->private is not needed.
Moving something from private->global won't always work (e.g.
Google-A) and in general you are leaking credentials which seems like
a bad idea.  If you really want something global you can already
logout and then join the network.

Perhaps we should talk offline and go through the usage scenarios.

> That said, I have a CL pretty much ready to go that will support multiple
> profiles in Chrome. I will test it with your changes when I have a chance;
> at the moment I am buried in VPN UI.
> -Steven
>
> On Tue, Mar 22, 2011 at 2:37 PM, <sleffler@chromium.org> wrote:
>>
>> PTAL; I think it's time to get this work in.  I've been running it for a
>> while
>> w/o obvious problems but given the Google-A breakage I haven't been able
>> to
>> re-test that bit.  My plan is to commit w/o enabling push/pop, wait for
>> Google-A
>> to work, do more testing, and then enable.
>>
>> Most recent changes added kicking auto-connect on push/pop so connectivity
>> is
>> re-established on login/logout as needed.  I also fixed the cmd line
>> --push op.
>>
>> http://codereview.chromium.org/6659006/
>
>

[Eric Shienbrood, 2011-03-23 21:14:21]

Mostly comments about the API documentation.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt
File doc/manager-api.txt (right):

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode45
doc/manager-api.txt:45: only by, the connection manager.
comma not needed.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode47
doc/manager-api.txt:47: If any existing profile is specified it's contents
it's => its

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode51
doc/manager-api.txt:51: [service].Error.AlreadyExists
An AlreadyExists error seems inconsistent with the explanation of what happens
when an existing profile is specified.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode53
doc/manager-api.txt:53: object PushProfile(string name)
Why isn't the argument an object path instead of a string? What is the meaning
of the return value?

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode65
doc/manager-api.txt:65: [service].Error.AlreadyExists
Is the default profile always at the bottom of the stack?

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode83
doc/manager-api.txt:83: not write any state to persistent storage.
In other words, it's possible to pop the default profile?

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode95
doc/manager-api.txt:95: void RemoveProfile(object path)
According to the code in manager.c, this now takes a string argument.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode102
doc/manager-api.txt:102: first.
Presumably, the profile is removed from the stack if it is present there?

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode104
doc/manager-api.txt:104: At minimum one profile must be available all the time.
What does "available" mean here? Above, under PopProfile, it says it's valid to
pop all profiles.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode309
doc/manager-api.txt:309: boolean OfflineMode [readonly]
According to the code in manager.c, this property is still writable.

http://codereview.chromium.org/6659006/diff/21001/doc/profile-api.txt
File doc/profile-api.txt (right):

http://codereview.chromium.org/6659006/diff/21001/doc/profile-api.txt#newcode54
doc/profile-api.txt:54: service has it's security credentials revoked but
it's => its

http://codereview.chromium.org/6659006/diff/21001/doc/profile-api.txt#newcode74
doc/profile-api.txt:74: results in powering down all devices.
It's probably worth saying that changes to this are always saved to the default
profile, so that they affect all users.

http://codereview.chromium.org/6659006/diff/21001/src/profile.c
File src/profile.c (right):

http://codereview.chromium.org/6659006/diff/21001/src/profile.c#newcode203
src/profile.c:203: }
Is it worth factoring out this stack-walking pattern into a function, and
passing the load function as an argument?

http://codereview.chromium.org/6659006/diff/21001/src/profile.c#newcode1085
src/profile.c:1085: * Check this profile is not already on the stack.
I don't think "already" is right in this context.

[Sam Leffler, 2011-03-26 00:30:03]

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt
File doc/manager-api.txt (right):

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode45
doc/manager-api.txt:45: only by, the connection manager.
Done

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode47
doc/manager-api.txt:47: If any existing profile is specified it's contents
Done

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode51
doc/manager-api.txt:51: [service].Error.AlreadyExists
Added an explanation that this is returned when a profile was previously
registered with CreateProfile or PushProfile.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode53
doc/manager-api.txt:53: object PushProfile(string name)
It's not an object path because there may not be an in-memory object backing the
on-disk profile data.  The usual scenario is that a profile is created once,
then push/pop'd many times.  Since we don't/can't auto-load all the profiles all
you may have is the identifier (e.g. ~chronos/foo).

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode65
doc/manager-api.txt:65: [service].Error.AlreadyExists
Not always; you can create an arbitrary stack contents.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode83
doc/manager-api.txt:83: not write any state to persistent storage.
Yes, intentionally so.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode95
doc/manager-api.txt:95: void RemoveProfile(object path)
Fixed

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode102
doc/manager-api.txt:102: first.
Actually I forgot to change this language.  The way things are implemented you
cannot remove a profile that is on the stack.  I'm not sure I even want this
method available as I don't see a lot of use for it and it's potentially risky. 
We probably could get away with using touch/rm but having methods encapsulates
the pathnames so I left it.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode104
doc/manager-api.txt:104: At minimum one profile must be available all the time.
This is leftover; removed.

http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode309
doc/manager-api.txt:309: boolean OfflineMode [readonly]
This refers to setting the property through a profile object.  I intentionally
removed that since all usage presently goes through the Manager object and
OfflineMode is only meaningful for the default profile.

[Eric Shienbrood, 2011-03-29 14:20:55]

Thanks for these responses. There were also a few comments in profile-api.txt
and profile.c that need replies.

On 2011/03/26 00:30:03, Sam Leffler wrote:
> http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt
> File doc/manager-api.txt (right):
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode45
> doc/manager-api.txt:45: only by, the connection manager.
> Done
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode47
> doc/manager-api.txt:47: If any existing profile is specified it's contents
> Done
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode51
> doc/manager-api.txt:51: [service].Error.AlreadyExists
> Added an explanation that this is returned when a profile was previously
> registered with CreateProfile or PushProfile.
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode53
> doc/manager-api.txt:53: object PushProfile(string name)
> It's not an object path because there may not be an in-memory object backing
the
> on-disk profile data.  The usual scenario is that a profile is created once,
> then push/pop'd many times.  Since we don't/can't auto-load all the profiles
all
> you may have is the identifier (e.g. ~chronos/foo).
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode65
> doc/manager-api.txt:65: [service].Error.AlreadyExists
> Not always; you can create an arbitrary stack contents.
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode83
> doc/manager-api.txt:83: not write any state to persistent storage.
> Yes, intentionally so.
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode95
> doc/manager-api.txt:95: void RemoveProfile(object path)
> Fixed
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode102
> doc/manager-api.txt:102: first.
> Actually I forgot to change this language.  The way things are implemented you
> cannot remove a profile that is on the stack.  I'm not sure I even want this
> method available as I don't see a lot of use for it and it's potentially
risky. 
> We probably could get away with using touch/rm but having methods encapsulates
> the pathnames so I left it.
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode104
> doc/manager-api.txt:104: At minimum one profile must be available all the
time.
> This is leftover; removed.
> 
>
http://codereview.chromium.org/6659006/diff/21001/doc/manager-api.txt#newcode309
> doc/manager-api.txt:309: boolean OfflineMode [readonly]
> This refers to setting the property through a profile object.  I intentionally
> removed that since all usage presently goes through the Manager object and
> OfflineMode is only meaningful for the default profile.

[Sam Leffler, 2011-03-29 18:25:35]

http://codereview.chromium.org/6659006/diff/21001/doc/profile-api.txt
File doc/profile-api.txt (right):

http://codereview.chromium.org/6659006/diff/21001/doc/profile-api.txt#newcode54
doc/profile-api.txt:54: service has it's security credentials revoked but
Done

http://codereview.chromium.org/6659006/diff/21001/doc/profile-api.txt#newcode74
doc/profile-api.txt:74: results in powering down all devices.
Done

http://codereview.chromium.org/6659006/diff/21001/src/profile.c
File src/profile.c (right):

http://codereview.chromium.org/6659006/diff/21001/src/profile.c#newcode203
src/profile.c:203: }
You can't refactor this w/o dealing with the different type signatures of the
load functions.  Regardless this code is gone already with the revised model
that pins data to a specific profile (will appear in a separate CL as soon as I
can convince git cl to not blow up).

http://codereview.chromium.org/6659006/diff/21001/src/profile.c#newcode1085
src/profile.c:1085: * Check this profile is not already on the stack.
Right, cut&pasto fixed.

[Eric Shienbrood, 2011-03-29 18:37:35]

LGTM