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

adguardhome: Bump to 0.107.55 #25521

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ra2-IFV
Copy link
Contributor

@Ra2-IFV Ra2-IFV commented Dec 9, 2024

Bump version to 0.107.54, change log in links below.

Use source from upstream tag tarballs instead of Git repo. Move working directory from /var/adguardhome to
/var/lib/adguardhome, according to Linux FHS.
Add option to store PID file, defaulting to /run/adguardhome.pid.

Link: https://github.com/AdguardTeam/AdGuardHome/milestone/89?closed=1

Maintainer: @dobo90 @1715173329

Compile tested: working on it
Run tested: not yet
Description:

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Dec 9, 2024

          Please dont do this. We can not download tarball named like this.

Originally posted by @BKPepe in #25450 (comment)

PKG_SOURCE_VERSION:=v$(PKG_VERSION)
PKG_SOURCE_URL:=https://github.com/AdguardTeam/AdGuardHome
PKG_MIRROR_HASH:=d74702bc4f8b82bda64a0a937a98e73ee602c21b9361c0c683671212e03e9316
PKG_SOURCE:=v$(PKG_VERSION).tar.gz
Copy link
Member

Choose a reason for hiding this comment

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

No, please check how other packages have done for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels so nasty when we have a tarball for this but you just cant use it because of download.pl
And it's even worse when the git script is doing a full clone from the repository, it's almost 3x larger than the tarball from tags

Copy link
Member

Choose a reason for hiding this comment

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

Does the size matter on the computer, which you are using for the buildsystem to evenutally to cross-compile packages for your devices? :-) I did not verify the size so far of your proposed solution, which says use upstream tag tarballs without the reason, why instead of the Git repo.

However, using Git sources is not preferred at least from my point of view. Its a big security risk, because if someone tinkers with the tarball, checksum is not verified anyhow. See: https://lists.openwrt.org/pipermail/openwrt-devel/2024-April/042521.html

In your case, you might want to use codeload, though. If we are talking about using codeload or your proposed solution, dont forget, that the tarball is created by GitHub itself, which means that there isnt checksum to verify (see checksums.txt file) in their release. Sometimes autogenerated tarballs are missing some things, which are required to have to build the package and the tarball from GitHub can be changed anyhow and then the checksum is not valid (I havent experienced this case, yet, but others said yes...)

So, I dont find anything wrong to keep using Git sources, i still dont know the reason, why you did it. 🤷

We are not living in the ideal world.

Copy link

Choose a reason for hiding this comment

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

Please do not repeat the same mistakes with libxz issue

And it's even worse when the git script is doing a full clone from the repository, it's almost 3x larger than the tarball from tags

also, why is the git script not doing shallow clones? does it pull the entire commit history?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincejv May I ask what are you talking about, the xz backdoor incident? I can't understand why you are mentioning that here
And yes, in my case the script is doing a full clone.

Cloning into 'adguardhome-0.107.54'...
remote: Enumerating objects: 38166, done.
remote: Counting objects: 100% (76/76), done.
remote: Compressing objects: 100% (49/49), done.
Receiving objects:   1% (514/38166), 204.00 KiB | 15.00 KiB/s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BKPepe because the script is doing a full clone, and for some reason my git keeps failing if trying to clone a very large repository, so I want to use tarballs instead.
Please forgive me for being so dumb, I still can't understand this... Even though the tarball is generated by Github, it's from a git tag, and in most cases it shouldn't change
We also have PKG_HASH to check malicious tarballs in case someone tries to do something nasty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ra2-IFV exactly what you are trying to do, getting the code from the tarballs instead of the repository and the tarball contained tainted and malicious backdoor code, which was NOT present at all in the repository, no one was able to check as reviewing source code from tarballs is not part of the code review process.

so the attacker uploaded a tarball by himself, and it's different from the repository?

it was assumed the tarball = reviewed source code, but it was not, the maintainer injected code and blobs before signing and uploading the tarballs

See: https://lists.openwrt.org/pipermail/openwrt-devel/2024-April/042521.html\

But from the wikipedia and other sources, Jia Tan gained trust from other developers and seems the malicious codes are already in the repository, he just signed off a new release..

Jia Tan gained the position of co-maintainer of XZ Utils and was able to sign off on version 5.6.0, which introduced the backdoor,

Copy link
Member

Choose a reason for hiding this comment

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

Well let's back to the topic. I believe @BKPepe was talking about the filename at first.
Your PR will create a unidentifiable tarball with just some random numbers and it could lead potential conflict issue.

So again, please check other packages for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I noticed that!
And the solution is PKG_SOURCE_URL:=https://codeload.github.com/vim/vim/tar.gz/v$(PKG_VERSION)?
Just copy pasted and replaced from xray package, so again thank you so much my dear teacher 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh sorry, that's for another pull request but almost the same.

@Ra2-IFV Ra2-IFV force-pushed the feat_adguardhome_update-0.107.54 branch from 3322bf8 to e150e82 Compare December 22, 2024 13:20
@Ra2-IFV Ra2-IFV changed the title adguardhome: Bump to 0.107.54 adguardhome: Bump to 0.107.55 Dec 22, 2024
@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Dec 22, 2024

Should I change initrc files in a seprate commit?

@1715173329
Copy link
Member

Should I change initrc files in a seprate commit?

Yes

Bump version to 0.107.55, change log in links below.

Use tarballs from upstream tags instead of a Git repo.

Link: https://github.com/AdguardTeam/AdGuardHome/milestone/90?closed=1
Signed-off-by: Ryan Keane <[email protected]>
@Ra2-IFV Ra2-IFV force-pushed the feat_adguardhome_update-0.107.54 branch from e150e82 to f42465a Compare December 22, 2024 14:13
Move working directory from `/var/adguardhome` to
`/var/lib/adguardhome`, according to Linux FHS.
Add option to store PID file, defaulting to `/run/adguardhome.pid`.

Signed-off-by: Ryan Keane <[email protected]>
@gl-dude
Copy link

gl-dude commented Jan 3, 2025

Has this PR or #25639 been confirmed to fix #25600?

Even though the AdGuard Home package hasn't been changed in months, it recently started to be compiled without the assets for the admin panel embedded into the executable.

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jan 3, 2025 via email

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.

5 participants