build a magisk module as well; also some minor fixes: #8
Conversation
add flashable package
Update readme
Implement fail-safe mitigations for A/B devices
infinity0
force-pushed
the
main
branch
2 times, most recently
from
4826a27
to
cdaeb49
Compare
|
The whole point of this installation method is not using Magisk. Is there any point in creating a Magisk module when one already exists? The rest of the additions are most welcome. |
|
@arovlad That other one is very heavy - it requires internet access and forces the user to download Bromite during the Magisk installation step. It's also unclear how Bromite is to be updated, whether it actually uses the user-installed copy or its own copy - its documentation says the user should re-install the module for updates. Also, that team produces good software but unfortunately due to lack of funding has decided to put all sorts of bloaty and ad-like features into their modules. This module for this PR is very light, it only includes your overlay, doesn't require internet access during installation, and the user can install Bromite separately however they want, and it is this copy that is used. |
| @@ -2,18 +2,26 @@ | |||
|
|
|||
| In order for Bromite SystemWebView to be [installed](https://github.com/bromite/bromite/wiki/Installing-SystemWebView), it must be one of the supported webviews hardcoded in the framework package. Since ROMs typically don't include Bromite SystemWebView among them, the community has developed some methods that allow the framework to be patched in order to include it. | |||
|
|
|||
| This package makes use of a [resource overlay](https://source.android.com/docs/core/architecture/rros) to replace the list of hardcoded webviews with one that also includes the Bromite WebView. I personally find this method more straightforward and elegant, as it does not require root access nor the tedious process of installing Magisk modules or patching the system framework itself manually — if anything breaks the package can simply be removed. Moreover, the WebView itself does not need to be installed as a system app and has no potential risk of breaking SafetyNet. | |||
There was a problem hiding this comment.
The wording here should not be modified: root access and Magisk is still not required. This change should be dropped.
There was a problem hiding this comment.
This paragraph is about the resource overlay, whereas how to install the overlay is a separate issue. So I think it's clearer to discuss what types of root access is required, in a different paragraph.
In fact, all installation methods require some type of root access. It is misleading from a security perspective to claim any of these methods "do not require [any type of] root access". More precisely, I think what you mean is that the custom recovery method means you don't require root access on your main system. This is fair, and I've amended the wording to say this, in a paragraph further below. But it is misleading to unconditionally say it "does not require root" with no other qualifiers - when I run adb sideload in custom recovery, this is effectively giving your script root access to my system, it modifies the system partitions.
There was a problem hiding this comment.
You are right, what I meant was that it does not require a rooted device, so I would suggest replacing
it does not require root access
with
it does not require a rooted device
while also keeping the mention about not needing Magisk. To reach a sort of compromise between our conflicting views about Magisk, I would suggest adding a paragraph below this line mentioning that if keeping Magisk for customisation purposes is the top-most priority for the user and or if there isn't any space available on the partition a Magisk module is included for convenience only. Please also mention that I do not endorse Magisk, eventually linking to this Reddit comment.
I am very skeptical about this addition though.
- it's updater-script not update-script - "zip - . > X" instead of "zip X" otherwise zip(1) adds to an existing X
README.md
Outdated
|
|
||
| It performs steps similar to the above, i.e. installs `treble-overlay-bromite-webview.apk` into `/vendor/overlay`, but as a Magisk module. The overlay is performed dynamically by Magisk at boot time, instead of directly modifying the vendor partition (until the next system update overwrites these changes). The OTA survival script is therefore not needed, as Magisk modules are not overwritten by system updates. | ||
|
|
||
| It is included for convenience and undergoes less frequent testing as the main developer does not endorse Magisk. Its main use case is for when you have Magisk already installed, where it works around an issue where some ROMs (in particular MicroG ROMs) do not have enough reserved partition space to install addons. This makes the previous installation options fail, sometimes in non-obvious ways. (Details in #5 and lineageos4microg/docker-lineage-cicd#358) |
There was a problem hiding this comment.
I would suggest moving this paragraph right below the heading and making the first sentence bold.
| @@ -2,18 +2,26 @@ | |||
|
|
|||
| In order for Bromite SystemWebView to be [installed](https://github.com/bromite/bromite/wiki/Installing-SystemWebView), it must be one of the supported webviews hardcoded in the framework package. Since ROMs typically don't include Bromite SystemWebView among them, the community has developed some methods that allow the framework to be patched in order to include it. | |||
|
|
|||
| This package makes use of a [resource overlay](https://source.android.com/docs/core/architecture/rros) to replace the list of hardcoded webviews with one that also includes the Bromite WebView. I personally find this method more straightforward and elegant, as it does not require root access nor the tedious process of installing Magisk modules or patching the system framework itself manually — if anything breaks the package can simply be removed. Moreover, the WebView itself does not need to be installed as a system app and has no potential risk of breaking SafetyNet. | |||
There was a problem hiding this comment.
You are right, what I meant was that it does not require a rooted device, so I would suggest replacing
it does not require root access
with
it does not require a rooted device
while also keeping the mention about not needing Magisk. To reach a sort of compromise between our conflicting views about Magisk, I would suggest adding a paragraph below this line mentioning that if keeping Magisk for customisation purposes is the top-most priority for the user and or if there isn't any space available on the partition a Magisk module is included for convenience only. Please also mention that I do not endorse Magisk, eventually linking to this Reddit comment.
I am very skeptical about this addition though.
README.md
Outdated
| Although this method should work on all Android versions that support Bromite and it's WebView, **currently testing has only been done on LineageOS 19.1 for MicroG based on Android 12.1**. | ||
| Although this method should work on all Android versions that support Bromite and its WebView, **currently testing has only been done on LineageOS 19.1 for MicroG based on Android 12.1**. | ||
|
|
||
| The resource overlay can be installed in a few different ways - as a system addon, manually into the system vendor partition, or as a Magisk module. Instructions for each approach, as well as their pros and cons, are discussed below. |
There was a problem hiding this comment.
Again, as a sort of compromise, the only method we should endorse is flashing via recovery. This line should be dropped.
README.md
Outdated
| @@ -15,6 +17,8 @@ Although this method should work on all Android versions that support Bromite an | |||
|
|
|||
| ## Installation | |||
|
|
|||
| Installation via recovery allows you to keep avoiding root access on your main system. | |||
There was a problem hiding this comment.
We don't have to discuss the pros and cons of each method, as we should only endorse flashing via recovery. This line should be dropped.
|
@arovlad For the sake of reaching an agreement and getting this PR merged, I have done what you asked. (The wording may be a little clumsy; if so please suggest more fixes.) Separately I would like to comment that I strongly disagree with this comment and I think it has an incorrect concept of security. While there is merit in keeping root access off your main system, it is weird and IMO a bit disingenious to complain that Magisk/MicroG is insecure simply because it can do horrible things to your system. All of these horrible things, LineageOS itself can do. That is the whole point of me rooting my own device, I become responsible for anything I install onto it. Assuming Magisk/MicroG/LineageOS themselves are secure, there is no way for an attacker to forcibly install bad stuff onto the user's device without their consent (or, it's a bug that must be fixed ASAP). So while there is merit in reducing the attack surface (so that it's only "LineageOS" rather than "LineageOS+MicroG+Magisk"), it is disingenuous to claim there is a fundamental security difference between them. Similarly, I do not see a fundamental difference between "unlocking" vs "rooting" a device, IMO having an unlocked bootloader is already the same as having a "rooted device", so I hesitate to use this wording as you suggested. (I am happy with "rooted/rooting [the] main system", which is what I've used in the latest commit.) On a normal unlocked device, the manufacturer is the God of your device, software updates must be signed by their key. On an unlocked device, whoever physically controls the device is the God of the device. Many people prefer the latter security model, which is why they unlock/root their device, generally technical people who are confident about their own security assessments. When I install a custom ROM I delegate some of my God powers to LineageOS, and likewise when I install Magisk/MicroG. A LineageOS dev complaining about Magisk/MicroG seems to me like someone who has been delegated God powers, complaining about other people also having been delegated God powers. I just can't buy into this logic. |
|
You are arguments are very compelling indeed. I've done the README cleanup myself instead of requesting changes again because it's been two weeks and we are blocking the merge by discussing a tiny piece of documentation. If you do not agree with the changes please file an issue or create a new pull request. I will happily engage in another productive discussion. Thank you! |
|
Thanks for the merge. I'm happy with the final copywriting except for the wording "rooted device" as discussed above. I'm not sure how to convince you beyond what I already said above, but since you say you are happy to continue the discussion I'll file an issue about it. |
As mentioned on #5 certain microG images don't have any space on the /system and /vendor partitions; we can work around this by installing the overlay as a Magisk module. This is very simple to do, as shown here in this PR.
Tested on OnePlus 8T (KB2003), LineageOS 19.1 with MicroG.