search

rust-secure-code / cargo-auditable

Make production Rust binaries auditable
Apache License 2.0
628 stars 26 forks source link

add commit hashes to git sources #123

Closed Adhalianna closed 1 year ago

Adhalianna commented 1 year ago

This would close the issue #122 . The suggested approach to serialization of git sources might seem over-engineered but my hope while working on it was to keep it all backwards compatible and to establish a good way of adding more of similar source parameters if needed.

At this very moment I am somewhat sleep deprived so I will probably get back here later to try and explain more if needed.

Shnatsel commented 1 year ago

Thank you! I will be traveling for the next three days, so I will review this on the 27th. Is that OK?

Adhalianna commented 1 year ago

I have fixed one thing that slipped from my previous commit. I worry however that this might not be enough to pass all the checks run by GithHub. I have previously on my own machine noticed that the test runs seemed to be nondeterministic (failure at cargo clean) when I was starting with them. I can no longer reproduce that now that I have found some time to sit at it again.

Shnatsel commented 1 year ago

Thank you for the PR! I will take a more thorough look in the next few days. Do you have a deadline for this PR to be merged?

Adhalianna commented 1 year ago

The deadline would be actually tomorrow but I can ask for extension. I am pretty sure I would get one but I don't know how much of an extension it would be.

On Tue, 27 Jun 2023, 17:10 Shnatsel, @.***> wrote:

Thank you for the PR! I will take a more thorough look in the next few days. Do you have a deadline for this PR to be merged?

— Reply to this email directly, view it on GitHub https://github.com/rust-secure-code/cargo-auditable/pull/123#issuecomment-1609705950, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMP4ZPZIYNIJ4FB3TK6DHQTXNLZXXANCNFSM6AAAAAAZRBKWQU . You are receiving this because you authored the thread.Message ID: @.***>

Shnatsel commented 1 year ago

I see. The timing of my trip was really inconvenient. I'll get this reviewed and merged later tonight then.

Adhalianna commented 1 year ago

Don't worry. If I miss it a day or two it won't be a big issue. Since with the kind of project I am doing the professor has nothing to review himself he rather won't mind.

On Tue, 27 Jun 2023, 18:22 Shnatsel, @.***> wrote:

I see. The timing of my trip was really inconvenient. I'll get this reviewed and merged later tonight then.

— Reply to this email directly, view it on GitHub https://github.com/rust-secure-code/cargo-auditable/pull/123#issuecomment-1609847868, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMP4ZP3OXJXGYUFZJVFXYMLXNMCFLANCNFSM6AAAAAAZRBKWQU . You are receiving this because you authored the thread.Message ID: @.***>

Shnatsel commented 1 year ago

I might investigate later whether I want to keep around the untagged enum magic. If there is a way to alter the format to keep the deserialization simple and stupid, it might be worth changing the spec to accommodate that. But this is a nice, general, united-tested solution within the confines of the current spec.

Great work, thank you!

Adhalianna commented 1 year ago

Thank you!

I was honestly quite ready to just put a git_rev field directly into the Package if this was decided to be too complex. It was however sticking my eyes hard enough that I decided to try something else first. I imagine it could be desirable to be able to extend e.g. Local case in the future, as the dependencies are all available while building with cargo-auditable the tool could try to include some other metadata, one that is not stored in cargo's lockfile. This made me wish for a solution that can be hierarchical and thanks to that still readable.

I'll subscribe to issues and watch out if there are any caused by the changes for at least this summer.

On Wed, 28 Jun 2023, 22:07 Shnatsel, @.***> wrote:

I might investigate later whether I want to keep around the untagged enum magic. If there is a way to alter the format to keep the deserialization simple and stupid, it might be worth changing the spec to accommodate that. But this is a nice, general, united-tested solution within the confines of the current spec.

Great work, thank you!

— Reply to this email directly, view it on GitHub https://github.com/rust-secure-code/cargo-auditable/pull/123#issuecomment-1612032262, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMP4ZP3HUZBJCJFZUDP5BUTXNSFITANCNFSM6AAAAAAZRBKWQU . You are receiving this because you authored the thread.Message ID: @.***>