portal: Keyring::load() verify secret before returning a Keyring #122
Conversation
warusadura
force-pushed
the
check_keyring_pwd
branch
2 times, most recently
from
ee92166
to
178a663
Compare
warusadura
force-pushed
the
check_keyring_pwd
branch
from
178a663
to
b6f67ff
Compare
warusadura
changed the title
warusadura
force-pushed
the
check_keyring_pwd
branch
3 times, most recently
from
5dfeb37
to
0a375a9
Compare
|
|
||
| let mut ret = false; | ||
|
|
||
| match Self::open(name, secret).await { |
There was a problem hiding this comment.
Not sure why I would open the keyring file a second time, when all of this verification the moment you call open.
There was a problem hiding this comment.
what second time? :)
this is a new function: verify_secret. we need this for the server side implementation. specifically to verify the secret during Prompt based unlock. this function just validates the secret and it doesn't return a keyring.
There was a problem hiding this comment.
load -> verify_secret -> open -> you end up opening reading the keyring twice. I am not sure the changes you made here even correct because on the other hand open would call load as well.... Not good.
There was a problem hiding this comment.
The reason of your confusion, is there is a method load, that is used only when interacting with a secret that comes from the secret portal. So it is assumed to be always correct where open is what you would use in the server side.
The secret verification should be done to both cases just in case the API is misused.
There was a problem hiding this comment.
load -> verify_secret -> open -> you end up opening reading the keyring twice. I am not sure the changes you made here even correct because on the other hand open would call load as well.... Not good.
Thanks. sorry for the late reply.
apologies if what I'm saying doesn't make any sense :)
so, when I call oo7::portal::Keyring::verify_secret() what happens is,
- calling
Keyring::open()right,- if the keyring is v0,
Keyring::open()will callKeyring::migrate(). in this case the keyring file is opened once and the secret verification is taken care here. there isn't another call toKeyring::load(). - if the keyring is v1
Keyring::open()will callKeyring::load(). in this case the keyring file is opened once and secret verification is done through verify_secret().
- if the keyring is v0,
so, for both cases: Keyring::load(), Keyring::open() secret verification are covered now and the keyring file is opened once during the vertification. am I still missing something here?
There was a problem hiding this comment.
What I meant is:
-
If you use
Keyring::open,- We verify if v1 exists -> call Keyring::load on it
- if v0 exists, call Keyring::migrate which by itself verifies the secret as it decrypts the v0 items to migrate them to v1
-
So basically, by handling the verification in Keyring::load code path, you cover all the use cases.
client/src/portal/api/mod.rs
Outdated
| } | ||
|
|
||
| // attempting to decrypt the first item | ||
| if self.items[0].clone().decrypt(&key).is_ok() { |
There was a problem hiding this comment.
Regarding style,
if let Some(value) = self.items.get(0) is simpler than asking if the vec is empty and then calling [0].
There was a problem hiding this comment.
Now, I'm decrypting all the items. Does it still make sense to follow this approach?
Adds `verify_secret()`. With this change, calls to `oo7::portal::Keyring::load()`, and `oo7::portal::Keyring::open()` will fail when used with a wrong secret/password. Also adds a new error: `InvalidSecret`. Signed-off-by: Dhanuka Warusadura <[email protected]>
This change introduces verify_keyring_secret() to verify the secret associated with a keyring. Note: we need this change for the sever side implementation. Specifically we need this to verify the secret during prompt based keyring unlock. Signed-off-by: Dhanuka Warusadura <[email protected]>
warusadura
force-pushed
the
check_keyring_pwd
branch
from
b2b854b
to
a16c422
Compare
|
I pushed a simpler implementation at 3ba3e8d |
Adds
verify_secret().With this change, calls to
oo7::portal::Keyring::load(), andoo7::portal::Keyring::open()will fail when used with awrong secret/password.
Also adds
oo7::portal::Keyring::verify_secret()API and introducesInvalidSecreterror.