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

[addons] define offical repositories #18111

Merged
merged 3 commits into from
Jul 17, 2020
Merged

[addons] define offical repositories #18111

merged 3 commits into from
Jul 17, 2020

Conversation

howie-f
Copy link
Contributor

@howie-f howie-f commented Jun 28, 2020

Description

This PR defines addon repositories (usually repository.xbmc.org) as offical.

Auto-updates of addons from third-party repositories (not official) will then be denied if the checked addon is provided by both repos.

Motivation and Context

This is part 1.) "trusted repos" of addon-system-refactoring. Numbered proposals can be found here: #17677

Types of change

  • New feature (non-breaking change which adds functionality)

Checklist:

@phunkyfish
Copy link
Contributor

I think it's ok to add a separate function to CompileInfo containing the new value. You may also need to pass it in CMakeLists.txt in the root of the repo, around line 225 (but I'm not sure on this!).

@howie-f
Copy link
Contributor Author

howie-f commented Jun 28, 2020

i removed 2d71b45 as this was a testing commit that should display addon-repos in the gui. they showed up without the comment

ADDON_REPOS repository.xbmc.org, repository.libreelec.tv # 2nd repo will be removed later (testing)

now things are getting a little tricky, but the current commit is working.. and the log-file says:
INFO <general>: ADDONS: Number of added trusted addon repositories : 2

@phunkyfish
Copy link
Contributor

You can also use StringUtils::Split to pull out the repos out of the single string.

@howie-f
Copy link
Contributor Author

howie-f commented Jun 28, 2020

You can also use StringUtils::Split to pull out the repos out of the single string.

yes, found it right now. and what i'm not sure about is the new AddonManagerTrustedRepos.h.in or if there's a different way

@phunkyfish
Copy link
Contributor

I think it's good, once the PR is ready to take out of draft someone will suggest something else if there's a better way.

So next you'll need to figure when an update happens how to restrict addon updates from a trusted repos to only come from trusted repos. So I think this will be to check the origin which should match a trusted repo.

@fuzzard
Copy link
Contributor

fuzzard commented Jun 30, 2020

You are mistaken. It does not prevent that.

xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonDatabase.cpp Outdated Show resolved Hide resolved
@phunkyfish
Copy link
Contributor

Nice work @howie-f. I think apart from the last couple of minor comments this is looking good.

Is there anything you would like to add to this before changing to a full PR? This feels to me like a full PR.

@howie-f
Copy link
Contributor Author

howie-f commented Jul 3, 2020

@phunkyfish actually reworking. i'll do a push shortly and then adjust to full pr.

@howie-f howie-f marked this pull request as ready for review July 3, 2020 10:11
@phunkyfish phunkyfish added Component: Add-ons No Jenkins do not run automatic Jenkins builds on this PR v19 Matrix and removed No Jenkins do not run automatic Jenkins builds on this PR labels Jul 3, 2020
@phunkyfish phunkyfish added this to the Matrix 19.0-alpha 1 milestone Jul 3, 2020
@howie-f
Copy link
Contributor Author

howie-f commented Jul 3, 2020

excerpt from debug log:

2020-07-03 11:50:20.608 T:1216    DEBUG <general>: ADDONS: trusted versions count : 456
2020-07-03 11:50:20.608 T:1216    DEBUG <general>: ADDONS: private versions count : 4
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = audioencoder.kodi.builtin.aac / Origin = b6a50484-93a0-4afb-a01c-8d17e059feda
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = audioencoder.kodi.builtin.wma / Origin = b6a50484-93a0-4afb-a01c-8d17e059feda
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = game.controller.default / Origin = b6a50484-93a0-4afb-a01c-8d17e059feda
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = game.controller.snes / Origin = b6a50484-93a0-4afb-a01c-8d17e059feda
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = inputstream.adaptive / Origin =
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.album.universal / Origin = repository.xbmc.org
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.artists.universal / Origin = repository.xbmc.org
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.common.allmusic.com / Origin = repository.xbmc.org
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.common.fanart.tv / Origin = repository.xbmc.org
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.common.imdb.com / Origin = repository.xbmc.org
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.common.musicbrainz.org / Origin = repository.xbmc.org
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.common.theaudiodb.com / Origin = repository.xbmc.org
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = metadata.common.themoviedb.org / Origin = repository.xbmc.org
.
.

2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = pvr.hts / Origin =
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = repository.sticky / Origin =
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = repository.xbmc.org / Origin = b6a50484-93a0-4afb-a01c-8d17e059feda
2020-07-03 11:50:20.609 T:1216    DEBUG <general>: ADDONS: update check: addonID = resource.images.weatherfanart.single / Origin = repository.xbmc.org

xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonDatabase.cpp Outdated Show resolved Hide resolved
yol
yol previously requested changes Jul 4, 2020
xbmc/CompileInfo.cpp.in Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.cpp Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.h Outdated Show resolved Hide resolved
xbmc/addons/AddonManager.h Outdated Show resolved Hide resolved
xbmc/CompileInfo.h Outdated Show resolved Hide resolved
@yol yol dismissed their stale review July 5, 2020 06:35

changes performed

@phunkyfish
Copy link
Contributor

phunkyfish commented Jul 11, 2020

  1. I think we need to solve that. Do these addons have the system origin?

for addons we ship (and haven't been updated) it uses some kind of hash as the origin

That’s the ORIGIN_SYSTEM hash. Ok, can add that.

  1. If you remove it from kodi repo you would need to have a higher version in the non kodi repo for that update to then happen.

hmm... are you sure? is there any magic code in this PR that removes (or ignores) the repository.xbmc.org origin if the addon is no longer available in our repo?

Ok, that’s true. I think in this case you would need to manually update the addon from another repo. I think we can add try and add this now or in a future PR. As there will be work on addons installing from there installed repo this may end up only being possible manually.

@howie-f
Copy link
Contributor Author

howie-f commented Jul 11, 2020

That’s the ORIGIN_SYSTEM hash. Ok, can add that.

then i'd come up now with this 06efd7e (hopefully doing what we want)

I think we can add try and add this now or in a future PR

let's do that in a future PR and when we have a possible solution for ronie's point 5.

@matthuisman
Copy link
Contributor

matthuisman commented Jul 12, 2020

regards to point #5, as kodi repo is pre-installed, it shouldn't be able to be hijacked

Its origin could be set to itself (or ORIGIN_SYSTEM - maybe already is?), so it shouldn't accept updates / installs from other repos.

So, only way it could be replaced is via install from zip.
You could simply block install from zip of ids that are in trusted repos.

Or just not allow repos to be installed via zip if they are already installed.
A repo should update itself anyway - not update via install from zip.

Also, a REPO should not be allowed to be updated from a different repo fullstop.
It should only be able to update from it's origin only.

In my original PR, I suggested that

"UNLESS the add-on is a system addon (origin is ORIGIN_SYSTEM) then it can get an update from any trusted repos (highest version used)."

Which looks like the latest commit does.

@howie-f
Copy link
Contributor Author

howie-f commented Jul 15, 2020

i finally dropped the refactoring commit, as it was obsolete now, and tested with a private skin.estuarywhich didn't auto-update if we're set to trusted only. setting is at level 1. so this seems ready to me now once all ticks are green.

@phunkyfish
Copy link
Contributor

@howie-f did you address point 5 raised by ronie?

5 - from a technical viewpoint, this change doesn't define our repository (http://mirrors.kodi.tv/) as trusted at all.
what it does is, it defines an addon id (repository.xbmc.org) as trusted.
while this may sound futile, it is a big difference to anyone with malicious intent.
it would take literately less than 5 seconds to craft an addon that hi-jacks the repository.xbmc.org addon id, while it would be (near) impossible to take over the mirrors.kodi.tv domain.

@howie-f
Copy link
Contributor Author

howie-f commented Jul 15, 2020

@phunkyfish that is unaddressed for now. wrote an idea about how to maybe solve this to the channel, but we have to decide if this is a appropiate. so i saw this as out of scope but to come.

@phunkyfish
Copy link
Contributor

Ah, I hadn’t looked at slack yet today 😉

@howie-f howie-f changed the title Define trusted addon repositories [addons] define trusted repositories Jul 16, 2020
Copy link
Contributor

@phunkyfish phunkyfish left a comment

Choose a reason for hiding this comment

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

LGTM

@phunkyfish phunkyfish changed the title [addons] define trusted repositories [addons] define offical repositories Jul 17, 2020
@DaveTBlake DaveTBlake merged commit 7b97a46 into xbmc:master Jul 17, 2020
@howie-f howie-f deleted the v19-trusted-repos branch July 17, 2020 16:45
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 8, 2020
@dagwieers
Copy link
Contributor

This will make it harder for official add-on developers to get an emergency fix out when e.g. a VOD provider made infrastructure changes. Official add-ons will now fully depend on the time it takes for an update to get reviewed, accepted and pushed through the normal channels (no possibility to fast-track through their own repository).

Non-official add-ons (like Netflix) will have the advantage they can publish asap through their own repository.

Maybe a mechanism where packages are signed, and signatures are trusted would have been a more flexible solution here, so the user can decide what to trust in addition to the official repositories?

@phunkyfish
Copy link
Contributor

Signing and signatures would be ideal. But out of scope for the initial feature set, it would have been far too much work to get this in place quickly.

Once the initial feature set is done we can look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants