-
Notifications
You must be signed in to change notification settings - Fork 21
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
General Improvements and Restructuring #120
base: master
Are you sure you want to change the base?
Conversation
- Add a better way of getting RIA's key - Implement sudo instead of direct root usage - Use absolute paths instead of relative paths - Add Kali Linux support - Formatting and readability improvements
- Use absolute paths instead of relative paths - Formatting and readability improvements
- Use absolute paths instead of relative paths - Formatting and readability improvements
Absolute paths break builds on other distros, and make the scripts harder to read. Is there any reason you don't use My workstation, Arch Linux:
|
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:
and
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 However, if the maintainers prefer me to revamp them, I can think of a solution that would satisfy the use of absolute paths. |
Next time, please separate your commits based on changes, not on files. As it stands now, your changes are hard to work with.
When changing, describe how your way is better. I think it is objectively worse since:
|
Not all systems have/use
Sudo is direct root usage. |
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. |
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.
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 :) |
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 I do not understand what you meant to imply with this statement:
This still performs the same actions as linux-installer/install-open-eid.sh Lines 63 to 69 in c586ce1
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 ( linux-installer/install-open-eid.sh Lines 91 to 98 in c586ce1
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).
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.
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.
I assume between the
I'll try to be there. Until then, have a nice evening yourself :) |
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 trust1, if you can provide it in the same place as the script. Correct, the script generally assumes internet connectivity. The point is more hypothetical.
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.
Yes. Footnotes
|
I dont understand why you use full paths? |
It is a security-practice for Bash scripts that run certain commands with
As stated in my notes (besides the RIA key and absolute paths):
I'll commit the reverts tomorrow. |
I have performed several improvements that were needed as I tried to get DigiDoc4 to work on my Kali Linux when I was addressing open-eid/DigiDoc4-Client#1215. This called for an "overhaul" of
install-open-eid.sh
. However, I noticed minor issues inuninstall-open-eid.sh
andesteid-update-nssdb
as well, which I have addressed.A general overview of the changes would be the following:
install-open-eid.sh
install-open-eid.sh
)install-open-eid.sh
Signed-off-by: Arslan Masood ([email protected])