search

lineageos4microg / docker-lineage-cicd

Docker microservice for LineageOS Continuous Integration and Continous Deployment
https://hub.docker.com/r/lineageos4microg/docker-lineage-cicd
GNU General Public License v3.0
480 stars 189 forks source link

Remove "old" signature spoofing patches for LineageOS 21, and introduce patch which enables it even in the user build variant #608

Closed nnsee closed 3 months ago

nnsee commented 3 months ago

As discussed in https://github.com/lineageos4microg/docker-lineage-cicd/issues/607, this PR removes the signature spoofing patches entirely for lineage-21.0 (Android 14), since 21 has signature spoofing functionality built in for eng and userdebug builds (even with SIGNATURE_SPOOFING set to no).

Instead, this PR replaces the standard patch with one which enables signature spoofing even for user builds if SIGNATURE_SPOOFING is set to either restricted or yes (both do the same thing).

Since the built-in signature spoofing checks whether the app is microG specifically, it is no longer possible to allow signature spoofing to other third party apps. If it turns out that this is a use case which is required, the "old" signature spoofing patches need to be fixed in order to build on LineageOS 21 and reintroduced (alongside this patch, so users have the option to use both if desired).

nnsee commented 3 months ago

I think it would make most sense to add a separate env var for disabling the debug build check, as it is very distinct from the "old" patches.

This makes sense, will change.

petefoth commented 3 months ago

The current logic, for all build versions from 18.1 onwards, is that:

  1. The LOS changes are applied, in userdebug builds only
  2. if SIGNATURE_SPOOFING is set to no - the default case - then only the LOS changes are applied, none of our patches
  3. if SIGNATURE_SPOOFING is set to restricted, then a modified version of our frameworks_base_patch is applied, allowing only privileged apps to spoof signature
  4. if SIGNATURE_SPOOFING is set to yes, then both our frameworks_base_patch and our modules_permission_patch are applied allowing any app to spoof signature

Our current build process for official (userdebug) builds now has SIGNATURE_SPOOFING is set to no, effectively allowing restricted spoofing using only the LOS changes and not using our own patches. I would like that logic to continue to work.

My preference would be to only apply the new patch if BUILD_TYPE is not userdebug: the patch is only required in this case

petefoth commented 3 months ago

My preference would be to only apply the new patch if BUILD_TYPE is not userdebug: the patch is only required in this case

A minimal level of testing would therefore include running a 21.0 build with SIGNATURE_SPOOFING set to no, and checking the new patch is not applied.

@nnsee As a matter of interest, have you made changes to the lemonadep sources allowing 21.0 builds to succeed? As far as I know all 21.0 builds (except for nio) are broken

nnsee commented 3 months ago

That's more or less in line with what I'm currently writing, although instead of heuristically looking at whether the build variant is userdebug or not, I've simply introduced a new env var USER_BUILD_SPOOFING which, when set to "yes", applies my patch (by default, I've left it as "no"). Is this okay, or would you rather disregard the env var and that the patch is always applied if the build variant isn't userdebug or eng?

I'm guessing that they consider signature spoofing a "security issue" and this is why they only enable it in "debuggable" builds, and I'd like to leave the choice to entirely disable it in user builds to the user if they wish to do so.

I literally just built 21 for lemonadep from the master branch and it worked fine with just the changes in this PR applied, no other changes.

petefoth commented 3 months ago

I've simply introduced a new env var USER_BUILD_SPOOFING which, when set to "yes", applies my patch (by default, I've left it as "no"). Is this okay,

That works (I think 😄 ). I guess the logic needs to be dependent on branch: your patch won't work for builds before 18.1 as the LOS code you're patching doesn't exist in earlier branches. So we probably need a check in this switch statement: if USER_BUILD_SPOOFING is set true for earlier builds then write a suitable message to the log ("Branch $branch does not support USER_BUILD_SPOOFING") and EITHER set USER_BUILD_SPOOFING to false and continue OR exit

I literally just built 21 for lemonadep from the master branch and it worked fine with just the changes in this PR applied, no other changes.

I would love to know which other devices will actually build for 21.0: according to hudson all devices are disabled

nnsee commented 3 months ago

I've:

Anything I should change?

As an aside, I noticed that the build.sh script seems to be turning rather... spaghetty-ish. I'd probably introduce a future PR to try and organize it a bit and reduce the amount of nesting going on.

petefoth commented 3 months ago

I'm not going to merge this directly to master. I've created a branch (nns-v21-spoofing-patches) that we can use for testing, and changed this PR. I'll create a new PR to merge to master when we're happy the changes are good 😄

petefoth commented 3 months ago

OK a docker image based on this branch is now available at DockerHub for testing docker pull lineageos4microg/docker-lineage-cicd:nns-v21-spoofing-patches

petefoth commented 2 months ago

As an aside, I noticed that the build.sh script seems to be turning rather... spaghetty-ish. I'd probably introduce a future PR to try and organize it a bit and reduce the amount of nesting going on.

I take your point :) A lot of the nesting was introduced in implementing "Make the operations performed by the docker image 'switchable'" #508 : almost by definition, optional operations need to be inside a conditional statement, and the script loops over every branch and every device specified in the docker run command, which makes two more levels of nesting unavoidable.

If we were going to simplify and restructure the code, then I would rewrite the whole script in python, rather than continuing with shell scripts. BUT ,as stated in the README.md

The project is currently in a fairly stable state:

  • we are (mostly) achieving our objective of delivering monthly builds the only essential work that is ongoing is to
    • monitor the delivery process, to fix any problems that may occur, and to make any changes that are needed to ensure that the problems do not recur
    • to make any changes needed when upstreams make changes. In particular, when LineageOS introduces support for a new Android version and / or drops support for older Android versions

The project is therefore - in the opinion of the currently active maintainers - essentially 'feature complete' and in 'maintenance' mode. The only change that we believe might significantly improve the project is to support other classes of Android devices,

My opinion is that restructuring and improving the current code is not worth the effort that would be involved in designing, implementing, testing, and documenting the necessary changes. There was an attempt in 2022 to simplify and improve the functionality of init.sh. Work on the issue went o for over three months, and was eventually abandoned.

Don't let that stop you from trying if it interests you, but there would be a lot of less interesting work to do (testing, documentation, reviewing etc.), some of which would have to be done by others (it's hard to review your own code 😄) before I would consider replacing the working code that we already have. I honestly think there are more productive things that you could do with your time and development skills.

Tthecreator commented 2 months ago

OK a docker image based on this branch is now available at DockerHub for testing docker pull lineageos4microg/docker-lineage-cicd:nns-v21-spoofing-patches

I decided to test this, but I'm getting this output unfortunately:

sudo docker run \
    -e "BRANCH_NAME=lineage-21.0" \
    -e "DEVICE_LIST=cheeseburger" \
    -e "SIGN_BUILDS=true" \
    -e "SIGNATURE_SPOOFING=restricted" \
    -e "USER_BUILD_SPOOFING=yes" \
    -e "WITH_GMS=true" \
    -e "INCLUDE_PROPRIETARY=true" \
    -e "ROOMSERVICE_BRANCHES=lineage-18.1" \
    -e "CLEAN_AFTER_BUILD=false" \
    -v "/home/user/lineage/lineage:/srv/src" \
    -v "/home/user/lineage/zips:/srv/zips" \
    -v "/home/user/lineage/logs:/srv/logs" \
    -v "/home/user/lineage/cache:/srv/ccache" \
    -v "/home/user/lineage/keys:/srv/keys" \
    -v "/home/user/lineage/manifests:/srv/local_manifests" \
    lineageos4microg/docker-lineage-cicd:nns-v21-spoofing-patches
Set cache size limit to 50.0 GB
>> [Tue Apr 23 10:24:41 UTC 2024] Branch:  lineage-21.0
>> [Tue Apr 23 10:24:41 UTC 2024] Devices: cheeseburger,
>> [Tue Apr 23 10:24:42 UTC 2024] (Re)initializing branch repository
>> [Tue Apr 23 10:24:42 UTC 2024] Copying '/srv/local_manifests/*.xml' to '.repo/local_manifests/'
>> [Tue Apr 23 10:24:43 UTC 2024] Syncing branch repository
>> [Tue Apr 23 10:28:05 UTC 2024] Calling git lfs pull disabled
>> [Tue Apr 23 10:28:05 UTC 2024] Applying the user build variant signature spoofing patch (android_frameworks_base-user_build.patch) to frameworks/base
/root/build.sh: line 299: frameworks_base_patch: unbound variable

It seems that the warning I'm supposed to get here isn't working properly. It seems the check itself, which should check if the variable exists creates this crash itself. Only completely removing SIGNATURE_SPOOFING in my command fixes this (at least the build is running now, whether it fully passes will have to wait).

Personally, I think we should maintain the distinction between "yes" and "restricted". Some people want the flexibility of yes (and accept the risk, which is just that you could accidentally give it to an app you should not trust.). Others want the restriction of this permission for system apps. Perhaps we should look to keep this option possible. When using USER_BUILD_SPOOFING, does this work like the old "restricted" or "yes"?

petefoth commented 2 months ago

OK a docker image based on this branch is now available at DockerHub for testing docker pull lineageos4microg/docker-lineage-cicd:nns-v21-spoofing-patches

I decided to test this, but I'm getting this output unfortunately:

<snip>
/root/build.sh: line 299: frameworks_base_patch: unbound variable

It seems that the warning I'm supposed to get here isn't working properly. It seems the check itself, which should check if the variable exists creates this crash itself. Only completely removing SIGNATURE_SPOOFING in my command fixes this (at least the build is running now, whether it fully passes will have to wait).

This has been addressed in https://github.com/lineageos4microg/docker-lineage-cicd/pull/612, but PR probably won;t get merged - see the following comments

Personally, I think we should maintain the distinction between "yes" and "restricted". Some people want the flexibility of yes (and accept the risk, which is just that you could accidentally give it to an app you should not trust.). Others want the restriction of this permission for system apps. Perhaps we should look to keep this option possible.

I agree, which is why I have separated the problem into two parts, which will (with luck) be fixed separately

I hope that clarifies things a bit