Nice work!

Looks promising! I too would be interested in saving databases. Thanks for the addition!

Since I made quite a mess of the git history in merging all the outstanding PRs, I have made a rebase of your changes onto the current master branch. You can find it at kdbx4-saving.

@sseemayer don't bother rebasing this PR. My plan is split this PR into smaller patches that are more focused and easier to review. For example, I'm going to create a PR that only extracts the constant values that are going to be used both when reading and writing. I also think that we're going to need to fence the database saving feature behind a feature flag. The reason is that writing a database will result in data loss until we support all the kdbx4 fields.

Since I made quite a mess of the git history in merging all the outstanding PRs

I think we should enable branch protection on the master branch. We have many CI checks that should pass before any commit makes its way to the master branch, I don't see any reason not to leverage those CI jobs for every change to the codebase. What do you think?

Sounds good! I have added something for the master branch.

This PR is ready for reviews. The Database::save function is only available from inside the library, using pub(crate). This will give us more time to stabilize the new code and finish the XML parsing and dumping functions. As of right now, using the Database::save function will result in data loss because we do not parse and dump all the XML elements in a database.

I added a bunch of unit tests for many of the ciphers and kdfs combinations, but not all the combinations are covered. After this PR is merged, I will open a PR to switch to matrix testing so that we cover all the combinations. This will also reduce duplicated code in the tests.

Being able to parse and dump databases will be very helpful in the future to add more granular unit tests, because we won't have to rely on "magic" databases, committed to the repo as fixtures. It will still be useful to parse actual databases from other apps as integration tests, but at least that won't be our only option for testing.

I added a bunch of TODOs in the XML parsing functions for all the fields we are not parsing right now. Those fields were seen in a KeePassXC database. I think KeePassXC supports all the fields in the kdbx4 format, but I'm not 100% sure about that.

@droidmonkey I'd like to have your opinion on the default HMAC block size. The block size (along with most of the hmac module) was inspired by the KeePassXC codebase. I'm honestly not sure if that value makes sense.

@DavidVentura I'd like to have your input on this PR, if you get a chance. The feature won't be available yet externally, so it won't be available to implement in pykeepass-rs right now, but I think we're pretty close to having the feature available at least behind a cargo feature flag.

Thank you for the fantastic work, it looks great!

I like the idea of getting things merged with a visibility of pub(crate) for now until we can do some more cleanups.

Hi!
I don't think I'll be able to review at least this week -- only think I'm able to contribute with is a similar effort I went through before:

Add write support, #1

I'm not sure how helpful it'll be for you, I wasn't really sure what I was doing at that time, but it did seem to work!

@DavidVentura Thanks for linking the PR. I had a look at it, and there's a few things in there that I think we could port over here, notably all the Group and Entry fields that you implemented. We'll have to implement all of those and all the remaining Database/Group/Entry fields before making the new Database::save function.

@droidmonkey I'd like to have your opinion on the default HMAC block size. The block size (along with most of the hmac module) was inspired by the KeePassXC codebase. I'm honestly not sure if that value makes sense.

@louib
I am personally not a fan of the data streams in the KeePassXC codebase, they are from KeePassX era. The block size is not relevant because it is embedded as part of the stream (HASH + BLOCKSIZE + DATA) so when you read the block it reads the size that was actually stored. So... the block size is really just for chunking the data out for efficient reads, 1 MiB seems reasonable to me 😄

@droidmonkey thanks for the review! Let's go with that default for now then.