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

feat: an opaque access token #14

Closed
wants to merge 1 commit into from
Closed

feat: an opaque access token #14

wants to merge 1 commit into from

Conversation

jarkkojs
Copy link
Contributor

@jarkkojs jarkkojs commented Mar 25, 2022

`mmledger` cares only that the equality can be compared.  Otherwise, it is
ignorant of their format. By using opaque access tokens, `mmledger` could
orchestrate different types of permission blobs from variety sources
(syscalls, ELF, SGX etc.) universally.

Rather than categorizing records by their inner structural details, it is
better to classify them of their function inside the ledger.  This allows
to leverage Rust's static checker for the benefit of data consistency.

Introduce `enum Token` to represent this categorization with
`Token::Access(u64)` and `Token::Length(u64)` classes.  Provide accessor
methods for both, for coveniently unwrapping the underlying data.  This
effectively injects a consistency invanriant to each access location.

@jarkkojs jarkkojs changed the title Feat/consistency feat: an opaque access token Mar 25, 2022
Copy link
Member

@npmccallum npmccallum left a comment

Choose a reason for hiding this comment

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

Bad parts of this design:

  1. Now you have a mismatch between usize and u64. So on 32-bit environments you have wasted space.

  2. Now length, which was previously private is now public.

  3. Switching to an enum means that all evaluations of access now have to be conditionalized on the Token type. This offloads burden to the user of Ledger.

None of the above are good things.

Further, you keep wanting to make access generic. But we don't need it to be generic. If we do in the future, we can make it generic then. For now, please just stick with permissions.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Mar 25, 2022

  1. This takes same amount of space 32 bytes, and does not waste space for length fields in records, which are not the one where ledger keeps length.
  2. It was also waste data in all other records, expect the first one. Further, why given that all other fields are public, this would matter for an unused field?
  3. This does objectively provide a consistency checker for the data structure too, and as you can see from tests, and is not more difficult to use.

I've explained in the commit message the objective benefits of doing this. It will help to catch bugs.

@jarkkojs
Copy link
Contributor Author

The existing version offloads more burden to the caller than this version as it introduces an arbitrary permission structure. I.e. you need to do more conversions. These changes put less burden to the caller, and make the whole thing very concrete, not generic.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Mar 25, 2022

Why we want to be more generic supporting non-existent 32-bit environments but less generic on having a built-in consistency checker, which actually helps? How does the existing structure deliver better for these environments? This 32-bit argument is just oddball. Way more important is to have predictable byte size and alignment properties.

I care more about measurable benefits than completely artificial matters. If I revert back to something that I see lacking these benefits I'd need to know how they can be addressed in that version. Consistency checks help now, not in the future.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Mar 25, 2022

I.e. this provides help using mmledger right now because it provides better assertions, not in the future. Named flags do not provide anything quantative, other than equality relation, which token handles just fine.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Mar 25, 2022

  1. Now length, which was previously private is now public.

It is now a private field, i.e. this has been addressed. I'm not sure why a consistency check is something to be avoided but type Region = Line<Address<usize, Page>> is in priority, and why named flags for RWX is a priority. Each unwrap() checks that regions has the expected data, which is an objective fact.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Mar 25, 2022

This like additional test built into the ledger. I can also implement a fn consistency_check() function that is called in the end of every public function but that costs. This a run-time cost-free version. I think that consistency check in book-keeping critical structure like this is essential, and this is just Rust version of doing exactly that.

Named flags help absolutely nothing measurable, except make the API more complex to use when you only need to compare equality of the tokens. I.e. you have to convert all your existing permission tokens to this new arbitrary token. It just pragmatically makes no sense.

I get the same buffer argument. It makes implementing traits easier. I hear no argument how to make consistency check better (and hopefully zero cost). 32 bytes is not a lot, and it is anyway the same as it has been, 32-bit environments that do no exist can cope with that.

`mmledger` cares only that the equality can be compared.  Otherwise, it is
ignorant of their format. By using opaque access tokens, `mmledger` could
orchestrate different types of permission blobs from variety sources
(syscalls, ELF, SGX etc.) universally.

Rather than categorizing records by their inner structural details, it is
better to classify them of their function inside the ledger.  This allows
to leverage Rust's static checker for the benefit of data consistency.

Introduce `enum Token` to represent this categorization with
`Token::Access(u64)` and `Token::Length(u64)` classes.  Provide accessor
methods for both, for coveniently unwrapping the underlying data.  This
effectively injects a consistency invanriant to each access location.

Closes: #13
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
@jarkkojs
Copy link
Contributor Author

Anyway, I finish up the Enarx part using this and we can compare then. Probably easiest way to sort this out.

@jarkkojs
Copy link
Contributor Author

This highlights the importance of internal consistency checks. Especially when they are zero-cost: #15

@jarkkojs jarkkojs closed this Mar 25, 2022
@jarkkojs jarkkojs deleted the feat/consistency branch March 25, 2022 23:05
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.

None yet

2 participants