1057128 - generate_certs.sh shouldn't clobber existing certs that don't need to change

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

See bug 1034124 comment 9 and bug 1009161 comment 18, wherein it is pointed out that additions to generate_certs.sh that involve things like adding tests shouldn't affect other tests like pinning (if everything gets regenerated, then the key hash for the pinning tests need to be updated, which involves re-generating the built-in key pinning data, which is unnecessary).

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Richard, do you have time to review this? Thanks. (This is the script that generates certificates for our tlsserver-involved xpcshell tests, if you weren't already familiar with it.)

Comment 2

[Richard Barnes [:rbarnes]]

Comment on attachment 8477100 [details] [diff] [review]
patch

Review of attachment 8477100 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of minor things.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +35,5 @@
> +if [ "${3}" == "--clobber" ]; then
> +  CLOBBER=1
> +else
> +  CLOBBER=0
> +fi

Might be slightly clearer to reorder as 

> CLOBBER=0
> if [...]; then CLOBBER=1

@@ +72,5 @@
> +  if [ $CLOBBER -eq 1 ]; then
> +    echo "Found pre-existing NSS DBs. Clobbering old OCSP certs."
> +    rm -f "$OUTPUT_DIR/cert9.db" "$OUTPUT_DIR/key4.db" "$OUTPUT_DIR/pkcs11.txt"
> +    $RUN_MOZILLA $CERTUTIL -d $DB_ARGUMENT -N -f $PASSWORD_FILE
> +  fi

Should there be a message here (as below) to let the user know that old stuff is persisting?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch with comments addressed

Great - thanks! Addressed comments, carrying over r+.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a25b15c2f8f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a25b15c2f8f

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch for beta (33)

This is needed for bug 1009161.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8483766 [details] [diff] [review]
patch for beta (33)

[Triage Comment]

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f1e228fac54