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

Changes made: #39

Merged
merged 23 commits into from
Nov 6, 2021
Merged

Changes made: #39

merged 23 commits into from
Nov 6, 2021

Conversation

sokorototo
Copy link
Collaborator

  • Originally since the loader checked if signatures existed from the flags in the Header, you could only toggle signatures globally. This was also the reason why Flags::SIGNED_FLAG did not appear in the RegEntry flags, this bug, since it existed in the Header. Now since the signed flag belongs in the RegistryEntry, it can be toggled locally when building the .vach file, ie it is now scoped ( with Leaf::sign(false) ). Also, that's why RegistryEntry::from_handle(---) now takes one argument instead of two, as it reads the signatures internally instead of taking a bool from Header::flags.contains(Flags::SIGNED_FLAG).
  • Now you can't construct a Flags instance directly: with Flags { bits: 0 }, as bits is now private, and can only be accessed with Flags::bits().
  • Also Header formatting was changed to include less information, for extra information use dbg!() as fmt::Debug gives more info.

spec/main.txt Outdated
0-3 => RESERVED, [ IF FLAGS::SIGNED is SET THEN THE ARCHIVE HAS SIGNATURES ]
4-15 => CUSTOM_DATA
0-3 => RESERVED
4-16 => ALL BITS HERE ARE FAIR GAME
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be 4-15, since we start counting at 0?

@zeskeertwee
Copy link
Owner

We want this in the 0.2.0 release, @sokorototo, right?

@sokorototo
Copy link
Collaborator Author

Yes, it all comes with the 0.2.0 release

@sokorototo
Copy link
Collaborator Author

@zeskeertwee probably merge this PR because the next PR switches from chacha20poly to AES for encryption, with support for hardware acceleration. In short the next PR will be fully focused on the encryption side. And after that a better error handling framework

@zeskeertwee
Copy link
Owner

Yeah this PR is almost ready to merge, its just the documentation that has one small mistake in it. Could you fix that @sokorototo ?

@sokorototo
Copy link
Collaborator Author

What issue @zeskeertwee?

@zeskeertwee
Copy link
Owner

I left a review comment, but basically the spec says bytes go from 0-16 and it should be 0-15 @sokorototo

@sokorototo
Copy link
Collaborator Author

@zeskeertwee The spec.txt has been fixed. I have also migrated encryption from chacha20_polyfill to aes-gcm-siv. This allows for hardware-accelerated, OpenSSL-less ( issue on windows, as windows, does not come with OpenSSL) de|en-cryption. Remind me to write in the docs how one should configure hardware-accelerated AES with RUSTFLAGS

vach/src/global/edcryptor.rs Show resolved Hide resolved
vach/src/global/edcryptor.rs Show resolved Hide resolved
vach/src/loader/archive.rs Show resolved Hide resolved
@zeskeertwee
Copy link
Owner

By the way, did you consider the encryption algorithm's speed? I saw this table comparing different algorithms, and the one we use definitely isn't close to the fastest @sokorototo . RustCrypto/AEADs#243 (comment)

@sokorototo
Copy link
Collaborator Author

Good point, we should probably shift to the much faster version

@sokorototo
Copy link
Collaborator Author

It's even better that I isolated the encryption logic. So we can easily swap out algorithms

@sokorototo
Copy link
Collaborator Author

My take on encryption

I figured that encryption is not about speed, but rather for safety. I still did some benchmarks, and crypto2 requires cargo nightly, so it was evicted from the start. I do not want to touch hazmat crates, lest I jeopardize the safety of vach archives (I am not a security researcher after all). I also wanted to avoid crates that had bindings to OpenSSL, as that would require installing OpenSSL on windows. And since vach is meant to be embedded into games, it was best to use a native solution: ie AES or chacha20PolyFill (?). And turns out, either I was so severely io bound and my benchmarks were microseconds apart, or that they performed very close to each other on my computer. So I just decided to go with AES as with the right compiler flags, it out-performed chacha20. Also, for a much more modern CPU (I am rocking an i5-4590) and faster storage, I think the performance is acceptable. Besides, if you want to load data fast, store it raw or only compress it. Encryption adds a lot of steps on top of loading the data, so only use it if you actually need it. @zeskeertwee

@zeskeertwee
Copy link
Owner

Allright, that makes sense!

@zeskeertwee zeskeertwee merged commit f73893d into zeskeertwee:main Nov 6, 2021
@sokorototo
Copy link
Collaborator Author

sokorototo commented Nov 6, 2021

  • Awesome! Anyway, is version 0.2.0 ready to be published? It has a new error type, which is in need of some streamlining.
  • I also plan to separate the benchmarks from the crate source. Why? Because cargo has to download and compile criterion, which comes with 50+ dependencies and adds a lot of download bloat during development. The only issue is that this will void small incremental performance updates, as the benchmark source and the crate source will now be separate. @zeskeertwee
  • Which means, a new workspace member: vach-benchmarks

@sokorototo
Copy link
Collaborator Author

Windows build successful.

capture

@sokorototo sokorototo deleted the test-lock branch November 6, 2021 17: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.

None yet

2 participants