Introduce install_data_to_pki() - Copy data-files to PKI #510

TinCanTech · 2022-03-27T13:24:32Z

The purpose here is to force EasyRSA find the required data-files:

'openssl-easyrsa.cnf' MUST be found.
'x509-types' MUST be found.
'vars.example' should be found.
'vars'
The 'vars' file is more complicated due to user expectations.
This patch does not copy 'vars', the code is included but DISABED.

The reasons are:

Allow running 'easyrsa' from PATH.
Make standard packaging work correctly.

Bug fixes:

Running easyrsa from a symlink or PATH #499 and associated issues with missing files.

Signed-off-by: Richard T Bonhomme tincantech@protonmail.com

The purpose here is to force EasyRSA find the required data-files: * 'openssl-easyrsa.cnf' MUST be found. * 'x509-types' MUST be found. * 'vars.example' should be found. * 'vars' The 'vars' file is more complicated due to user expectations. This patch does not copy 'vars', the code is included but DISABED. The reasons are: * Allow running 'easyrsa' from PATH. * Make standard packaging work correctly. Bug fixes: * #499 and associated issues with missing files. Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>

TinCanTech · 2022-03-27T13:26:57Z

NOTE: Fix multiple typos and structure in commit message!

This is the final version of #508.

Please, tear it to pieces with me ;-)

My version: https://github.com/TinCanTech/easy-rsa/blob/master/easyrsa3/easyrsa

Prouflon

Very nice.
I reviewed all the changes, and agree mostly.
There are some things I would suggest though.

Prouflon · 2022-03-27T20:28:17Z

easyrsa3/easyrsa

+		"$area_prog" \
+		"$area_etc" \
+		"$area_ubuntu" \
+		# EOL - # Add more distros here


Is there any advantage to the area_* style variables?
Writing the Paths directly in the for loop seems alright.
And adding another directory for example /usr/local/share would just be more convenient in the future.

Excellent observation, thanks!

However, there does not seem to be a better way to present maintainable code. This code is clear, alternatives are less clear.

I think adding a string in a for loop is more clear, than declaring your own unique area_* variable with said string and also add that area_* variable into the for loop.
In any case the loop is altered.
That's two changes to support another directory as opposed to one:

for area in \ "$PWD" \ "${0%/*}" \ '/etc/easy-rsa' \ '/usr/share/easy-rsa' \ # EOL - # Add more distros here do

Now that I've written it, I kind of see what you mean.
I am still convinced that this solution is preferable.

It's a style choice, I prefer my way because it has good comments.

May be swing back to this another day?

I have decided; Your way is better, thanks !

easyrsa3/easyrsa

Prouflon · 2022-03-27T20:50:00Z

easyrsa3/easyrsa

+# Currently, *if* 'vars' is copied to the PKI then the PKI 'vars' will take
+# priority over './vars'. But it will not be updated if './vars' is changed.
+#
+# Copying 'vars' to the PKI is complicated, code is included but DISABLED.


After some consideration, I would not touch vars at all for some reasons:

Providing the user with vars.example when they just have to rename it to vars for it to work is fine

Users who already rely on the functionality of a shared ./vars file will find it annoying that after init-pki they have to remove $EASYRSA_PKI/vars because of priorities

Fewer files in $EASYRSA_PKI

We wouldn't have to implement it

I do agree ..

However, moving all PKI related files into said PKI is where EasyRSA is trying to get. It is just vars is going to be tricky.

Isn't vars optional?
If it's not found the default values are loaded from within easyrsa, or am I wrong?

I would be in favour of removing ./vars altogether and moving vars into the PKI at init-pki, so there is never any external-to-PKI vars, at all.

Isn't vars optional? If it's not found the default values are loaded from within easyrsa, or am I wrong?

You are absolutely correct.

EasyRSA wants to change this default, to enable PATH and fix some bugs.

The problem is, old PKIs will have either nothing or fixed ./vars.

The real problem is: Once pki/vars exists it will take priority and that is a breaking change.

easyrsa would have to remove ./vars and present a severe warning to the user.

This patch-set gets things in place so that a more invasive change is less objectionable, in time.

There may be a way to infiltrate all new PKIs so that ./vars can be copied to the PKI but only if it does not grep for a specific string.

@Prouflon you are to blame ;-)

I absolutely support the notion of a warning, that appears if a ./vars file ist detected, that tells the user that this file is not read and that this default behaviour has changed.
We can expect users to move old vars files into the /pki on their own when the time comes, right?

easyrsa3/easyrsa

TinCanTech · 2022-03-27T21:52:08Z

@Prouflon I would like to credit you for your effort on this but I'm not sure that you would agree ..

Let me know your thoughts, thanks.

Prouflon · 2022-03-27T21:55:31Z

Credit me as much as you ~~can~~ like :]

TinCanTech · 2022-03-27T22:49:11Z

@Prouflon I give my thanks to you here :-)

All feedback is invaluable.

TinCanTech · 2022-03-27T23:28:55Z

@Prouflon shall we throw this to the wolves ?

LGTM.

TinCanTech · 2022-03-28T01:36:15Z

This sounds crazy:

easyrsa upgrade vars - Explicitly move ./vars to PKI and remove source vars ... ?
User consent given..

pfffft .. as if ..

TinCanTech · 2022-03-28T12:35:40Z

I think this patch needs one more change:

install_data_to_pki() failure should not be fatal, only issue a warning.

Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>

Prouflon · 2022-03-28T17:51:45Z

+1
The warning should inform the user, that they have to check the openssl-easyrsa.cnf and x509-types/ exist and install them themselves if necessary.

Prouflon · 2022-03-28T18:35:09Z

@Prouflon shall we throw this to the wolves ?

LGTM.

As in merge? Sorry, not sure what you mean exactly ...

TinCanTech · 2022-03-28T18:44:31Z

LGTM -> Looks good to me.

Thanks to excellent community feedback, this patch forces a single, reliable list of sources for EasyRSA data-files. Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>

This is a deliberate misuse of shellcheck: Reminder to fix PKI/vars. Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>

Prouflon · 2022-03-28T22:11:03Z

I have looked for typos and the like and come up empty.
Looks good to me too.
(I somehow tought LGTM meant "legitimate")

TinCanTech · 2022-03-28T22:19:03Z

@Prouflon When the trifle hits the fan, I am going to blame all of this on you!

Genuinely: Thank you very much. Your participation has been invaluable. 🍰 🍺

TinCanTech self-assigned this Mar 27, 2022

TinCanTech added feedback welcome Priority Acknowledged priority development Possible changes labels Mar 27, 2022

TinCanTech mentioned this pull request Mar 27, 2022

added support to specify open-ssl config file using --ssl-conf command flag #67

Merged

TinCanTech added the initial-approval label Mar 27, 2022

Prouflon suggested changes Mar 27, 2022

View reviewed changes

Prouflon mentioned this pull request Mar 27, 2022

Insert functional code: install_data_to_pki() #509

Closed

TinCanTech added Full-Approval Merge is imminent and removed initial-approval labels Mar 28, 2022

Change install_data_to_pki() failures to non-fatal warnings

bc07187

Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>

TinCanTech added this to the v3.0.9 milestone Mar 28, 2022

TinCanTech added 2 commits March 28, 2022 22:44

Optimize install_data_to_pki()

a8a2171

Thanks to excellent community feedback, this patch forces a single, reliable list of sources for EasyRSA data-files. Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>

Declare 'vars_file' without use

388aa03

This is a deliberate misuse of shellcheck: Reminder to fix PKI/vars. Signed-off-by: Richard T Bonhomme <tincantech@protonmail.com>

TinCanTech added the Community reveiwed Genuine community participation label Mar 28, 2022

TinCanTech merged commit 3f7c7df into OpenVPN:master Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce install_data_to_pki() - Copy data-files to PKI #510

Introduce install_data_to_pki() - Copy data-files to PKI #510

TinCanTech commented Mar 27, 2022

TinCanTech commented Mar 27, 2022 •

edited

Prouflon left a comment

Prouflon Mar 27, 2022 •

edited

TinCanTech Mar 27, 2022

TinCanTech Mar 27, 2022

Prouflon Mar 28, 2022

TinCanTech Mar 28, 2022

TinCanTech Mar 28, 2022

Prouflon Mar 27, 2022

TinCanTech Mar 27, 2022

Prouflon Mar 27, 2022

TinCanTech Mar 27, 2022

TinCanTech Mar 27, 2022 •

edited

TinCanTech Mar 27, 2022

TinCanTech Mar 27, 2022 •

edited

TinCanTech Mar 27, 2022

TinCanTech Mar 27, 2022 •

edited

Prouflon Mar 28, 2022

TinCanTech commented Mar 27, 2022

Prouflon commented Mar 27, 2022

TinCanTech commented Mar 27, 2022

TinCanTech commented Mar 27, 2022 •

edited

TinCanTech commented Mar 28, 2022 •

edited

TinCanTech commented Mar 28, 2022

Prouflon commented Mar 28, 2022

Prouflon commented Mar 28, 2022

TinCanTech commented Mar 28, 2022

Prouflon commented Mar 28, 2022 •

edited

TinCanTech commented Mar 28, 2022

Introduce install_data_to_pki() - Copy data-files to PKI #510

Introduce install_data_to_pki() - Copy data-files to PKI #510

Conversation

TinCanTech commented Mar 27, 2022

TinCanTech commented Mar 27, 2022 • edited

Prouflon left a comment

Choose a reason for hiding this comment

Prouflon Mar 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinCanTech Mar 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinCanTech Mar 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinCanTech Mar 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinCanTech commented Mar 27, 2022

Prouflon commented Mar 27, 2022

TinCanTech commented Mar 27, 2022

TinCanTech commented Mar 27, 2022 • edited

TinCanTech commented Mar 28, 2022 • edited

TinCanTech commented Mar 28, 2022

Prouflon commented Mar 28, 2022

Prouflon commented Mar 28, 2022

TinCanTech commented Mar 28, 2022

Prouflon commented Mar 28, 2022 • edited

TinCanTech commented Mar 28, 2022

TinCanTech commented Mar 27, 2022 •

edited

Prouflon Mar 27, 2022 •

edited

TinCanTech Mar 27, 2022 •

edited

TinCanTech Mar 27, 2022 •

edited

TinCanTech Mar 27, 2022 •

edited

TinCanTech commented Mar 27, 2022 •

edited

TinCanTech commented Mar 28, 2022 •

edited

Prouflon commented Mar 28, 2022 •

edited