evolix
/
shellpki
21
0
Fork 0
Code Issues Pull Requests Releases 3 Wiki Activity

rewrite #5

Merged
jdubois merged 89 commits from dev into master 2022-04-14 17:20:36 +02:00
Conversation 11 Commits 89 Files Changed 6 +1067 -311
jlecour commented 2020-05-04 14:25:04 +02:00
Owner
Copy Link

With a new command line option, we can accept a file containing the user password.

if --password-file is provided with a path to a readable file, the first line is read and stored into the PASSWORD variable.

With a new command line option, we can accept a file containing the user password. if `--password-file` is provided with a path to a readable file, the first line is read and stored into the `PASSWORD` variable.
👍 1
jlecour changed title from WIP: Accept a password file to WIP: rewrite 2020-05-05 00:29:37 +02:00
jlecour commented 2020-05-05 00:30:27 +02:00
Author
Owner
Copy Link

I've also added --end-date and --days to specify the expiration date.

I've also added `--end-date` and `--days` to specify the expiration date.
jlecour commented 2020-05-06 09:52:10 +02:00
Author
Owner
Copy Link

I've also added --non-interactive and --replace-existing command line options

I've also added `--non-interactive` and `--replace-existing` command line options
bwaegeneire commented 2020-10-28 10:07:44 +01:00
Owner
Copy Link

I would really like to see this new feature set merged. It would save us a lot of time when renewing certificates.

What is missing for this work-in-progress to be a reall PR? How can I help?

I would really like to see this new feature set merged. It would save us a lot of time when renewing certificates. What is missing for this work-in-progress to be a reall PR? How can I help?
jdubois added 1 commit 2021-02-08 15:36:52 +01:00
jdubois added 1 commit 2021-03-02 10:09:29 +01:00
jdubois added 1 commit 2021-06-14 14:30:52 +02:00
jdubois added 1 commit 2022-02-18 11:47:21 +01:00
jdubois reviewed 2022-02-21 15:13:46 +01:00
jdubois requested changes 2022-02-21 15:24:19 +01:00
shellpki
@ -4,3 +4,3 @@
#
set -e
set -u
jdubois commented 2022-02-21 15:21:21 +01:00
Member
Copy Link

This cause somes bugs, especially with "list" options.

Whit "set -u" :

root@test-vpn:~# shellpki list -a
/usr/local/sbin/shellpki: 775: 1: parameter not set

root@test-vpn:~# shellpki list -a anything
test-vpn.evolix.net

Without "set -u" :

root@test-vpn:~# shellpki list -a
test-vpn.evolix.net

This bug is also present on the current master version if I add "set -u" but I cannot find the reason.
Not sure if some code need a fix or not, but I think we should at least remove "set -u".

This option also cause the error messages added in the code to handle a missing variable to not be displayed.

This cause somes bugs, especially with "list" options. Whit "set -u" : ``` root@test-vpn:~# shellpki list -a /usr/local/sbin/shellpki: 775: 1: parameter not set root@test-vpn:~# shellpki list -a anything test-vpn.evolix.net ``` Without "set -u" : ``` root@test-vpn:~# shellpki list -a test-vpn.evolix.net ``` This bug is also present on the current master version if I add "set -u" but I cannot find the reason. Not sure if some code need a fix or not, but I think we should at least remove "set -u". This option also cause the error messages added in the code to handle a missing variable to not be displayed.
jlecour commented 2022-03-11 10:17:44 +01:00
Author
Owner
Copy Link

I'm pretty sure we can find a way to keep the security added by set -u and fix this.

I'm pretty sure we can find a way to keep the security added by `set -u` and fix this.
shellpki Outdated
@ -354,0 +766,4 @@
if [ -z "${1}" ]; then
show_usage >&2
exit 1
fi
jdubois commented 2022-02-21 15:15:56 +01:00
Member
Copy Link

This is not needed as "shellpki list" should, thanks to lines 771 and 772, list all valids certificates by default.

This is not needed as "shellpki list" should, thanks to lines 771 and 772, list all valids certificates by default.
jdubois commented 2022-02-21 16:35:50 +01:00
Member
Copy Link

We should update the README file and the show_usage function with the new options.

We should update the README file and the show_usage function with the new options.
👍 1
jdubois commented 2022-02-21 16:36:06 +01:00
Member
Copy Link

With a new command line option, we can accept a file containing the user password.

if --password-file is provided with a path to a readable file, the first line is read and stored into the PASSWORD variable.

Great ! We can automate the creation of many secured certificates with a loop, for example :

for i in a b c d e f; do apg -n1 -m 12 > $i; shellpki create --password-file $i $i; done

Maybe we could have an option in shellpki to not have to use a loop by ourselves ?

> With a new command line option, we can accept a file containing the user password. > > if `--password-file` is provided with a path to a readable file, the first line is read and stored into the `PASSWORD` variable. Great ! We can automate the creation of many secured certificates with a loop, for example : ``` for i in a b c d e f; do apg -n1 -m 12 > $i; shellpki create --password-file $i $i; done ``` Maybe we could have an option in shellpki to not have to use a loop by ourselves ?
jdubois commented 2022-02-21 16:36:34 +01:00
Member
Copy Link

I've also added --end-date and --days to specify the expiration date.

Ok for "--days" option, but I had to examine the code for the "--end-date" one : it uses date -d, so the format is MM/DD/[YY]YY [hh:mm:ss].

This option is compatible on Debian, but date on OpenBSD does not implement the -d option. Why don't we directly use the date format expected by openssl YYMMDDHHMMSSZ, with an explicit readme ?

> I've also added --end-date and --days to specify the expiration date. Ok for "--days" option, but I had to examine the code for the "--end-date" one : it uses `date -d`, so the format is MM/DD/[YY]YY [hh:mm:ss]. This option is compatible on Debian, but `date` on OpenBSD does not implement the `-d` option. Why don't we directly use the date format expected by openssl `YYMMDDHHMMSSZ`, with an explicit readme ?
jdubois commented 2022-02-21 16:36:49 +01:00
Member
Copy Link

I've also added --non-interactive and --replace-existing command line options

Great ! --non-interactive could be use to automate the installation with Ansible.

But I've found a bug with the two options at once, if we do not provide CA_PASSWORD :

root@test-vpn:~# shellpki create --replace-existing --non-interactive g
Password for CA key:

The In non-interactive mode, you must pass CA_PASSWORD as environment variable message error is not displayed anymore.

> I've also added --non-interactive and --replace-existing command line options Great ! `--non-interactive` could be use to automate the installation with Ansible. But I've found a bug with the two options at once, if we do not provide CA_PASSWORD : ``` root@test-vpn:~# shellpki create --replace-existing --non-interactive g Password for CA key: ``` The `In non-interactive mode, you must pass CA_PASSWORD as environment variable` message error is not displayed anymore.
jdubois commented 2022-02-21 17:22:04 +01:00
Member
Copy Link

Great ! --non-interactive could be use to automate the installation with Ansible.

In fact it is not possible for now, because openssl still ask for the CA password :

root@test-vpn:~# CA_PASSWORD=XXX shellpki init --non-interactive test-vpn.evolix.net
Enter pass phrase for /etc/shellpki/cakey.key:
Verifying - Enter pass phrase for /etc/shellpki/cakey.key:

But we could add -passout pass:${CA_PASSWORD} to openssl command :

openssl genrsa -out $ca_key -aes256 -passout pass:$ca_password $ca_key_lenght
> Great ! --non-interactive could be use to automate the installation with Ansible. In fact it is not possible for now, because openssl still ask for the CA password : ``` root@test-vpn:~# CA_PASSWORD=XXX shellpki init --non-interactive test-vpn.evolix.net Enter pass phrase for /etc/shellpki/cakey.key: Verifying - Enter pass phrase for /etc/shellpki/cakey.key: ``` But we could add `-passout pass:${CA_PASSWORD}` to openssl command : ``` openssl genrsa -out $ca_key -aes256 -passout pass:$ca_password $ca_key_lenght ```
jdubois reviewed 2022-02-22 10:05:27 +01:00
shellpki Outdated
@ -171,0 +354,4 @@
--password-file)
# password-file option, with value separated by space
if [ -n "$2" ]; then
password_file=$(readlink --canonicalize -- "${2}")
jdubois commented 2022-02-22 10:05:27 +01:00
Member
Copy Link

Option --canonicalize is not compatible with OpenBSD, but option -f is :

On Debian, man readlink :

       -f, --canonicalize
              canonicalize by following every symlink in  every  component  of
              the  given name recursively; all but the last component must ex‐
              ist

On OpenBSD, man readlink :

     -f      Canonicalize by following every symlink in every component of the
             given path recursively.  readlink will resolve both absolute and
             relative paths and return the absolute pathname corresponding to
             file.  The argument does not need to be a symbolic link.

I suggest to replace all "--canonicalize" occurences by "-f".

Option `--canonicalize` is not compatible with OpenBSD, but option `-f` is : On Debian, `man readlink` : ``` -f, --canonicalize canonicalize by following every symlink in every component of the given name recursively; all but the last component must ex‐ ist ``` On OpenBSD, `man readlink` : ``` -f Canonicalize by following every symlink in every component of the given path recursively. readlink will resolve both absolute and relative paths and return the absolute pathname corresponding to file. The argument does not need to be a symbolic link. ``` I suggest to replace all "--canonicalize" occurences by "-f".
jlecour added 7 commits 2022-03-11 14:18:42 +01:00
jlecour commented 2022-03-11 14:34:56 +01:00
Author
Owner
Copy Link

As of now, those topics remain:

  • end-date format
  • fix the non-interactive + replace-existing behaviour
As of now, those topics remain: * end-date format * fix the non-interactive + replace-existing behaviour
jdubois added 2 commits 2022-03-14 10:56:34 +01:00
jdubois added 1 commit 2022-03-14 11:03:49 +01:00
jlecour added 1 commit 2022-03-14 14:40:53 +01:00
jdubois added 1 commit 2022-03-22 18:01:55 +01:00
jdubois added 1 commit 2022-03-22 18:04:35 +01:00
jdubois added 1 commit 2022-03-22 18:09:25 +01:00
jdubois added 1 commit 2022-03-22 18:14:51 +01:00
jdubois added 9 commits 2022-03-29 18:49:08 +02:00
5f27702f17 Delete ovpn.conf.example unnecessary here 
shellpki alone is not enough to install OpenVPN, and the openvpn role provides
the openvpn server configuration
9f13a42355 Handle the case where --days argument is not a number or a negative one 
Before this test, the error was displayed but ignored and the certificate was
still created depending on the default_days value in openssl.cnf
jdubois commented 2022-03-29 18:58:02 +02:00
Member
Copy Link

Everything mentioned here has been reviewed, and I fixed some other bugs.

I found one that I don't know how to fix for now, but not sure if very important :
If we use shellpki with --end-date --days in this particular order, --days is seen as correct on Linux for date(1), as it is interpreted as "one day", and the certificate is created for one day :

$ date -d "--days"
Wed 30 Mar 2022 06:53:22 PM CEST

Plus, if we use it with --end-date --days 5 cert.example.org for example, it won't create the certificate "cert.example.org" for 5 days, but the certificate "5" for one day.

I still have some tests to do before we can merge this PR.

Everything mentioned here has been reviewed, and I fixed some other bugs. I found one that I don't know how to fix for now, but not sure if very important : If we use shellpki with `--end-date --days` in this particular order, --days is seen as correct on Linux for `date(1)`, as it is interpreted as "one day", and the certificate is created for one day : ``` $ date -d "--days" Wed 30 Mar 2022 06:53:22 PM CEST ``` Plus, if we use it with `--end-date --days 5 cert.example.org` for example, it won't create the certificate "cert.example.org" for 5 days, but the certificate "5" for one day. I still have some tests to do before we can merge this PR.
jdubois added 1 commit 2022-03-29 18:59:35 +02:00
jdubois added 3 commits 2022-04-04 18:06:59 +02:00
jdubois added 1 commit 2022-04-04 18:14:42 +02:00
jdubois added 6 commits 2022-04-14 16:48:45 +02:00
55e02c6a13 Check if CN already exists only after having asked for user password 
Otherwise, with "-p", "--replace-existing" and "--non-interactive", with
CA_PASSWORD set but PASSWORD unset, the existing certificate was revoked but
the new one could'nt be created. Now, PASSWORD must be set or the exisiting
certificate won't be revoked
jdubois commented 2022-04-14 17:07:01 +02:00
Member
Copy Link

I finished testing everything and I made some more fixes and improvements.
Everything is working fine for me. We can merge this PR.

I finished testing everything and I made some more fixes and improvements. Everything is working fine for me. We can merge this PR.
jdubois changed title from WIP: rewrite to rewrite 2022-04-14 17:07:07 +02:00
jdubois added 1 commit 2022-04-14 17:15:32 +02:00
jdubois added 1 commit 2022-04-14 17:20:16 +02:00
jdubois merged commit 480ead3ff2 into master 2022-04-14 17:20:36 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
jdubois
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: evolix/shellpki#5
No description provided.
Delete Branch "dev"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?