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

Remove byteorder dependency #81

Closed
wants to merge 9 commits into from

Conversation

psychon
Copy link
Contributor

@psychon psychon commented May 2, 2024

Hi,

I was asked to rebase zip-rs/zip-old#390 against this repository. Here is the result.

What follows is the description of my original PR to what is now called zip-old (well, I removed some outdated parts).

Note that CI will still fail, because:

error[E0433]: failed to resolve: use of undeclared crate or module `bzip2`
Error:   --> src/read.rs:31:5
   |
31 | use bzip2::read::BzDecoder;
   |     ^^^^^ use of undeclared crate or module `bzip2`

I'll leave that one to you. I think I did not cause this.


Hi,

I came across the use of "byteorder" in this crate and thought "one does not really need byteorder for this". Feel free to disagree and to just close this PR.

Since there are many uses of "byteorder", I tried to do this in small parts and do a commit for each one. Hopefully this makes it easier to understand what is going on. Dunno.

Converting an u16 to bytes can easily be done with just the standard
library. This does not need the byteorder crate.

Signed-off-by: Uli Schlachter <[email protected]>
Reading u8, u16, u32, and u64 can easily be done with what the standard
library provides and does not need the byteorder crate.

Signed-off-by: Uli Schlachter <[email protected]>
Similar to the previous commit: What byteorder provides here can easily
be done with the standard library.

This moves the helper functions that were added in the previous commit
to "spec" (since "read" already imports "spec", but not the other way)
and converts the code to use these helpers instead of byteorder.

Signed-off-by: Uli Schlachter <[email protected]>
One really does not need the byteorder crate to write some bytes of
data. It can easily be done with methods from the std library.

Signed-off-by: Uli Schlachter <[email protected]>
We do not need the "byteorder" crate for writing little endian numbers
to a Write, but can do that just with methods from std.

Signed-off-by: Uli Schlachter <[email protected]>
The previous commits replaces the use of this crate and it is now unused
and can be removed.

Signed-off-by: Uli Schlachter <[email protected]>
roundtrip() takes a &mut, but only uses this argument non-mutably.

Signed-off-by: Uli Schlachter <[email protected]>
@Pr0methean
Copy link
Member

Please sign your commit.

@psychon
Copy link
Contributor Author

psychon commented May 2, 2024

What do you mean? As far as I can tell, all of them have a Signed-off-by line.

@psychon
Copy link
Contributor Author

psychon commented May 2, 2024

Uhm... why? What's the point?

I don't think I want to jump through your hoops. And so far I fail to see the point.

@Pr0methean
Copy link
Member

The point is so that GitHub can assure users that commits were actually written by the person they say they were written by. Git lets you put anyone's name and email on a commit. Following the xz backdoor, I expect infosec managers to demand it on all libraries as widely used as this one.

@psychon
Copy link
Contributor Author

psychon commented May 2, 2024

So how exactly does signed commits prevent Jia Tan to create a github account and a GPG key in their name?

The point is so that GitHub can assure users that commits were actually written by the person they say they were written by.

What exactly does "person" here refer to? To you, I am just a random person on the internet with a github account. If I now create a GPG key for "Socket puppet" and add it to my GitHub account, then I am still a random person on the internet. Just with a github account and a GPG key instead of just a github account.

I expect infosec managers to demand it on all libraries as widely used as this one.

No idea what an infosec manager is. And Google doesn't know either.

When I meet an infosec manager, I'll ask them what the point to this security theatre is.

@psychon psychon closed this May 2, 2024
@Pr0methean
Copy link
Member

So how exactly does signed commits prevent Jia Tan to create a github account and a GPG key in their name?

You have to open a confirmation email to show that you control the address on the GPG key, if you haven't done so already to use other account features. It also makes you refresh your 2FA, so that compromising the GitHub account wouldn't be a trivial workaround.

@psychon
Copy link
Contributor Author

psychon commented May 2, 2024

So... at that point, GitHub knows that I have a mail address. GitHub already knew that before, because I managed to register. Sure, that mail address is not public and now it would be public knowledge that GitHub verified my mail address.

Since you brought up XZ: Jia Tan got maintainer access by helping via mail off-list. So... somehow they managed to overcome an even bigger hurdle than "receive a mail once". And still that didn't stop them.

It also makes you refresh your 2FA,

Okay, I clicked your links. None of them mention anything about 2FA. So I guess nothing in there actually forces me to setup 2FA.

Edit: One more data point: When I open the GitHub web interface and press "edit" to create a change via the web interface, the resulting commit is signed and shows up as verified. And that without any 2FA. This is just security theatre.

Sadly, I cannot use that hack to open a PR since GitHub always uses zip-old as the base commit.

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.

2 participants