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

Gnuton Support #186

Merged
merged 228 commits into from
Aug 12, 2024
Merged

Gnuton Support #186

merged 228 commits into from
Aug 12, 2024

Conversation

ExtremeFiretop
Copy link
Owner

Gnuton Support (Proof of Concept)

@ExtremeFiretop
Copy link
Owner Author

Ha. I finally properly learned how to update it without causing a mess.
Only took so many tests.

@ExtremeFiretop
Copy link
Owner Author

Yah, I've been doing this totally wrong the whole time which is why I've been fighting it merging the changes unexpectedly.
Oh well, at least I'll know how to update x branch after more changes were pushed to dev without causing a mess.

This should be the last PR this happens lol

@ExtremeFiretop
Copy link
Owner Author

ExtremeFiretop commented Apr 14, 2024

@Martinski4GitHub

So for now this dev is stalled. We made a patch for Gnutons web_update script. It was accepted. But that won't be in the world until the next firmware release.

Which means people will continue to have the problem when trying to run our MerlinAU script, where the Gnuton webs_update.sh is dumping everything and giving some errors.

I tried to have my one tester replace his version of the webs_update script by mounting his root filesystem as read and write. And then patching.

But unfortunately his root "/" is mounted as squashfs

/dev/root on / type squashfs (ro,relatime)

Which means this command won't work: mount -o remount,rw /

Like it does on my ubifs system.
So again we are stuck since he can't patch his system until the next firmware release.

I could try and find another user which has the file system mounted as ubifs like mine, which is why I reopened the issue on GitHub.

But I'm wondering if there's an easy way to identify if any of the Gnuton models would also be ubifs instead of squashfs. Instead of asking users at random to check with the mount command.

Any ideas? If not I can either just ask openly in the Gnuton discord and see who responds, or we can wait until the next firmware release.

@Martinski4GitHub
Copy link
Collaborator

So for now this dev is stalled. We made a patch for Gnutons web_update script. It was accepted. But that won't be in the world until the next firmware release.

Which means people will continue to have the problem when trying to run our MerlinAU script, where the Gnuton webs_update.sh is dumping everything and giving some errors.
....

... So again we are stuck since he can't patch his system until the next firmware release.
....
Any ideas?

For testing purposes, you can just mount the newly updated script file on top of the old one in the F/W:

  1. First, copy the new script version to "/jffs/scripts/webs_update_NEW.sh"

  2. Make sure to make it executable:
    chmod a+x /jffs/scripts/webs_update_NEW.sh

  3. Finally mount the modified version over the old one (include enclosing curly braces):

{
    mount | grep -q "/usr/sbin/webs_update.sh" && \
    umount /usr/sbin/webs_update.sh 2>/dev/null
    [ -x /jffs/scripts/webs_update_NEW.sh ] && \
    mount -o bind /jffs/scripts/webs_update_NEW.sh /usr/sbin/webs_update.sh
    mount | grep -q "/usr/sbin/webs_update.sh" && echo "MOUNTED OK" || echo "NOT MOUNTED"
}

Once mounted, run the script once to check that it runs OK:
sh /usr/sbin/webs_update.sh

Then run our script normally to see if it gets the right F/W version info.

@ExtremeFiretop
Copy link
Owner Author

ExtremeFiretop commented Apr 14, 2024

@Martinski4GitHub

We have made first contact!

image

One small step for a man, one giant leap for mankind!

@Martinski4GitHub
Copy link
Collaborator

@Martinski4GitHub

We have made first contact!

Is that with the updated script mounted over the old version?
It's looking good. I have not seen much feedback from the GNUton developer on GitHub.
Are you getting in contact privately? Otherwise, very little progress can be made without more collaboration.

P.S. That's a very weird screenshot!! LOL :>)

@ExtremeFiretop
Copy link
Owner Author

ExtremeFiretop commented Apr 15, 2024

@Martinski4GitHub
We have made first contact!

Is that with the updated script mounted over the old version? It's looking good. I have not seen much feedback from the GNUton developer on GitHub. Are you getting in contact privately? Otherwise, very little progress can be made without more collaboration.

P.S. That's a very weird screenshot!! LOL :>)

Yes that's the updated webs_update.sh mounted over the old and the latest Gnuton branch of MerlinAU.

I'm mostly contacting him and the users through their discord community. (Found on the Gnuton GitHub page)

Gnuton seems more willing to "chat" there, along with some Gnuton users considering that seems to be where Gnuton announces his releases, changes, progress. Etc

I've put the script in a "debug mode" with a hardcoded lower version value and asked a user to try and update but cancel when prompted at the end. Waiting to see what the log and shell output will be!

@ExtremeFiretop
Copy link
Owner Author

@Martinski4GitHub

image

image

image

image

image

@ExtremeFiretop
Copy link
Owner Author

ExtremeFiretop commented Apr 15, 2024

2 Bugs Identified so far.

  1. is that copy and paste does not update the value counter for the password entry anymore

  2. The "2: parameter not set" when running on Gnuton build randomly.

@Martinski4GitHub
Copy link
Collaborator

2 Bugs Identified so far.

1. is that copy and paste does not update the value counter for the password entry anymore

Yeah, my friend reported the problem last night and I already fixed it. I'll submit it the PR soon.

MerlinAU_PSWD_Bug

2. The **"2: parameter not set"** when running on Gnuton build randomly.

The root cause is likely to be similar to the one reported before and was fixed (ATM, I don't recall which PR it was).

@ExtremeFiretop
Copy link
Owner Author

@Martinski4GitHub

Apparently the password also needs to be masked for the test function to work? And if he holds TAB it spaces away as if you were in an editor and pressed tab? He is using CMD in Windows instead of putty, I didn't notice this behaviour in Putty but will try in CMD now.

@ExtremeFiretop
Copy link
Owner Author

@Martinski4GitHub

I see, if you copy and paste masked vs unmasked it changes the password result, therefore causing the test login to fail.

@ExtremeFiretop
Copy link
Owner Author

@Martinski4GitHub

Your latest PR resolves the counter issue and the issue with the test function failing when masked vs unmasked :)

@Martinski4GitHub
Copy link
Collaborator

@Martinski4GitHub

image

@ExtremeFiretop,

Can you ask whomever is sending you the screenshots to provide better & clearer snapshots that capture only the relevant information?

Many screenshots show the relevant info in a tiny corner and the rest (~80%) of the image is just empty space. When I tried to read the info on my iPad, I can barely read the pertinent info. Compare that to my screenshot showing the password problem. Screenshots should be clear & easy to read.

@ExtremeFiretop
Copy link
Owner Author

@Martinski4GitHub
image

@ExtremeFiretop,

Can you ask whomever is sending you the screenshots to provide better & clearer snapshots that capture only the relevant information?

Many screenshots show the relevant info in a tiny corner and the rest (~80%) of the image is just empty space. When I tried to read the info on my iPad, I can barely read the pertinent info. Compare that to my screenshot showing the password problem. Screenshots should be clear & easy to read.

Hey @Martinski4GitHub

I can ask, it is weird we get the full screenshot of his CMD window instead of just the script, but technically the entire picture is available in high res if you just click on it in Github.

On my desktop, I click the screenshot and it loads it in a new browser window in full screen and res and I can see everything fine.

@ExtremeFiretop
Copy link
Owner Author

ExtremeFiretop commented Apr 15, 2024

Many screenshots show the relevant info in a tiny corner and the rest (~80%) of the image is just empty space. When I tried to read the info on my iPad

Did you just admit to using an apple product? Idk man, idk how I feel about this ;) JK!

@Martinski4GitHub
Copy link
Collaborator

What would the built in firmware verification be checking against? I'm just curious, maybe I'm missing that due to some lack of understanding in the field.
We already discussed how these firmwares files aren't signed. So it can't be checking for signatures, and it can't be checking SH256 or MD5 for Gnuton, can it?
That would require it obtaining the SH256 or MD5 for Gnuton and Merlin

I don't know the details of ASUS implementation; AFAIK, that is a closed source. All RMerlin has stated is that some "file verification" is performed along with a router model check.
I know of a couple of ways to perform an image file check (without a digital signature). For example, during the F/W image build, fixed-size markers could be embedded at the beginning and at the end of the file to include a verification tag. This tag can be a hash value calculated based on the original image size. The markers (with the tags) are then embedded into the file without altering the actual image contents. On the receiving end, the markers are extracted from the image file and compared against the hash calculated on the image itself. If they match, the image is considered validated. Other special tags may indicate whether the image was built by ASUS (i.e. OEM stock F/W) or by RMerlin (i.e. 3rd-party sanctioned F/W build), and the intended router model. Without looking at the actual source, I cannot say exactly what type of file verification is performed.

Another "black box" situation, but i wouldn't rule it out completely.

And RMerlin can't and won't really say much about it, likely due to some confidentiality agreement with ASUS to maintain his "sanctioned build" status.

@Martinski4GitHub
Copy link
Collaborator

@ExtremeFiretop,

BTW, let me know when this Gnuton PR is fully in sync with the latest master release and ready for review so that I can grab the script file, take it to my work PC, and run it through the Linter tool. I plan to do that tomorrow night and then review the report to check the results. The tool takes about 10-12 minutes (it does 3 passes through the entire file) to do the operation and generate the report. It's the review of the results that always takes longer, depending on what it finds. There are 3 sections: Errors, Warnings & Recommended Suggestions. So I have to go through each one to make sure nothing new was found.

Anyway, just letting you know.

@ExtremeFiretop
Copy link
Owner Author

ExtremeFiretop commented Aug 9, 2024

@ExtremeFiretop,

BTW, let me know when this Gnuton PR is fully in sync with the latest master release and ready for review so that I can grab the script file, take it to my work PC, and run it through the Linter tool. I plan to do that tomorrow night and then review the report to check the results. The tool takes about 10-12 minutes (it does 3 passes through the entire file) to do the operation and generate the report. It's the review of the results that always takes longer, depending on what it finds. There are 3 sections: Errors, Warnings & Recommended Suggestions. So I have to go through each one to make sure nothing new was found.

Anyway, just letting you know.

Hey @Martinski4GitHub

This file is currently in sync, and will remain that way until the next push to the dev branch :)

Feel free to grab a copy, test, run it through the linter tool, make suggestions and like I said before, ask lots of questions.

If I can explain why I did it a specific way, great, if not, let's change it. I wanna address everything we can before the merge.

I was going to a summery report of all the functions we touched with this, and then use that as the bases for the "feature" we need to test.

@ExtremeFiretop
Copy link
Owner Author

Okay... NOW it's in sync. It was missing the modified workflow file, but that has no bearing on MerlinAU haha

@Martinski4GitHub
Copy link
Collaborator

@ExtremeFiretop,
BTW, let me know when this Gnuton PR is fully in sync with the latest master release and ready for review so that I can grab the script file, take it to my work PC, and run it through the Linter tool. I plan to do that tomorrow night and then review the report to check the results. The tool takes about 10-12 minutes (it does 3 passes through the entire file) to do the operation and generate the report. It's the review of the results that always takes longer, depending on what it finds. There are 3 sections: Errors, Warnings & Recommended Suggestions. So I have to go through each one to make sure nothing new was found.
Anyway, just letting you know.

Hey @Martinski4GitHub

This file is currently in sync, and will remain that way until the next push to the dev branch :)

Feel free to grab a copy, test, run it through the linter tool, make suggestions and like I said before, ask lots of questions.

If I can explain why I did it a specific way, great, if not, let's change it. I wanna address everything we can before the merge.

OK, good. I'll just grab a copy tonight, let it run through the Linter tool, and get the report before going to bed. Then tomorrow evening I'll start reviewing the results.

I was going to a summery report of all the functions we touched with this, and then use that as the bases for the "feature" we need to test.

Sounds good.

MerlinAU.sh Outdated Show resolved Hide resolved
MerlinAU.sh Outdated Show resolved Hide resolved
@Martinski4GitHub
Copy link
Collaborator

@ExtremeFiretop,

The latest Linter tool report shows no errors, but there are now 43 warnings (previously there were 21: 9 false positives, and 12 that can be ignored). I'm going to bed now but I'll review the warnings tomorrow evening.

FYI.

@ExtremeFiretop
Copy link
Owner Author

@ExtremeFiretop,

The latest Linter tool report shows no errors, but there are now 43 warnings (previously there were 21: 9 false positives, and 12 that can be ignored). I'm going to bed now but I'll review the warnings tomorrow evening.

FYI.

Fantastic! I'm I understanding that this means I didn't break anything drastically?
What type of mistakes are flagged as warnings?

If you let me know I can start doing my own review here 😉😁

@Martinski4GitHub
Copy link
Collaborator

Martinski4GitHub commented Aug 11, 2024

@ExtremeFiretop,
The latest Linter tool report shows no errors, but there are now 43 warnings (previously there were 21: 9 false positives, and 12 that can be ignored). I'm going to bed now but I'll review the warnings tomorrow evening.
FYI.

Fantastic! I'm I understanding that this means I didn't break anything drastically?

The "Error section" pretty much always points out syntactic errors and occasionally some semantic errors. So yeah, none of those were found to "break the script."

What type of mistakes are flagged as warnings?

These are usually syntactic expressions that may or may not be a problem per se, but they may lead to potential issues depending on the context and the intended goal of the expression, hence the warning. Also, non-compliant expressions WRT coding best practices or coding standards are flagged. It's always worth taking a look and addressing as many warnings as possible. It may contain some false positives, or issues that can be ignored because they don't apply to a particular case.

If you let me know I can start doing my own review here 😉😁

Note that I can't provide the report itself because it's got the name of the company in the headers, and the tool & its byproducts are proprietary and intended for employees only. Employees can use it freely as long as we don't violate the terms of use, or the company policies WRT conflicting interests (e.g. using it for something that directly competes with the company's products or helps the competition - there's an entire chapter about this in the company's employee handbook).

@ExtremeFiretop
Copy link
Owner Author

If you let me know I can start doing my own review here 😉😁

Note that I can't provide the report itself because it's got the name of the company in the headers, and the tool & its byproducts are proprietary and intended for employees only. Employees can use it freely as long as we don't violate the terms of use, or the company policies WRT conflicting interests (e.g. using it for something that directly competes with the company's products or helps the competition - there's an entire chapter about this in the company's employee handbook).

No worries, I just figured if I had the report in hand, I could help you tag-team it!
2 is better than one in that regard right? But I totally understand.

I'll be patient ;)

@Martinski4GitHub
Copy link
Collaborator

If you let me know I can start doing my own review here 😉😁

Note that I can't provide the report itself because it's got the name of the company in the headers, and the tool & its byproducts are proprietary and intended for employees only. Employees can use it freely as long as we don't violate the terms of use, or the company policies WRT conflicting interests (e.g. using it for something that directly competes with the company's products or helps the competition - there's an entire chapter about this in the company's employee handbook).

No worries, I just figured if I had the report in hand, I could help you tag-team it! 2 is better than one in that regard right? But I totally understand.

I'll be patient ;)

OK, I've gone through the Linter tool report and reviewed every piece of code that had been flagged with a warning. Most of them were caused by non-compliance with coding standards or with coding best practices. A handful of them was related to code that needed to be "clearer" WRT the semantics. While reviewing this code, I also made some improvements & fine-tuning.

So what I'll do is merge the current Gnuton PR "as is" into the development branch, and then I'll submit a separate PR with my changes. That way you can review all the changes that were made after the initial merge.

Copy link
Collaborator

@Martinski4GitHub Martinski4GitHub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for initial merge into the development branch.

@Martinski4GitHub Martinski4GitHub merged commit 4a30e59 into dev Aug 12, 2024
1 check passed
@ExtremeFiretop ExtremeFiretop deleted the Gnuton branch August 13, 2024 00:12
@ExtremeFiretop ExtremeFiretop mentioned this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants