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

[WIP] Hash Checking #94

Closed
wants to merge 1 commit into from
Closed

[WIP] Hash Checking #94

wants to merge 1 commit into from

Conversation

hammy275
Copy link

@hammy275 hammy275 commented Nov 4, 2021

This commit adds to the USB Image Writer (mintstick -m iso) the ability to check the SHA256 of an opened file, as requested in #84 . This is my first contribution to Linux Mint as a whole, so any and all feedback is greatly appreciated! I'm sure it isn't perfect in its current state, though I'm absolutely willing to learn to improve this contribution! 🙂

@clefebvre
Copy link
Member

Thanks @hammy3502 for contributing and welcome to the development side of things :)

I like your idea to add sha256 sum checking to this tool but I don't like how you did it.

The empty field, the button and the icon are a bit confusing in my opinion. Some ISOs don't even have a sum to check against.. I would suggest to simply just calculate the sum without asking anything and display it there instead.

@clefebvre
Copy link
Member

It would need to be done async, with a spinner spinning while it's calculating.

@clefebvre clefebvre changed the title Hash Checking [WIP] Hash Checking Nov 24, 2021
@hammy275
Copy link
Author

Sounds good, @clefebvre !

I'll have something like that ready, but do you have an ideal placement you'd want for the text in the window (the same spot it's in now, next to the cancel button, inside the details area, etc.)?

Thank you so much! :)

@hammy275
Copy link
Author

I moved the hash checking to an expander and made it so it automatically calculates when an ISO is selected, however I'm currently running into an issue where when attempting to exit, Python will wait until the hashing operation is done before quitting out. There isn't an easy way to check mid hash-calculation if we should exit, and multiprocessing runs in a separate process, so it can't access GLib to do an idle_add to get back to our main window. Is there an ideal way you'd want this resolved (disable exiting while waiting for hash to calculate, leave it as-is, use a hack to get around it, or something else)?

@haggen88
Copy link

haggen88 commented Nov 25, 2021

@hammy3502 @clefebvre how about something like that?

1

Simply that the positive check is shown in green, but that it is not an exclusionary requirement for burn the iso image [in the case of isos without sum].

@JosephMcc
Copy link

I haven't followed this but I'd suggest staying away from the colored text. That sort of thing can lead to readability issues when people use different themes.

@haggen88
Copy link

Considering what @JosephMcc says it is better to show something more transversal like a message ... "Checksums match" / "Hash matched".

@hammy275
Copy link
Author

hammy275 commented Nov 29, 2021

@haggen88 I personally really like the idea you have (and I originally had something somewhat similar implemented), though based on what I interpreted from @clefebvre , I've changed it to just display the hash inside of an expander (similar to the "Details" expander). Maybe there could be a compromise and I can add a field to check the hash inside the expander, so people only interact with SHA256-related things when they want to.

Would love to here people's thoughts on the compromise mentioned above.

@haggen88
Copy link

haggen88 commented Dec 7, 2021

@hammy3502 could you show a picture of what you have so far please?

@hammy275
Copy link
Author

hammy275 commented Dec 7, 2021

@haggen88 Here's what it looks like before the feedback from @clefebvre
image
and here's what it looks like after:
image

As you can probably tell, the "after" version isn't entirely polished just yet, but that's just because I'm waiting on feedback before making any further changes.

@haggen88
Copy link

haggen88 commented Dec 7, 2021

@hammy3502 my opinion:

  • The first picture is useful when you download the iso from a website that provides the SHA-256 and you with 'USB Image Writer' want to check the match.
  • The second picture automatically calculates the SHA-256 but does not compare the hash with anything and the check is left open to what the user can do/read.

I understand Clem's approach to use 'USB Image Writer' with isos without checksum, but my idea was to be able to integrate the Hash comparison, but that this function is not mandatory to burn an iso [or maybe it would be more comfortable to integrate it as another mintstick mode ??].

In my opinion the second option is missing an option like the one highlighted in this image:

checksum utility

Best regards

@hammy275
Copy link
Author

hammy275 commented Dec 7, 2021

@haggen88 I totally agree with you there, hence my interest in hearing feedback from both you and from Clem on the compromise I mentioned above.*

Maybe there could be a compromise and I can add a field to check the hash inside the expander, so people only interact with SHA256-related things when they want to.

To clarify a bit more, the field inside the expander would basically just be the field and button from the first screenshot.

Cheers for the response! :)

*EDIT: My sincerest apologies if this sentence came off as condescending or something similar, that wasn't my intention at all.

@haggen88
Copy link

haggen88 commented Dec 7, 2021

@hammy3502

*EDIT: My sincerest apologies if this sentence came off as condescending or something similar, that wasn't my intention at all.

No problem, I did not perceive it as such.

To clarify a bit more, the field inside the expander would basically be the field and button in the first screenshot.

Shouldn't the field say "Paste SHA-256" instead of 'Check SHA-256'? [is a more enlightening text].

Regards

@hammy275
Copy link
Author

hammy275 commented Dec 7, 2021

@haggen88 I know you sent it a couple times in the screenshot, but I don't see much of a need for a paste button (after all, CTRL+V and Right click --> Paste both exist). The Check SHA256 button is what starts the calculation to calculate the hash before we match it with whatever the use put into the box.

Maybe instead of a button we could add some alt text (I think that's what it's called, but I mean the gray text that you put in an input field that isn't actually there, but just provides a "hint") that says to CTRL+V or Right click --> Paste the hash. If you feel there's some advantage to having a paste button though, I'd love to hear about why that is!

@haggen88
Copy link

haggen88 commented Dec 7, 2021

@hammy3502 I don't mean to add a button, but to change the text of A.

QQQQQQ

@hammy275
Copy link
Author

hammy275 commented Dec 7, 2021

@haggen88 I gotcha. It definitely would make sense to change that text, and I'll definitely do so if we end up going down that route (still waiting on feedback from Clem)

@hammy275
Copy link
Author

My apologies for the tag, but I can't really proceed on this issue without feedback from you @clefebvre (since you seem to be the maintainer who's looking at this issue) . More specifically, I mentioned in #94 (comment) that we could put the SHA256 section in an expander instead of just calculating the sum and displaying it (as then, the user would need to compare all the characters of the hash to any hash they may already have). This way, users who have no need for hash checking can effectively ignore it.

If you'd want to proceed with just displaying the hash text still though, I can finish that up.

@haggen88
Copy link

haggen88 commented Jun 6, 2022

ping @clefebvre

@clefebvre
Copy link
Member

@hammy3502 sorry for the delay.

If you right-click an .iso file in Nemo, you can choose Check SHA256. This feature is an action placed in /usr/share/nemo/actions/mint-sha256sum.nemo_action and provided by mintsystem through a command called mint-sha256sum.

It produces a small dialog window with the hash inside of it. I think we should improve this instead and simply link to it from mintstick.

In mintstick, we can just add a third button at the bottom to "Check SHA256 sum". Insensitive by default unless/until a file is selected (like the Write button). Clicking on it just calls mint-sha256sum <path_to_the_iso>.

In mintsystem, we can really improve the feature. Here we've got an entire window or dialog to play with. Beside just letting the user to paste a sum and compare it, I'd love it if it could also do authenticity checks, either from a pasted signature, or just simply from the ones we already trust.

@hammy275
Copy link
Author

hammy275 commented Jun 22, 2022

That sounds great to me, and I'd be happy to do the work for both mintstick and the hash work in mintsystem (or do my best to at least, I'm not very familiar with shell scripting to be honest)!

Would we want to keep using zenity in mintsystem, or switch to something like Python like minstick itself uses? I'm not sure if zenity supports translations, and we'd probably have to prompt the user to enter the hash and the signature.

@hammy275
Copy link
Author

hammy275 commented Jun 22, 2022

Changes should be done to mintstick. I'll open an issue in mintsystem to expand on the functionality of mint-sha256sum. Until then, please respond to the above comment when you get the chance, and if you have any feedback on the commit, please let me know! :)

@clefebvre
Copy link
Member

OK, this is finally done :)

mintsystem's sha256sum calculator is removed.
mintstick gains a new verification tool.
This tool is added to the USB stick formatter.

image

@clefebvre clefebvre closed this Sep 26, 2022
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.

4 participants