search

open-eid / linux-installer

Ubuntu meta package
55 stars 21 forks source link

General Improvements and Restructuring #120

Open Arszilla opened 10 months ago

Arszilla commented 10 months ago

I have performed several improvements that were needed as I tried to get DigiDoc4 to work on my Kali Linux when I was addressing https://github.com/open-eid/DigiDoc4-Client/issues/1215. This called for an "overhaul" of install-open-eid.sh. However, I noticed minor issues in uninstall-open-eid.sh and esteid-update-nssdb as well, which I have addressed.

A general overview of the changes would be the following:

Signed-off-by: Arslan Masood (contact@arszilla.com)

jtagcat commented 10 months ago

Absolute paths break builds on other distros, and make the scripts harder to read. Is there any reason you don't use $PATH (man 3 exec)?

My workstation, Arch Linux:

$ which ldconfig
/usr/bin/ldconfig
Arszilla commented 10 months ago

Absolute paths break builds on other distros. Is there any reason you don't use $PATH?

My workstation, Arch Linux:

$ which ldconfig
/usr/bin/ldconfig

My modifications were made in consideration of the OS' supported by the script - which are Debian and Ubuntu derivate distros. These follow a similar structure and layout.

For example:

root:~/ # which ldconfig
/usr/sbin/ldconfig
root:~/ # cat /etc/os-release                                                                                                                                                                         [14:59:27]
PRETTY_NAME="Ubuntu 23.04"
NAME="Ubuntu"
VERSION_ID="23.04"
VERSION="23.04 (Lunar Lobster)"
VERSION_CODENAME=lunar
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=lunar
LOGO=ubuntu-logo

and 

arszilla:~/ $ which ldconfig
/usr/sbin/ldconfig
arszilla:~/ $ cat /etc/os-release
PRETTY_NAME="Kali GNU/Linux Rolling"
NAME="Kali GNU/Linux"
VERSION_ID="2023.4"
VERSION="2023.4"
VERSION_CODENAME=kali-rolling
ID=kali
ID_LIKE=debian
HOME_URL="https://www.kali.org/"
SUPPORT_URL="https://forums.kali.org/"
BUG_REPORT_URL="https://bugs.kali.org/"
ANSI_COLOR="1;31"

Thus, these modifications should not affect Debian or Ubuntu-based systems. Arch, Gentoo, etc., are too "different" compared to Debian/Ubuntu. Additionally, Arch does not use apt but uses pacman instead. Thus, I do not understand why you would even run this script in Arch, given that this script is intended for Debian/Ubuntu-based systems.

However, if the maintainers prefer me to revamp them, I can think of a solution that would satisfy the use of absolute paths.

jtagcat commented 10 months ago

Next time, please separate your commits based on changes, not on files. As it stands now, your changes are hard to work with.

Add a better way of getting RIA's key in install-open-eid.sh

When changing, describe how your way is better. I think it is objectively worse since:

jtagcat commented 10 months ago

Implement sudo instead of direct root usage (especially in install-open-eid.sh)

Not all systems have/use sudo. Pure Debian included.

man sudo: sudo, sudoedit — execute a command as another user

Sudo is direct root usage.

jtagcat commented 10 months ago

Add Kali Linux support

Kali is not supported by Information System Authority, and likely will not be. PopOS (added by me, many years ago) is compatible with the script, but not supported, like Kali. If you want it to be supported, you'd have to advocate, acquire a budget commitment for multiple decades, gather support of the idea, and work with risk analysis with technical and non-technical people.

jtagcat commented 10 months ago

Thus, I do not understand why you would even run this script

Scripts are patched and adapted to make them work, you'd be surprised what happens in packaging. This could potentially make it harder for package maintainers.

Is there any reason you don't use $PATH (man 3 exec)?

$PATH is built in to the standard C library's exec, and its variants. You did not explain your reasoning for the change.

Formatting and readability improvements

You do realize you are nitpicking your reviewers, who are the same people (maintainers) whom have and must live with the changes day-to-day, and could have changed it last year, the year before, or in 2015?

In my opinion, the whitespace makes it harder to read.

If you want to have a chat, feel free to at 20:00 @ https://k-space.ee. Have a nice evening. Duty calls :)

Arszilla commented 10 months ago

Next time, please separate your commits based on changes, not on files. As it stands now, your changes are hard to work with.

Add a better way of getting RIA's key in install-open-eid.sh

When changing, describe how your way is better. I think it is objectively worse since:

* It outsources the script's trust to the machine's root certificates. National digital identity goes through regular risk analysis — usually, it is not a point of compromise, but still is an additional unneeded risk / exposure.

* Is less auditable

* Adds an additional dependency, `wget`

* Requires internet connectivity, while with the embedded key, the repository can be added offline / in an image (for potential machine deployments).

Regarding your initial statement: I initially was planning multiple commits, however, CONTRIBUTING.md advised/requested squashing (trivial) commits into one. Since the changes I did were chained with one another, I decided to squash them even further to address the changes. I could close this MR, re-do/re-commit my work and make it more clear, if desired.

Regarding the use of wget: I have tried using curl instead, as that's the most common (one might say industry-standard) way of installing GPG keys to Debian/Ubuntu systems. However, RIA's GPG key cannot be curl'd properly (using curl -sS https://installer.id.ee/media/install-scripts/C6C83D68.pub)

I do not understand what you meant to imply with this statement:

It outsources the script's trust to the machine's root certificates.

This still performs the same actions as https://github.com/open-eid/linux-installer/blob/c586ce1cdf8188999658c1d59e0cac011aea3955/install-open-eid.sh#L63-L69 but in one line instead.

I assume the "less auditable" statement is mostly for the RIA key part. I agree on that part, but this is also a common problem, where every package you initially install (from 3rd party sources) does this to an extent. For example, Docker: https://docs.docker.com/engine/install/debian/#install-using-the-repository

The additional dependency (wget) should not be much of a concern IMO. You did state that this adds the internet connectivity as a requirement, but by the natural flow of the script, it does expect the user to have internet. This is clearly demonstrated when make_install open-eid is triggered, performing sudo apt update and sudo apt install opensc open-eid:

https://github.com/open-eid/linux-installer/blob/c586ce1cdf8188999658c1d59e0cac011aea3955/install-open-eid.sh#L91-L98

Kali is not supported by Information System Authority, and likely will not be.

I do not expect Kali to be supported. I realized the need for it during my pentests. Hence my proposing this change. Since the repository/script supports Debian and Ubuntu-based systems, it does not hurt to include Kali in this list, in my personal opinion. Contrary to popular belief, Kali is a stable distro, as stable as Arch Linux is (i.e., given the user is smart and conscious about their actions).

Scripts are patched and adapted to make them work, you'd be surprised what happens in packaging. This could potentially make it harder for package maintainers.

True. I could re-do some of my work to make it more accessible. I'll give it some thought and also await feedback from the maintainers to see if I should close this MR or continue committing etc.

You do realize you are nitpicking your reviewers, who are the same people (maintainers) whom have and must live with the changes day-to-day, and could have changed it last year, the year before, or in 2015?

That was not my intention. It felt the formatting of the script was seriously inconsistent, whether it be in tabs, spacing, readability etc. God knows I won't be always noticing potential changes to the script(s) to help fix these over time, so I understand what you mean.

In my opinion, the whitespace makes it harder to read.

I assume between the case/esac statements?

If you want to have a chat, feel free to at 20:00 @ https://k-space.ee/. Have a nice evening. Duty calls :)

I'll try to be there. Until then, have a nice evening yourself :)

jtagcat commented 10 months ago

Regarding the use of wget: I have tried using curl instead

You don't need to wget, curl or network anything. This eliminates the possibility say RIA changes out the public key and serves malicious packages only for your IP. There is no need to use additional trust[^h], if you can provide it in the same place as the script.

[^h]: HTTPS / TLS / Public PKI infrastructure. It ends up in root certificates installed on your machine, though if they are tampered with, you probably have bigger problems. Before it 'ends up' in root certs, it 'passes the trust' through various providers. Since id.ee doesn't have a CAA record, any issuer trusted by your root certs could issue a malicious certificate for id.ee, MitM you, and provide a malicious public key to your trust.

Correct, the script generally assumes internet connectivity. The point is more hypothetical.

I do not expect Kali to be supported.

Sorry, I misread your changes, I thought Debian is supported (and adding it to the same logic flow without make_warn), but only Ubuntu is.

I assume between the case/esac statements?

Yes.

metsma commented 9 months ago

I dont understand why you use full paths? Please do not remove certificate from script. Hard to understand what does it fix?

Arszilla commented 9 months ago

I dont understand why you use full paths?

It is a security-practice for Bash scripts that run certain commands with sudo/as root. I'll revert that change (alongside the certificate).

Hard to understand what does it fix?

As stated in my notes (besides the RIA key and absolute paths):

I'll commit the reverts tomorrow.