Added KeyIdentity to replace KeyTriple #500

MattDavis00 · 2021-08-09T12:15:22Z

KeyIdentity contains an ApplicationIdentity, ProviderIdentity and a key_name.
KeyTriple still exists but solely for the use by the OnDiskKeyInfoManager.
This is preliminary work towards the new SQLiteKeyInfoManager as part of #486

Apologies for the large pr... 👨🏼‍💻

There may be some incorrect variable naming in places regarding key_triple where it should be key_identity instead. I will address this in a future PR as this will be a ~200 loc by itself.

Closes #488

Signed-off-by: Matt Davis [email protected]

ionut-arm

Thanks for the massive change! 💪🏻 I left a bunch of comments below, apologies if some of them seem to contradict each other - I went on a journey of self-discovery myself, reading through this.

src/key_info_managers/mod.rs

src/key_info_managers/on_disk_manager/mod.rs

src/key_info_managers/mod.rs

src/authenticators/mod.rs

src/key_info_managers/on_disk_manager/mod.rs

src/key_info_managers/mod.rs

src/providers/core/mod.rs

src/key_info_managers/mod.rs

ionut-arm

Another few comments, should've noticed the pub fields yesterday :(

src/authenticators/mod.rs

hug-dev

Thanks for the changes. Sorry for the big amount of review comments adding up to what you already got 😛
It is good though since this is a big and important change!

src/authenticators/mod.rs

src/key_info_managers/mod.rs

src/providers/mod.rs

MattDavis00 · 2021-08-20T15:39:35Z

Currently admin operations such as delete_client are not scoped to the authenticator of the administrator.
Example:

Apps:
app1-authenticator A
app2-authenticator B
Admin:
admin-authenticator B

Currently the admin can delete both app1 and app2. With regards to the new KIM and admin permissions, how do we want this to work? Currently only 1 authenticator is supported and hence this isn't really an issue yet. Do we want an administrator authenticated with authenticator B to be scoped only to actions on authenticator B resources, or do we want them to have "global" admin privileges where they can perform admin actions on any resource regardless of authenticator?

Edit: I have implemented the scoped version of this, rather than the "global" version of this. This is as we can always revert to a more lenient/less-restrictive set of permissions if we wish to.

KeyIdentity contains a ApplicationIdentity, ProviderIdentity and a key_name. KeyTriple still exists but solely for the use by the on_disk_manager KIM. This is preliminary work towards the new SQLite KIM as part of parallaxsecond#486 Closes parallaxsecond#488 Signed-off-by: Matt Davis <[email protected]>

hug-dev

Mainly fine for me. I think I should not block the implementation for too long since this is hard to work little by little here. And it's all OK since you are on your own branch.

Do we want an administrator authenticated with authenticator B to be scoped only to actions on authenticator B resources, or do we want them to have "global" admin privileges where they can perform admin actions on any resource regardless of authenticator?

I think the scoped version is the one that we want. Having the new KIM in mind we wrote in the book:

Only the clients using the same authentication method as this request will be deleted. It has no impact currently as only one authentication method in the service is supported but might do if the service supports multiple.

src/authenticators/direct_authenticator/mod.rs

src/key_info_managers/mod.rs

ionut-arm self-requested a review August 9, 2021 12:23

ionut-arm reviewed Aug 9, 2021

View reviewed changes

MattDavis00 force-pushed the feature-key-identity branch from 8e1a595 to 75241ff Compare August 10, 2021 11:21

ionut-arm reviewed Aug 10, 2021

View reviewed changes

src/authenticators/mod.rs Show resolved Hide resolved

src/authenticators/mod.rs Outdated Show resolved Hide resolved

hug-dev reviewed Aug 12, 2021

View reviewed changes

MattDavis00 force-pushed the feature-key-identity branch 2 times, most recently from 21b263e to 3620599 Compare August 20, 2021 10:31

MattDavis00 force-pushed the feature-key-identity branch from 3620599 to bfea080 Compare August 23, 2021 14:46

MattDavis00 force-pushed the feature-key-identity branch from bfea080 to 201e7c7 Compare August 23, 2021 16:08

hug-dev approved these changes Aug 24, 2021

View reviewed changes

MattDavis00 merged commit d353e92 into parallaxsecond:feature-sqlite-kim Aug 25, 2021

MattDavis00 deleted the feature-key-identity branch September 2, 2021 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added KeyIdentity to replace KeyTriple #500

Added KeyIdentity to replace KeyTriple #500

MattDavis00 commented Aug 9, 2021

ionut-arm left a comment

ionut-arm left a comment

hug-dev left a comment

MattDavis00 commented Aug 20, 2021 •

edited

Loading

hug-dev left a comment

Added KeyIdentity to replace KeyTriple #500

Added KeyIdentity to replace KeyTriple #500

Conversation

MattDavis00 commented Aug 9, 2021

ionut-arm left a comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment

hug-dev left a comment

Choose a reason for hiding this comment

MattDavis00 commented Aug 20, 2021 • edited Loading

hug-dev left a comment

Choose a reason for hiding this comment

MattDavis00 commented Aug 20, 2021 •

edited

Loading