Skip to content
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

XSWD permission changes and tests #176

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

SixofClubsss
Copy link

Description

XSWD permission changes and further enhancements to the XSWD protocol.

dApps can submit permissions to perform requests with their ApplicationData when they initially connect to the wallet. If these permissions are unknowingly accepted by the wallet user when connection is made the wallet may not receive any further prompts for this dApps requests due to requestPermission() returning any stored permission.

func (x *XSWD) requestPermission(app *ApplicationData, request *jrpc2.Request) Permission {

Proposed solutions to address this:

  • forceAsk param added to the server and hard coded true in NewXSWDServer() so that no permission requests upon initial connection will be accepted from dApp to wallet. (defaults all wallet requests to Ask behavior)

  • Package logger updated to display further permission info.

  • CLI wallet inherits the above changes making incoming and current permissions clearer. It continues to use NewXSWDServer() so it will not accept any initial permission requests.

  • If using NewXSWDServerWithPort() forceAsk may be false allowing initial permissions to be requested. Changes made to addApplication() for filtering permission requests before attempting to add a application. On the initial connection all of the following will be rejected (behaves as Ask):

    • Allow or Deny as they are positive and negative
    • Methods outside of rpcserver/xswd
    • Conflicting requests, ex [getbalance: Ask, GetBalance: AlwaysAllow]
    • Daemon methods as they behaves as AllwaysAllow

Further enhancements

  • The app signature was further integrated into requesting permissions. Signature is now a DERO signed message and for XSWD to consider it valid (dApp can request permissions) the message of the signature must match the applications ID. If signature is submitted and does not match ID, the connection will be automatically rejected. Signature is not required if no permission are to be requested.

  • More client side info on connection acceptance/rejection.

  • Enable interrupt signal during CLI prompts.

  • Prevented custom method crossover to rpcserver.

  • Completed XSWD SignData method and added CheckSignature, GetDaemon methods.

  • Synchronized daemon pass through calls to return RPCResponse same as wallet calls are.

  • ctx, cancel for the server so there is no orphaned handler_loops.

  • Added package tests at 90% coverage.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This will require a HardFork to be enabled

Which part is impacted ?

  • Wallet
  • Daemon
  • Miner
  • Explorer
  • Simulator
  • Misc (documentation, comments, text...)

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

License

Im am contributing & releasing the code under DERO Research License (which can be found here).

@Slixe
Copy link
Contributor

Slixe commented May 1, 2024

looks cool! thanks

But, I was waiting on Captain to have another signature function to implement it for permissions because:

  • SignData is exposed, relying on it would be a risk
  • Application ID is selected by the application that try to connect

During a previous connection he could be able to request SignData method, sign its application ID and then reconnect to inject permissions.

The initial plan was that signature is based on AppID + permissions map, so permissions are reviewed before being signed for this application. Here he can set any permission he want as long the user signed the appID.

lmk what you think

@SixofClubsss
Copy link
Author

Thanks for the feedback. Sounds like you had some similar thoughts on the topic of permissions and signatures.

SignData would be accessible as a XSWD request yes. It is permissoned so that the wallet user can choose to accept or deny the request. Attempting unauthorized SignData requests are in the package tests.
https://github.com/civilware/derohe/blob/ef4cd10335bc798c9d1d28450f635dc9578b7a2b/walletapi/xswd/xswd_test.go#L489
https://github.com/civilware/derohe/blob/ef4cd10335bc798c9d1d28450f635dc9578b7a2b/walletapi/xswd/xswd_test.go#L913

Application ID is selected by dApp yes. When using software contained within this PR, if a dApp did as described and requested a signature which the user then chose to accept, and then dApp proceeds to reconnect using that signature. No permissions would be applied upon connection as the wallets uses the added forceAsk param set to true. Package tests are looking for unauthorized permissions when forceAsk is true.
https://github.com/civilware/derohe/blob/ef4cd10335bc798c9d1d28450f635dc9578b7a2b/walletapi/xswd/xswd_test.go#L539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants