-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: app usable without local secure environment support #209
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Great first step, left some comments
apps/easypid/.env
Outdated
@@ -0,0 +1,2 @@ | |||
EXPO_PUBLIC_WALLET_SERVICE_PROVIDER_AUTH_TOKEN=0dd5e0ac-f6a5-42ce-b945-09735c3a8f17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auth token doesn't really work for mobile apps as you can extract it from the build.
Or is this temporary until we can do app integrity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was very temporary. Also, I should not push the .env file haha
@@ -0,0 +1,52 @@ | |||
import type { SecureEnvironment } from '@animo-id/expo-secure-environment' | |||
|
|||
export class WalletServiceProviderClient implements SecureEnvironment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to a separate package?
await response.text() | ||
} | ||
|
||
public async getPublicBytesForKeyId(keyId: string): Promise<Uint8Array> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will probably have to sign each request with a key or something for auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should do: app integrity, issue access token bound to app integrity.
Also c' flow has some guidance on how to interact with hsm i think, did you look into that already?
@@ -0,0 +1,52 @@ | |||
import type { SecureEnvironment } from '@animo-id/expo-secure-environment' | |||
|
|||
export class WalletServiceProviderClient implements SecureEnvironment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, i'm not really sure why this implements secure env? We're going yo have to interact a lot more with the wallet provider backend (for attestations etc). I think it makes more sense to create a separate service.
Also my understanding was we'd also create other keys in the HSM, but it now looks we only do the PID key?
eb01eb2
to
80ec51c
Compare
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
82d29a9
to
87cdbda
Compare
Signed-off-by: Berend Sliedrecht <[email protected]>
apps/easypid/src/crypto/salt.ts
Outdated
return saltString | ||
} | ||
|
||
const getSalt = async (agent: EasyPIDAppAgent): Promise<string | null> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a salt should be unique per key. So I think we should provide a salt id. or we should name these methods specific to the context of the salt (so e.g. WalletServiceProviderPinKeySalt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WalletServiceProviderPinKeySalt
thats fine. This will only be used for the device key, so renaming for the method would be good, yes!
Signed-off-by: Berend Sliedrecht <[email protected]>
draft as we need a hosted version of the wallet service provider to make sure we can use it easier.
related PR: https://github.com/animo/funke-wallet-service-provider/pull/1