Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add proper authorisation on the key manager facade #6951
Conversation
| @@ -153,3 +154,23 @@ func newControllerUserFromGroup(everyoneAccess permission.UserAccess, | ||
| everyoneAccess.UserName = userTag.Id() | ||
| return everyoneAccess | ||
| } | ||
| + | ||
| +// HasModelAdmin checks the specified model and returns true if the user |
axw
Feb 9, 2017
Member
I can't parse this comment. Acting on their own behalf? wat?
How about:
HasModelAdmin reports whether or not a user has admin access to the specified model.
A user has model access if they are the model owner, if they are a controller superuser,
or if they have been explicitly granted admin access to the model.
| check: common.NewBlockChecker(st), | ||
| }, nil | ||
| } | ||
| +func (api *KeyManagerAPI) checkCanRead(sshUser string) error { | ||
| + if api.checkCanWrite(sshUser) == nil { |
| + api.state.UserAccess, | ||
| + *api.apiUser, | ||
| + permission.ReadAccess, | ||
| + api.state.ControllerTag(), |
wallyworld
Feb 9, 2017
Owner
yeah it is, the controller tag is passed in so that superuser can be checked
axw
Feb 9, 2017
Member
No it's not. common.HasPermission takes the tag of an object for which we check the user's access.
| + // Are we a machine agent writing the Juju system key. | ||
| + if api.apiUser == nil { | ||
| + if sshUser == config.JujuSystemKey { | ||
| + _, ismachinetag := api.authorizer.GetAuthTag().(names.MachineTag) |
axw
Feb 9, 2017
Member
isn't this all implied by the fact that api.apiUser is nil? because apiUser is nil we know it's a controller machine agent, ergo it has access to read the system key?
wallyworld
Feb 9, 2017
Owner
this was from the original code - not only was machine agent checked, but also the ssh key passed in as a param had to be equal to the system key
axw
Feb 9, 2017
Member
Isn't the aim always to make things better? I won't block on this, but I don't see why we can't drop it.
axw
Feb 9, 2017
Member
Also, the new code has different behaviour. Is that intentional? As it is, model owners/admins will now be able to modify the system key.
wallyworld
Feb 9, 2017
Owner
It's not such much not wanting to make it better - but not wanting to mess with the existing behaviour. If the ssh key param was being checked before (even after we knew it was a machine agent connecting), I didn't want to change that now. Otherwise a machine agent could change something other than the system key.
I though it would be ok for admins to be able to modify the system key - they have admin right? But maybe that's not a good idea? I could check that for users, it's not the system key
axw
Feb 9, 2017
Member
It's not such much not wanting to make it better - but not wanting to mess with the existing behaviour. If the ssh key param was being checked before (even after we knew it was a machine agent connecting), I didn't want to change that now. Otherwise a machine agent could change something other than the system key.
Maybe there's some misunderstanding, because I don't understand your response. I didn't mean we should stop checking the ssh key param. I meant that the "ismachinetag" bits are redundant, since we already know the authorized entity is a controller machine agent.
I though it would be ok for admins to be able to modify the system key - they have admin right? But maybe that's not a good idea? I could check that for users, it's not the system key
I would err on the side of caution and leave it as it was: have it return ErrPerm if a user attempts to modify (or read!) the system identity key.
| @@ -113,8 +135,8 @@ func (api *KeyManagerAPI) ListKeys(arg params.ListSSHKeys) (params.StringsResult | ||
| for i, entity := range arg.Entities.Entities { | ||
| // NOTE: entity.Tag isn't a tag, but a username. | ||
| - if !api.canRead(entity.Tag) { | ||
| - results[i].Error = common.ServerError(common.ErrPerm) | ||
| + if err := api.checkCanRead(entity.Tag); err != nil { |
wallyworld
Feb 9, 2017
Owner
histerical reasons - username is not even used anyway, only "admin" is ever passed in by the juu cli and it is effectively ignored IIANM. Te original implementation was all cocked up I think
| @@ -194,8 +216,9 @@ func (api *KeyManagerAPI) AddKeys(arg params.ModifyUserSSHKeys) (params.ErrorRes | ||
| return result, nil | ||
| } | ||
| - if !api.canWrite(arg.User) { | ||
| - return params.ErrorResults{}, common.ServerError(common.ErrPerm) | ||
| + // NOTE: arg.User isn't a tag, but a username. |
axw
Feb 9, 2017
Member
The comment is necessary when passing in entity.Tag (which is awful, we should not have used Entities for passing in non-tagged things). It's just confusing here.
| @@ -285,8 +308,9 @@ func (api *KeyManagerAPI) ImportKeys(arg params.ModifyUserSSHKeys) (params.Error | ||
| return result, nil | ||
| } | ||
| - if !api.canWrite(arg.User) { | ||
| - return params.ErrorResults{}, common.ServerError(common.ErrPerm) | ||
| + // NOTE: arg.User isn't a tag, but a username. |
| @@ -364,8 +388,9 @@ func (api *KeyManagerAPI) DeleteKeys(arg params.ModifyUserSSHKeys) (params.Error | ||
| return result, nil | ||
| } | ||
| - if !api.canWrite(arg.User) { | ||
| - return params.ErrorResults{}, common.ServerError(common.ErrPerm) | ||
| + // NOTE: arg.User isn't a tag, but a username. |
| - s.assertEnvironKeys(c, append(initialKeys, key3, keymv[0], keymv[1], keymp[0], key4)) | ||
| + s.assertKeysForModel(c, st, append(initialKeys, key3, keymv[0], keymv[1], keymp[0], key4)) | ||
| +} | ||
| + |
axw
Feb 9, 2017
Member
please add another test that shows that users cannot touch the system key (or can touch it, if that change really was intentional)
| + user := s.Factory.MakeUser(c, &factory.UserParams{Name: "admin" + otherModel.ModelTag().String()}) | ||
| + s.assertImportKeys(c, otherModel, user.UserTag(), true) | ||
| +} | ||
| + |
axw
Feb 9, 2017
Member
If it's even possible, please add a test for model owner that is not an admin. I don't know that it's sensible for the model to allow an owner to not be an admin, but the code caters for it so we should test it (esp. given the accidental ControllerTag use).
| + } else if err != common.ErrPerm { | ||
| + return errors.Trace(err) | ||
| + } | ||
| + if api.apiUser == nil { |
axw
Feb 9, 2017
Member
if api.apiUser == nil || sshUser == config.JujuSystemKey
and please add a test that shows that a user cannot read the system key
| - s.assertEnvironKeys(c, append(initialKeys, key3, keymv[0], keymv[1], keymp[0], key4)) | ||
| + s.assertKeysForModel(c, st, append(initialKeys, key3, keymv[0], keymv[1], keymp[0], key4)) | ||
| +} | ||
| + |
axw
Feb 9, 2017
Member
please add another test that shows that users cannot touch the system key (or can touch it, if that change really was intentional)
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedFeb 9, 2017
Description of change
The facade to manipulate ssh keys in the Juju was not checking permissions properly. Specifically, model admins or controller superusers could not import, add, delete keys. This has been fixed.
As a driveby, the fake authorizer has EnvironManager renamed to Controller.
QA steps
bootstrap
create a user and login as that user
juju ssh-import-id -> fail
as controller admin, grant that user admin on the model
juju ssh-import-id -> success
repeat above with superuser access granted
Bug reference
https://bugs.launchpad.net/juju/+bug/1663035