Bug 41013 - univention-certificate should offer a renew-option with a transition period
univention-certificate should offer a renew-option with a transition period
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: SSL
UCS 4.1
Other Linux
: P5 enhancement (vote)
: UCS 4.3-2-errata
Assigned To: Jannik Ahlers
Philipp Hahn
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-06 22:17 CEST by Michael Grandjean
Modified: 2018-09-26 13:24 CEST (History)
13 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 5: Blocking further progress on the daily work
User Pain: 0.286
Enterprise Customer affected?: Yes
School Customer affected?:
ISV affected?:
Waiting Support: Yes
Flags outvoted (downgraded) after PO Review:
Ticket number: 2018062721000232
Bug group (optional): Usability
Max CVSS v3 score:
requate: Patch_Available+


Attachments
Suggested patch for univention-ssl package plus simple test (5.69 KB, patch)
2016-09-26 17:26 CEST, Julius Hinrichs
Details | Diff
Improved patch (8.77 KB, patch)
2016-09-28 16:16 CEST, Julius Hinrichs
Details | Diff
Patch with improved test (8.88 KB, patch)
2016-09-28 18:29 CEST, Julius Hinrichs
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Grandjean univentionstaff 2016-04-06 22:17:47 CEST
"univention-certificate renew" does the following in this order:

1. Revoke the old certificate based on the serial number
2. Update the Certification Revocation List (CRL)
3. Generate a new certificate based on the given FQDN

This makes the old certificate invalid in the very same moment that the new certificate is created. This might be problematic because it breaks every service that checks the CRL until the new certificate is copied to the client and all relevant services are restarted. 
This is really cumbersome if this has to be done for dozens or even hundreds of certificates. 

Therefore univention-certificate should offer a renew-option that allows the creation of a new certificate while the old one is still valid (not revoked). After a configurable grace period the old certificate should then be revoked automatically.
Comment 1 Nico Stöckigt univentionstaff 2016-05-24 10:41:20 CEST
dearly required at Ticket#2016052321000451
Comment 2 Julius Hinrichs univentionstaff 2016-09-26 17:26:14 CEST
Created attachment 8035 [details]
Suggested patch for univention-ssl package plus simple test

With this patch, you can use the '-grace'-option of 'univention-certificate renew' to define a grace period in days. A new certificate with the same name will be created and the old one will only be revoked after the grace period has passed.

In order to have two certificates with the same FQDN, you need to set 'unique_subject = no'. On an existing system this must be set both in '/etc/univention/ssl/openssl.cnf' and '/etc/univention/ssl/ucsCA/index.txt.attr'.
See:
https://pyro.eu.org/how-to/micro/openssl-txt_db-error-number-2.txt
https://www.openssl.org/docs/manmaster/apps/ca.html -> unique_subject
Comment 3 Julius Hinrichs univentionstaff 2016-09-28 16:16:54 CEST
Created attachment 8045 [details]
Improved patch
Comment 4 Julius Hinrichs univentionstaff 2016-09-28 18:29:38 CEST
Created attachment 8049 [details]
Patch with improved test
Comment 5 Florian Best univentionstaff 2017-06-28 14:53:05 CEST
There is a Customer ID set so I set the flag "Enterprise Customer affected".
Comment 6 Jannik Ahlers univentionstaff 2018-08-22 16:42:29 CEST
I applied the provided patch.
To make this patch work, you have to set 'unique_subject = no' and therefore allow multiple certificates with the same name. To make sure every certificate can still be accessed I built in the new option 'number'. Using this, you can address certificates by the ids provided by the 'list' and 'list-all' options.


univention-ssl (12.0.0-8)
93db6e20098d | Bug #41013: Merge branch 'jahlers/41013-certificate-invalidation' into 4.3-1
e36768cda8fc | Bug #41013: univention-certificate now offers a grace period for the renew option. Certificates can now get adressed by their number.

ucs-test (8.0.28-166)
93db6e20098d | Bug #41013: Merge branch 'jahlers/41013-certificate-invalidation' into 4.3-1
24a4b3b4bcd0 | Bug #41013: New Test for 'univenion-certificate renew' with grace period

Successful build
Package: univention-ssl
Version: 12.0.0-9A~4.3.0.201808221626
Branch: ucs_4.3-0
Scope: errata4.3-1
Comment 7 Felix Botner univentionstaff 2018-08-23 17:34:37 CEST
(1) changelog

fix the indentation of the 12.0.0-8 message

(2)

After the update i could not use the new functionality. I had to change unique_subject = yes to unique_subject = no in /etc/univention/ssl/ucsCA/index.txt.attr.

(3)

fix update_pending, it is broken (local pending_certs=$(cat "$pending_file") -> for cert in "${pending_certs[@]}" does not work) and possibly revoke_cert too

--- /usr/share/univention-ssl/make-certificates.sh.o    2018-08-23 17:23:02.720000000 +0200
+++ /usr/share/univention-ssl/make-certificates.sh  2018-08-23 17:07:21.420000000 +0200
@@ -468,16 +468,16 @@
    local pending_certs=$(cat "$pending_file")
    local temp=$(mktemp)
 
-   for cert in "${pending_certs[@]}"; do
-       local num=$(sed 's/:.*//' <<< "$cert")
-       local expire=$(sed 's/.*://' <<< "$cert")
+   while read line; do
+       local num=${line%%:*}
+       local expire=${line#*:}
        local now=$(date +"%s")
        if [ "$now" -gt "$expire" ]; then
            openssl ca -config "${SSLBASE}/openssl.cnf" -revoke "${SSLBASE}/${CA}/certs/${num}.pem" -passin pass:"$PASSWD"
        else
            echo "$num:$expire" >>"$temp"
        fi
-   done
+   done < "$pending_file"
 
    mv "$temp" "$pending_file"
    chmod 600 "$pending_file"

(4)

add update-pending list-pending to univention-certificate


(5) cron.daily

remove

. /usr/share/univention-ssl/make-certificates.sh
update_pending_certs

use univention-certificate update-pending (only on domaincontroller_master)

(6)

add a hint in the documentation https://help.univention.com/t/renewing-the-ssl-certificates/37 about the new switch
Comment 8 Philipp Hahn univentionstaff 2018-08-24 09:59:19 CEST
(In reply to Felix Botner from comment #7)
> (3)
...
> --- /usr/share/univention-ssl/make-certificates.sh.o    2018-08-23
> 17:23:02.720000000 +0200
> +++ /usr/share/univention-ssl/make-certificates.sh  2018-08-23
> 17:07:21.420000000 +0200
...
> +   while read line; do
> +       local num=${line%%:*}
> +       local expire=${line#*:}

while IFS=: read -r num expire
do
Comment 9 Erik Damrose univentionstaff 2018-08-27 11:30:49 CEST
As discussed, i reverted the commits for now. Please verify the correct revert and reopen the bug

c4a1178c
    Revert "Bug #41013: fix test test_sign_req"
    This reverts commit d81c4edc386f6a20525eaf12812815ef1b136b41.

7af8bebd
    Revert "Bug #41013: Merge branch 'jahlers/41013-certificate-invalidation' into 4.3-1"
    This reverts commit 93db6e20098dd815225f0d3fcbd3072ee49474ec, reversing
    changes made to a93c6d11fb27b650abc7b9c6865fcd510dd62bb4.

25bad3b9 remove yaml

I did not remove the source package. When reapplying the patch, the package version has to be increased to at least 12.0.0-10
Comment 10 Felix Botner univentionstaff 2018-08-27 12:10:15 CEST
OK reverted
Comment 11 Jannik Ahlers univentionstaff 2018-09-10 16:41:28 CEST
Successful build
Package: univention-ssl
Version: 12.0.0-10A~4.3.0.201809101627
Branch: ucs_4.3-0
Scope: errata4.3-2

univention-ssl (12.0.0-10)
977aadb67344 | Bug #41013: changelog
17795d4560b8 | Bug #41013: several functions in make-certificates.sh use certificate ids internally now to address individual certs

univention-ssl (12.0.0-9)
284d66269a96 | Bug #41013: reapply changes

NONE
b68df5756fad | Bug #41013: adjust extended documentation to new features

(1) Fixed
(2) Fixed; correct values get set in postinst
(3) Fixed
(4) Added
(5) Fixed
(6) Not fixed:
    - I currently don't have the rights to edit forum posts (I opened a helpdesk ticket)
    - It makes no sense to edit the docu before the errata is released
    - I made some minor changes to the extended domain documentation
  ==> I will edit the post when errata is released

To overcome some problems that come with CNs not being unique anymore, I had to rewrite some of make-certificate.sh's inner workings. I hope I didn't break too much ;).
Anyhow, I would suggest anyone to use the -number flag instead of the -name flag in the future, especially when using the grace period. This is also the reason I made some changes to the documentation.
Comment 12 Jannik Ahlers univentionstaff 2018-09-10 16:42:20 CEST
Successful build
Package: univention-ssl
Version: 12.0.0-10A~4.3.0.201809101627
Branch: ucs_4.3-0
Scope: errata4.3-2

univention-ssl (12.0.0-10)
977aadb67344 | Bug #41013: changelog
17795d4560b8 | Bug #41013: several functions in make-certificates.sh use certificate ids internally now to address individual certs

univention-ssl (12.0.0-9)
284d66269a96 | Bug #41013: reapply changes

NONE
b68df5756fad | Bug #41013: adjust extended documentation to new features

(1) Fixed
(2) Fixed; correct values get set in postinst
(3) Fixed
(4) Added
(5) Fixed
(6) Not fixed:
    - I currently don't have the rights to edit forum posts (I opened a helpdesk ticket)
    - It makes no sense to edit the docu before the errata is released
    - I made some minor changes to the extended domain documentation
  ==> I will edit the post when errata is released

To overcome some problems that come with CNs not being unique anymore, I had to rewrite some of make-certificate.sh's inner workings. I hope I didn't break too much ;).
Anyhow, I would suggest anyone to use the -number flag instead of the -name flag in the future, especially when using the grace period. This is also the reason I made some changes to the documentation.
Comment 13 Felix Botner univentionstaff 2018-09-11 16:05:21 CEST
I don't have a good feeling about these -name -number changes, at least this breaks the usercert cool solution

run /usr/sbin/univention-certificate-user renew -name Administrator -cn Administrator -days 1825 -certpath /etc/univention/ssl/user -sslbase /etc/univention/ssl -ca ucsCA -> Renew certificate: Administrator
 ca: Cannot open input file /etc/univention/ssl/ucsCA/certs/Administrator.pem, No such file or directory
ca: Use -help for summary.
Using configuration from /etc/univention/ssl/openssl.cnf
unable to write 'random state'
Using configuration from openssl.cn

Is this necessary?
Let us discuss the changes tomorrow.
Comment 14 Ole Schwiegert univentionstaff 2018-09-12 08:55:08 CEST
In the changelog file for ucs-test the entry for this bug has a date from august, yet is the newest version and comes after smaller version numbers with dates from september.
Comment 15 Jannik Ahlers univentionstaff 2018-09-13 10:47:21 CEST
I discussed it with Felix:
- specifying a grace period is probably unnecessary, as the certs have a validity date anyway
- the -number flag is necessary, as cert names aren't unique anymore. Giving the name of multiple certs should offer revocation for all of them at once.
Comment 16 Jannik Ahlers univentionstaff 2018-09-13 14:43:04 CEST
univention-ssl (12.0.0-11)
e0701c846f2a | Bug #41013: remove unnecessary grace option, enhance univention-certificate for dns with multiple certs

univention-ssl.yaml
d2b45d3a513f | Bug #41013: YAML

Successful build
Package: univention-ssl
Version: 12.0.0-11A~4.3.0.201809131431
Branch: ucs_4.3-0
Scope: errata4.3-2

- I removed the -grace flag. the old certs will still be valid until they expire or are revoked manually.
- the -name flag now works on multiple certs at once. Revoking multiple certs at once triggers a confirmation dialog, except the option revoke-non-interactive is used.
Comment 17 Felix Botner univentionstaff 2018-09-17 14:47:01 CEST
These name vs id changes break the univention-usercert cool solution.

Do not change the "api" of /usr/share/univention-ssl/make-certificates.sh, just add things that are necessary for this bug

revoke_cert $NAME
=> revokes all certs for this name

renew_cert
=> just create a new certficate

 

(1)
We shouldn't change the parameters of 
gencert
revoke_cert
renew_cert
has_valid_cert

(2)
revert the tests changes (e.g.
-revoke_cert "${name}"
+revoke_cert "$(has_valid_cert "${R64}")"
)

(3)
remove revoke-non-interactive, revoke $name revokes all certificates for that name, revoke -id $id revoke just this particular certficate
Comment 18 Felix Botner univentionstaff 2018-09-20 16:21:02 CEST
I would suggest to revert everything and start from scratch with only those changes that are really necessary (i have the impressing that there are now changes that were required only for the first approach)

What we want:

 * unique_subject = yes
 * revoke -> revoke all certs for given name
 * revoke -id -> revoke cert for given ID only
 * renew -> just create a new cert, no revoke 
 * do not change test unless it is really necessary
 * add tests for the new functionality 
 * the api of make-certificates.sh/univention-certificate should be 
   backwards compatible as much as possible

QA:

Please also test the usercert cool solution.
Comment 19 Philipp Hahn univentionstaff 2018-09-20 16:28:14 CEST
(In reply to Felix Botner from comment #18)
> I would suggest to revert everything and start from scratch with only those
> changes that are really necessary (i have the impressing that there are now
> changes that were required only for the first approach)
> 
> What we want:
> 
>  * unique_subject = yes

NO: This bug is about the "transition" period: for that time period you have two (or more) valid certificates with the same name. As such the subject is NOT unique!
Comment 20 Felix Botner univentionstaff 2018-09-20 16:35:32 CEST
(In reply to Philipp Hahn from comment #19)
> (In reply to Felix Botner from comment #18)
> > I would suggest to revert everything and start from scratch with only those
> > changes that are really necessary (i have the impressing that there are now
> > changes that were required only for the first approach)
> > 
> > What we want:
> > 
> >  * unique_subject = yes
> 
> NO: This bug is about the "transition" period: for that time period you have
> two (or more) valid certificates with the same name. As such the subject is
> NOT unique!

yes sorry, that is a type, we want unique_subject = no
Comment 21 Jannik Ahlers univentionstaff 2018-09-24 15:53:24 CEST
Successful build
Package: univention-ssl
Version: 12.0.0-12A~4.3.0.201809241545
Branch: ucs_4.3-0
Scope: errata4.3-2

univention-ssl (12.0.0-12)
a50846baca2a | Bug #41013: completely rewrote the changes for the bug. univention-certificate renew now doesn't revoke the old cert anymore. certs can now be addressed by their serial number by using the -id flag. adapted some tests, expanded one to test the new feature.

As Felix suggested, i completely rewrote the patch.
I had to edit some of the tests in the tests folder, as otherwise the package won't build.
The cool solution seems to work as well.
Comment 23 Philipp Hahn univentionstaff 2018-09-26 10:59:36 CEST
FIXED:
[4.3-2] d2179984b5 Bug #41013: univention-ssl 12.0.0-12A~4.3.0.201809241545
 doc/errata/staging/univention-ssl.yaml | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

(In reply to Felix Botner from comment #18)
OK: unique_subject = yes
OK: revoke -> revoke all certs for given name
OK: revoke -id -> revoke cert for given ID only
OK: renew -> just create a new cert, no revoke 
OK: do not change test unless it is really necessary
OK: add tests for the new functionality
    PATH=$PATH:/usr/sbin: run-parts -v --regex='^test.*' tests
OK: the api of make-certificates.sh/univention-certificate should be backwards compatible as much as possible

OK: Please also test the usercert cool solution.
OK: univention-usercert
OK: univention-windowscert
OK: <https://wiki.univention.de/index.php/Cool_Solution_-_Creation_and_management_of_user_and_Windows_certificates>

OK: git diff --stat 68492d144d72ec89d176815dde61360f22bb1d13.. -- base/univention-ssl test/ucs-test/tests/01_base
OK: Jenkins
Comment 24 Erik Damrose univentionstaff 2018-09-26 13:24:27 CEST
<http://errata.software-univention.de/ucs/4.3/244.html>