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

chore: update Timer.md with caveats #3827

Merged
merged 8 commits into from
Dec 4, 2024
Merged

chore: update Timer.md with caveats #3827

merged 8 commits into from
Dec 4, 2024

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Feb 22, 2023

... and suggestions. Touch on security and the default mechanism.

Also linking @dfinity-berestovskyy's timer FAQ, which is https://internetcomputer.org/docs/current/developer-docs/backend/periodic-tasks#timers-library-limitations

@ggreif ggreif requested review from roelstorms and crusso February 22, 2023 10:00
@github-actions
Copy link

github-actions bot commented Feb 22, 2023

Comparing from 6603c0a to 14e05db:
The produced WebAssembly code seems to be completely unchanged.

doc/md/base/Timer.md Outdated Show resolved Hide resolved
doc/md/base/Timer.md Outdated Show resolved Hide resolved
Comment on lines +15 to +17
to re-establish timers after an upgrade is to walk stable variables
in the `post_upgrade` hook and distill necessary timer information
from there.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some will come up with a good example. Maybe we could have made this work using a single well-known update method, but that wasn't in the Rust version either.

Choose a reason for hiding this comment

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

Is this correct?

is to walk stable variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I think it is common usage to walk (i.e. traverse) data structures.

crusso
crusso previously approved these changes Feb 22, 2023
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

But see suggestions

in the `post_upgrade` hook and distill necessary timer information
from there.

Note: Basing security (e.g. access control) on timers is almost always
Copy link

@roelstorms roelstorms Feb 23, 2023

Choose a reason for hiding this comment

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

What's the underlying reason that security controls shouldn't use timers? Or is this based on my comment in slack?

I can't come up with a good scenario in which one would want to. Just wondering if this statements mean that timers are not very reliable or that usually if you need timers in a security control you there is a design issue. The thing we wanted to remark from ProdSec is that IF you use timers for security controls, make sure to reinstate them after an upgrade.

Agreed that access control would likely not need timers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is triggered by your slack comment. There are access control methods that use time, e.g. exponential lockout for mistyped passwords etc. These are problematic here, as there is no protection against reentrancy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a blurb. PTAL.

Choose a reason for hiding this comment

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

I missed this comment back then. LGTM if you still want to merge it.

@luc-blaeser
Copy link
Contributor

Just doing some repo cleanup for better focussing on active work:
Closing PR as it seems inactive for a longer time. Please re-open if it is still relevant and after it has been updated with latest master changes.

@luc-blaeser luc-blaeser closed this Dec 3, 2024
@ggreif ggreif reopened this Dec 4, 2024
Copy link

@roelstorms roelstorms left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for improving the docs!

@ggreif ggreif self-assigned this Dec 4, 2024
@ggreif ggreif added the automerge-squash When ready, merge (using squash) label Dec 4, 2024
@ggreif ggreif merged commit eb3cbd3 into master Dec 4, 2024
9 checks passed
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Dec 4, 2024
@mergify mergify bot deleted the gabor/timer-explainer branch December 4, 2024 16:35
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.

4 participants