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

EndSession support #1227

Merged
merged 10 commits into from Aug 28, 2020
Merged

EndSession support #1227

merged 10 commits into from Aug 28, 2020

Conversation

matejcik
Copy link
Contributor

this PR introduces an implementation of the EndSession message and fixes #912

the new behavior is as follows:

  • until first Initialize message is received, any attempt to use cached data is rejected with a ProcessError("Invalid session")
  • after Initialize, a session is allocated
  • calling EndSession destroys data of that session, and resets to the "invalid session" state. Subsequent uses of cache will again raise a ProcessError

This allows us to reuse session cache slots: a session ended with EndSession is cleared away, so we can have 9 "long-lived" sessions and reuse the 10th slots for things like requesting passphrase again. Suite should start using it ASAP.

trezorctl is also updated to make use of this: every command ends the session after itself, unless the session was resumed by trezorctl -s <session_id>


open questions:

  • should we introduce a new failure code, InvalidSession, as suggested in the original bug? ISTM there is low possibility of breakage, as previously the call would end in a FirmwareError anyway.
  • should we enforce session validity in all calls, or just the ones that touch the cache? It would be cleaner, but ISTM there is no actual need to do it.

@matejcik
Copy link
Contributor Author

there is a behavior change on legacy: previously, calling e.g. GetAddress without a valid session would silently allocate one. Now it is necessary to call Initialize explicitly.

@matejcik
Copy link
Contributor Author

Randomness has changed again in the TT PIN tests. I originally thought this is because we now call client.clear_session() unconditionally, but the affected tests are those where client.clear_session() would be called either way. It's probably nothing but I'll need to look into it more closely anyway.

@tsusanka
Copy link
Contributor

tsusanka commented Aug 27, 2020

The only breaking change for hosts is that we currently require Initialize beforehand and they should be using that already anyway. Right? And EndSession is not "mandatory".

We have to test extensively.

@tsusanka
Copy link
Contributor

should we introduce a new failure code, InvalidSession, as suggested in the original bug? ISTM there is low possibility of breakage, as previously the call would end in a FirmwareError anyway.

Please, what is the original bug?

@tsusanka
Copy link
Contributor

tsusanka commented Aug 27, 2020

Btw is this going to work with HWI? Because they had some problems with our sessions didn't they?

Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve for Core.

@mmilata it might be early but would you like to have a look on a first PR as a reviewer? :)

@matejcik
Copy link
Contributor Author

The only breaking change for hosts is that we currently require Initialize beforehand and they should be using that already anyway. Right? And EndSession is not "mandatory".

It's only breaking on T1. On TT since 2.3.0 doing anything without Initialize would result in a FirmwareError anyway.

other than that, yes. well-behaved clients will see no changes, and it is not necessary to use EndSession

Please, what is the original bug?

i meant the original issue, it's not a bug :) #912

Btw is this going to work with HWI? Because they had some problems with our sessions didn't they?

i believe so, but we should do some tests anyway

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very familiar with the session handling but for the untrained eye the change is looking good 👍

@@ -128,6 +161,15 @@ def call_Initialize(**kwargs):
call_Initialize(session_id=session_id)
self.assertEqual(cache.get(KEY), "hello")

def test_EndSession(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this work without @mock_storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mock_storage is only required for Initialize, which calls apps.base.get_features, which looks into storage fields

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw maybe also consider documenting EndSession message in docs/common/communication/sessions.md?

@matejcik matejcik merged commit 5e27c8a into master Aug 28, 2020
@matejcik matejcik deleted the matejcik/end-session branch August 28, 2020 13:37
@prusnak prusnak mentioned this pull request Dec 11, 2021
4 tasks
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.

explicit closing of sessions
4 participants