Skip to content
Snippets Groups Projects

implement stream isolation

Merged implement stream isolation
trinity-1686a/arti:stream-isolation into main
2 unresolved threads
Merged trinity-1686a requested to merge trinity-1686a/arti:stream-isolation into main
2 unresolved threads

should fix #73 (closed)
Use IsolationToken (glorified u64) to allow flexible isolation of unrelated streams from the API.

Also implement isolation in Arti (as in SocksProxy to Tor). The rules used are not configurable (yet), but similar to default rules for Tor (isolate by client addr, by listening port and by authentication if any)

Edited by trinity-1686a

Merge request reports

Approval is optional

Merged by Nick MathewsonNick Mathewson 3 years ago (Jul 15, 2021 4:37pm UTC)

Merge details

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
tor-units/src/lib.rs
294 295 }
295 296 }
296 297
298 /// This type represent a token used to isolate unrelated streams on different circuits.
299 #[derive(Clone, Copy, From, Debug, PartialEq, Eq, Default)]
  • Nick Mathewson
    Nick Mathewson @nickm
    Owner

    derive(From) here is dangerous, since it lets callers create any authentication token they want. I think we should leave it out for now.

    I think default here may also give unexpected results: "all streams are isolated by default" is not necessarily what people would expect. (I'm not sure about that, however.)

  • trinity-1686a
    trinity-1686a @trinity-1686a
    Author Reporter

    derive(From) was so one can create tokens the way they want, but I guess its a risk for almost nothing.

    However for Default, I think it doesn't do what you think it does. It's required to be able derive Default on ConnectPrefs, and it's not actually isolating. Tokens created with Default will have id 0, so they are different from any token created with new (that's why the static counter in new start at 1, not 0), but every default token is equal

    Edited by trinity-1686a
  • trinity-1686a changed this line in version 2 of the diff

    changed this line in version 2 of the diff

  • Please register or sign in to reply
  • Nick Mathewson
  • Nick Mathewson
    Nick Mathewson @nickm started a thread on commit 59434d0a
    • tor-circmgr/src/usage.rs
    75 76 /// This type should stay internal to the circmgr crate for now: we'll probably
    76 77 /// want to refactor it a lot.
    77 78 #[derive(Clone, Debug)]
    78 79 pub(crate) enum TargetCircUsage {
    • Nick Mathewson
      Nick Mathewson @nickm
      Owner

      In the long run, IsolationGroup should be orthogonal to the rest of the Target/SupportedCircUsage enum, so that we can declare an IsolationGroup (or not) for any usage in the future. That probably doesn't need to be done before we merge branch, however: it can wait.

    • Please register or sign in to reply
  • Nick Mathewson
  • Nick Mathewson
  • Nick Mathewson
    Nick Mathewson @nickm
    Owner

    Awesome! This is a good set of patches, and it looks like it should implement isolation well. I think we can use this as the basis for our isolation code going forward.

    I've made some notes about issues to fix in the near term and in the long term: the near-term ones should get fixed before we merge, or immediately after. Please feel free to fix anything you want to, and I'll take on the rest once you're done.

    Thanks again!

  • Nick Mathewson added Needs Revision label

    added Needs Revision label

  • trinity-1686a added 1 commit

    added 1 commit

    • 52fab4e3 - move IsolationToken to circmgr

    Compare with previous version

  • trinity-1686a added 1 commit

    added 1 commit

    • 402662f0 - make isolation_group optional in SupportedCircUsage

    Compare with previous version

  • trinity-1686a added 1 commit

    added 1 commit

    • 7eeecf8d - create new type for isolation_map

    Compare with previous version

  • trinity-1686a
    trinity-1686a @trinity-1686a
    Author Reporter

    I'm not sure how to separate IsolationGroup from Target/SupportedCircUsage cleanly, so I guess I'm done.

    Edit: last commit to reexport IsolationToken in tor-client, to not make #118 (closed) worse.

    Edited by trinity-1686a
  • trinity-1686a added 1 commit

    added 1 commit

    • e86acfe1 - reexport IsolationToken in tor-client

    Compare with previous version

  • trinity-1686a added 1 commit

    added 1 commit

    • 74ad265e - reexport IsolationToken in tor-client

    Compare with previous version

  • trinity-1686a added 1 commit

    added 1 commit

    • 93347ec8 - remove now invalid doc-comment

    Compare with previous version

  • Nick Mathewson removed Needs Revision label

    removed Needs Revision label

  • Nick Mathewson
    Nick Mathewson @nickm
    Owner

    Yes, this looks good now! I'm merging it to main. I'll open new tickets for the remaining part of the work.

    Thanks again!

  • Nick Mathewson merged

    merged

  • Nick Mathewson mentioned in issue #150 (closed)

    mentioned in issue #150 (closed)

    • Please register or sign in to reply