Split SecureKey into smaller SecureKeys

[timshadel]

I want to use pwHash to generate enough key bytes to run both a secretbox and a keyed genericHash. But I also want the memory protections provided by the internal FFI functions like copy(). I've created a method split() which will create a list of SecretKeys from an existing SecretKey. I've put it in the interface, and implemented it in FFI and web.

Please check my FFI memory handling to make sure I didn't accidentally taint it by bringing it into the vm.

Additionally, I have a commit which puts the generated files into the repo. Without this I couldn't use the git syntax in my pubspec.yaml because the dart code is used as-is from the .pub-cache. I'm experienced enough to understand the tradeoffs with committing generated code. I hope you keep this so that we can rely on git if needed for future times you may have a feature you want others to try, but don't want to release it fully yet.

[Skycoder42]

Hi, first, thanks for your contribution. I will address each part seperately:

---

Regarding the split function: You do in fact not need to extend the SecureKey to archive this, as you can operate on the lists passed to the runUnlocked* functions - these lists are mere views of the raw memory. A simply implementation, that would use these methods to copy data from one key to another would be the following:

SecureKey extract(Sodium sodium, SecureKey source, int begin, int end) {
  final out = SecureKey(sodium, end - begin);
  try {
    out.runUnlockedSync(
      (outData) => source.runUnlockedSync((sourceData) {
        outData.setRange(
          0, 
          outData.length,
          sourceData, 
          begin,
        );
      }),
      writable: true,
    );
    return out;
  } catch (e) {
    out.dispose();
    rethrow;
  }
}

The reason why this works is, that both sourceData and outData are simply views of the underlying raw pointers. In addition, setRange is implemented in a way that the dart vm utilizes memcpy if both lists are native data, which is the case here.

My recommendation would be for you to simply create an extension method on SecureKey with an implementation similar to this one to extract sub keys from the original one. I would be willing to merge such an extension methods, but I would like to keep the SecureKey interface as slim as possible, so it would have to be an extension.

Note: Your current implementation does indeed copy data from C to dart and back. To archive true memory efficiency, you must use the runUnlocked* methods and limit yourself to setRange (and the Uint8List.*view constructors) - anything else will lead to extra copies.

---

Secondly, regarding the generated build files - I am sorry to say, but I am strictly against it. Have a look at the CI pipeline - this is one of the many downsides of checked in generated files. I tend to treat them the same way as "compiled output" - they do not belong in a version control and their content is irrelevant, as it may change anytime, depending on the exact build dependencies used etc.

I do however understand your wish to be able to include the repository directly via git. However, you would not be able to do this with a C-library as well. I tend to do frequent releases even for minor changes, and if it is important for you, you can always request a new version via an issue (I might release it as pre-release, but unless the repository is broken, I will) - which leads to the last point. I don't give any guarantees on the state of the repository between releases. It might be completely broken, buggy, untested, etc. at any point. So using anything but the official releases is not recommended anyways.

Sorry for the long text, but I hope you understand my point of view. Again, if you refactor you split method to be an extension with the proposed copying mechanism, I would be happy to merge it.

Regards, Skycoder42

[timshadel]

I love the deep explanation on both counts. When I have some time next week I’ll try to pull this into an extension using the methods you suggest (your explanation of the memory management was exactly what I hoped to get so that I could properly understand how it worked).

As for the generated outputs, I can absolutely respect differing views, each well thought out. It’s vital that a package align with the intent and view of its creator and I’m happy to have my code do so.

I need to test locally against my own project which depends on this code. Would you recommend a specific way to do that? I’d guess just using the path: in my project’s pubspec.yaml, but while I’m quite experienced with other languages and tooling I’m still catching up on Dart.m and would love your advice.

[Skycoder42]

I am glad I could help, and I am looking forward to your contribution.

Using a path dependency is probably the only real way. The other options are hosted or git dependencies. Git won't work, because of the build files, and setting up your own pub.dev is not really something you would want to do. So, all thats left is the path. It's not optimal, but it works.

If you need to temporarily override the dependency, I suggest using the dependency-overrides for that.

[timshadel]

@Skycoder42 would you mind reviewing the new code I've pushed? I think error handling, naming conventions, and of course memory handling could all use specific review.

[Skycoder42]

The code itself looks nice, good work!

I added one comment, related to imports that should be easy to fix. In addition, please run dart format --fix lib/src/api/secure_key_extensions.dart to make sure the file is formatted correctly.

One other thing is testing: As you might have noticed, this library is fully unit and integration tested, and I would like to keep it that way. However, that is not something I require contributers to do themselves. If you want to, you can write these tests. If not, I will merge anyways and simply write them myself. If you have any questions regarding testing, feel free to ask.

[timshadel]

I've fixed the imports, and I'd actually like to put together the unit tests for it myself. I had some trouble with the melos stuff earlier because it wanted me to upgrade libraries which couldn't actually be updated. Can you tell me how you run the unit tests?

[timshadel]

@Skycoder42 ok, I've added some unit tests and fixed the data validation errors they exposed. I looked at the integration tests, and I wasn't clear on whether this change should be tested there. Because I didn't see the SecureKeyEquality in the integration testing I assumed that SecureKeySplit should also be skipped for that. What do you think?

[Skycoder42]

Your tests and changes to checking validity of the input parameters looks good so for. However, there is one check I don't understand - see the comment.

Integration tests are indeed not needed, as this method does not invoke any native libsodium APIs. My bad.

Otherwise, this looks fine to me, great work.

[Skycoder42]

Thanks for your contribution 🙂