auto-lock for trezor-core #1027

matejcik · 2020-05-28T08:20:23Z

fixes #75

this has WAY more commits than it rightly should.

The core functionality is f1c3172, which introduces a global idle timer. It is then possible to register an idle callback, which, after a configurable timeout (we already have autolock timeout fields in storage), engages the lock.

Another big part is a new loop.spawn syscall (docs included), which serves as a "controllable" subtask on the application level. All workflow tasks are now spawned, so that it is possible to shut them down when the autolock engages. (this also answers this comment)

As a nicety, the lockscreen is now dimmed. I'm not sure about the backlight level, maybe we'll need to tweak that a little.

The remaining 80 % of the work is busywork:

workflow.kill_default() was superseded by workflow.close_others(), because we now have the capability to shut down ALL tasks, not just the default.
there is now a button_request() function that does just that in addition to ButtonRequest/ButtonAck calls. The logic here is that when you are calling ButtonRequest, you want to do something on the UI, and so you need to shut down whatever was there before you.
debuglink.wait_layout has been reworked. If you are using wait_layout, you need to wait for all layout changes that happen. In exchange, it's resilient to race conditions: you will receive all layout changes that happen.
for this reason I needed to introduce ButtonRequestType.PinEntry, to cleanly distinguish the TT asking for PIN on screen.
some UI test artifacts were cleared up by moving the "notify layout change" call to a more appropriate location

There's work to be done on the FIDO app, where another loop.spawn should be used for the keepalive loop, and that might simplify #729.

matejcik · 2020-05-28T08:27:58Z

open question: should FIDO use the global idle timer for its PIN timeout?

I think FIDO should respect the user-configured auto-lock delay, AS LONG AS it's 10 minutes (the default) or less. Opinions?

If it respects the autolock fully, we can remove the custom timeout feature of the PIN dialog.

We can also replace the custom timeout by an idle callback, for a slightly different result. As implemented, the PIN timeout applies to FIDO unlocks only. With an idle callback, it would apply to all device activity.
Do we want that?

tsusanka · 2020-05-29T09:23:56Z

UI changes:

https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/571346102/artifacts/master_diff/index.html

matejcik · 2020-05-29T09:38:17Z

the UI report seems wrong -- as if tests copied a lot of their screens from their longer friends

matejcik · 2020-05-29T10:22:20Z

fixed UI report: https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/572864736/artifacts/master_diff/index.html

core/src/apps/common/__init__.py

tsusanka · 2020-05-29T08:16:03Z

core/src/apps/common/passphrase.py

@@ -26,6 +26,7 @@ async def get(ctx: wire.Context) -> str:


 async def _request_from_user(ctx: wire.Context) -> str:
+    workflow.close_others()  # request exclusive UI access


Why is this needed? _request_on_device is calling button_request

It's because of _entry_dialog(). We are now in the passphrase flow and that always puts something on screen

I mean why it is not in _entry_dialog then. In _entry_dialog for _request_on_host and _request_on_device has it in button_request.

It makes more sense to me like this.

Before (no "request UI exclusivity" exists): we draw things as needed, and as a special case, in _entry_dialog we also need to make sure that default is killed.

Now: in _request_from_user we are entering the UI-enabled part of passphrase entry. Request UI exclusivity because we need it in either case: the fact that button_request() does the same thing later becomes an implementation detail.

tsusanka · 2020-05-29T08:31:16Z

core/src/apps/homescreen/lockscreen.py

+        if __debug__ and self.should_notify_layout_change:
+            from apps.debug import notify_layout_change
+
+            self.should_notify_layout_change = False
+            notify_layout_change(self)


It seems at least this block we could refactor to separate ui.Layout function and use here.

That doesn't really seem worth it? This is a one-off. If we end up doing something like this in more places, the requirements will become clearer.

(fwiw if we want to get rid of this, we could change backlight_fade(style.BACKLIGHT_NORMAL) to backlight.fade(self.backlight_level) and remove the copy-paste. It didn't seem worth polluting all Layouts with a new variable, but maybe the cost is negligible.
same thing with the loop.sleep() which could also be a class or instance variable, and in case of homescreen and lockscreen could be changed to something like loop.DO_NOT_RESCHEDULE)

backlight.fade(self.backlight_level)

I would do it. My concern is that if we change something in Layout.handle_rendering we forget to add it here. IMHO.

done in 7a2b8e7

core/src/apps/management/apply_settings.py

core/src/storage/device.py

tsusanka · 2020-05-29T11:33:44Z

Btw this is not turned on on default for both old and new users correct? I should talk to Product whether we would like to turn this on in Suite by default (aka during onboarding).

I have reviewed the code without the fido2 part. The loop and workflow code looks alright as well AFAICT!

core/src/trezor/loop.py

core/src/trezor/workflow.py

matejcik · 2020-05-29T12:52:14Z

Btw this is not turned on on default for both old and new users correct? I should talk to Product whether we would like to turn this on in Suite by default (aka during onboarding).

Very good point.

With this PR, auto-lock is always enabled. If the user never configured it, the default timeout of 10 minutes is used.
IMHO this is what we actually want, the "always unlocked" behavior from previous revisions was not desirable. But it is a Product consideration.

core/src/trezor/workflow.py

core/src/trezor/loop.py

andrewkozlik · 2020-05-29T14:53:12Z

Unable to test. When I load this firmware on a physical device I get a black screen after startup.

tsusanka · 2020-05-29T14:56:11Z

Unable to test. When I load this firmware on a physical device I get a black screen after startup.

That's because this PR is missing b01b24f. That is why force-pushes are needed sometimes :/. Maybe let's finish the review and rebase then?

matejcik · 2020-06-01T10:29:17Z

I tried to rebase on master and run this on a real hardware. Currently the device freezes after the initial startup, I'm investigating.

matejcik · 2020-06-02T09:42:24Z

i fixed the scheduler issue by lowering resolution, and cherry-picked b01b24f so that this branch can now run on real hardware. Ready for review again.

andrewkozlik · 2020-06-02T11:14:42Z

I don't think that the screen should be dimmed in the "Not connected" state. I am OK with it dimming after a few minutes of inactivity, but not from the start.
Is it just my impression, or has the "Not connected"/"Locked" label shifted downwards? It doesn't look centred to me (in relation to the standard backgrounds we have).

matejcik · 2020-06-02T11:17:44Z

Is it just my impression, or has the "Not connected"/"Locked" label shifted downwards? It doesn't look centred to me (in relation to the standard backgrounds we have).

I don't think I moved this. Compared to 2.3.0 it looks the same.

tsusanka · 2020-06-02T10:54:02Z

core/src/trezor/loop.py

-    value is the calculated deadline.
+    """Pause current task and resume it after given delay.
+
+    Resultvalue is the calculated deadline.


fixed (i think?) in 50052a6

tsusanka · 2020-06-02T10:57:58Z

core/src/storage/device.py

@@ -40,6 +40,8 @@
 HOMESCREEN_MAXSIZE = 16384
 AUTOLOCK_DELAY_MINIMUM = 10 * 1000  # 10 seconds
 AUTOLOCK_DELAY_DEFAULT = 10 * 60 * 1000  # 10 minutes
+# autolock intervals larger than AUTOLOCK_DELAY_MAX cause issues in the scheduler


.. AUTOLOCK_DELAY_MAXIMUM ..

Yes, I am nit-picking 👼 .

done in a00abbf

andrewkozlik · 2020-06-02T11:35:22Z

Is it just my impression, or has the "Not connected"/"Locked" label shifted downwards? It doesn't look centred to me (in relation to the standard backgrounds we have).

I don't think I moved this. Compared to 2.3.0 it looks the same.

I see it's also not showing the label, so that might be causing a shift in the background image.

Master

Autolock

…xternally

so that pytest doesn't think it is a testcase

The original wait_layout was unreliable, because there are no guarantees re order of arrival of the respective events. Still, TT's event handling is basically deterministic, so as long as the host sent its messages close enough to each other, the order worked out. This is no longer the case with the introduction of loop.spawn: TT's behavior is still deterministic, but now ButtonAck is processed *before* the corresponding wait_layout, so the waiting side waits forever. In the new process, the host must first register to receive layout events, and then receives all of them (so the number of calls to wait_layout must match the number of layout changes). DebugLinkWatchLayout message must be version-gated, because of an unfortunate collection of bugs in previous versions wrt unknown message handling; and this interests us because upgrade-tests are using wait_layout feature.

this makes sense, really: close_others() requests UI exclusivity, and that is something that generally happens at the same places we emit a ButtonRequest

This will have unintended consequences if you call a wirelink function on the debulink interface. TT allows this ... and will behave badly.

This prevents a race condition where sometimes an Initialize message could arrive before the homescreen was fully booted -- and Recovery homescreen would cancel it as part of its bootup sequence.

…n tests)

This avoids problems with large timeouts causing the scheduler queue to think the time counter has overflown, and ordering the autolock task before immediate tasks. The maximum reasonable time difference is 0x20000000, which in microseconds is ~8 minutes, but in milliseconds a more reasonable ~6 days.

matejcik requested review from andrewkozlik, onvej-sl, prusnak and tsusanka as code owners May 28, 2020 08:20

matejcik force-pushed the matejcik/autolock branch from 277cb17 to 76b01a0 Compare May 28, 2020 12:47

matejcik removed the request for review from onvej-sl May 28, 2020 12:51

prusnak removed their request for review May 28, 2020 12:54

matejcik mentioned this pull request May 29, 2020

Soft-locking for Trezor T #958

Merged

7 tasks

matejcik force-pushed the matejcik/autolock branch from 660d4f4 to 8f3480b Compare May 29, 2020 09:55

tsusanka reviewed May 29, 2020

View reviewed changes

andrewkozlik reviewed May 29, 2020

View reviewed changes

core/src/trezor/loop.py Show resolved Hide resolved

andrewkozlik reviewed May 29, 2020

View reviewed changes

core/src/trezor/workflow.py Show resolved Hide resolved

andrewkozlik reviewed May 29, 2020

View reviewed changes

core/src/trezor/workflow.py Outdated Show resolved Hide resolved

andrewkozlik reviewed May 29, 2020

View reviewed changes

core/src/trezor/loop.py Outdated Show resolved Hide resolved

matejcik requested review from tsusanka and andrewkozlik June 2, 2020 09:42

tsusanka reviewed Jun 2, 2020

View reviewed changes

matejcik added 27 commits June 4, 2020 15:38

core/loop: introduce spawn syscall

ac8e0ae

core: replace workflow.on_start/on_close with workflow.spawn

bc09fc2

core: make sure that auto-lock shuts down running workflows

c1e3451

tests: enable auto-lock tests for TT

0f77e81

tests: update UI test fixtures

66d6765

core/loop: prevent finalizers from doing things when task is closed e…

7da63b7

…xternally

tests: rename test_address to get_test_address

8aca7b9

so that pytest doesn't think it is a testcase

common: introduce ButtonRequestType.PinEntry

c58a0c9

core: use ButtonRequestType.PinEntry for PIN entry

b4da6bf

core: bump version (necessary for wait_layout detection)

0eb7ce5

tests: move autolock tests to a separate file

e385ad6

core: call close_others() in place of ButtonRequest

0c08397

this makes sense, really: close_others() requests UI exclusivity, and that is something that generally happens at the same places we emit a ButtonRequest

tests: fix wait_layout() behavior

acba6aa

core: replace workflow.kill_default with workflow.close_others

1de58d4

core: add Cancel to a list of allowed messages while locked

991649a

core: debuglink interface avoids workflow management

40f6505

This will have unintended consequences if you call a wirelink function on the debulink interface. TT allows this ... and will behave badly.

core: fix artifacts in click-based UI tests

1d2d6eb

core: start USB after booting apps

e65c350

This prevents a race condition where sometimes an Initialize message could arrive before the homescreen was fully booted -- and Recovery homescreen would cancel it as part of its bootup sequence.

tests: update UI test fixtures

cf6d594

core: only unlock storage if it is locked (solves determinism issue i…

e18a3c5

…n tests)

tests/ui_tests: use separate temporary directories for test cases

2340671

core: simplify homescreen and lockscreen implementations

c44050c

core: set a scheduler-safe maximum for autolock

9a944b8

core/ui: lower lockscreen brightness

4573b6a

core: fix rendering issues in homescreens

093f886

matejcik force-pushed the matejcik/autolock branch from 54ffbb5 to 093f886 Compare June 4, 2020 13:41

matejcik merged commit 093f886 into matejcik/softlock Jun 4, 2020

matejcik deleted the matejcik/autolock branch June 4, 2020 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto-lock for trezor-core #1027

auto-lock for trezor-core #1027

matejcik commented May 28, 2020 •

edited

Loading

matejcik commented May 28, 2020

tsusanka commented May 29, 2020

matejcik commented May 29, 2020

matejcik commented May 29, 2020

tsusanka May 29, 2020

matejcik May 29, 2020

tsusanka May 29, 2020

matejcik Jun 1, 2020

tsusanka May 29, 2020 •

edited

Loading

matejcik May 29, 2020

tsusanka May 29, 2020

matejcik Jun 1, 2020

tsusanka commented May 29, 2020 •

edited

Loading

matejcik commented May 29, 2020

andrewkozlik commented May 29, 2020

tsusanka commented May 29, 2020

matejcik commented Jun 1, 2020

matejcik commented Jun 2, 2020

andrewkozlik commented Jun 2, 2020

matejcik commented Jun 2, 2020

tsusanka Jun 2, 2020

matejcik Jun 2, 2020

tsusanka Jun 2, 2020

matejcik Jun 2, 2020

andrewkozlik commented Jun 2, 2020

		@@ -26,6 +26,7 @@ async def get(ctx: wire.Context) -> str:


		async def _request_from_user(ctx: wire.Context) -> str:
		workflow.close_others() # request exclusive UI access

auto-lock for trezor-core #1027

auto-lock for trezor-core #1027

Conversation

matejcik commented May 28, 2020 • edited Loading

matejcik commented May 28, 2020

tsusanka commented May 29, 2020

matejcik commented May 29, 2020

matejcik commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusanka May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsusanka commented May 29, 2020 • edited Loading

matejcik commented May 29, 2020

andrewkozlik commented May 29, 2020

tsusanka commented May 29, 2020

matejcik commented Jun 1, 2020

matejcik commented Jun 2, 2020

andrewkozlik commented Jun 2, 2020

matejcik commented Jun 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkozlik commented Jun 2, 2020

matejcik commented May 28, 2020 •

edited

Loading

tsusanka May 29, 2020 •

edited

Loading

tsusanka commented May 29, 2020 •

edited

Loading