811331 - Implement self-contained testing of OCSP STAPLING

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Kai Engert [:KaiE:]]

After the various dependency bugs are done,

once we have a selfserv that implements OCSP stapling, 

we should replace the tests from bug 360420 (currently in proposed state), that depend on external well known HTTPS servers that implement OCSP stapling.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: patch for round 36

This patch applies on top of the patches "round 36" from bug 360420.

Some review comments from Wan-Teh (in bug 360420) have already been addressed in this patch.

Comment 2

[Kai Engert [:KaiE:]]

Note this patch also implements the server side of the SSL OCSP stapling protocol.

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 701838 [details] [diff] [review]
patch for round 36

I'm going to split this patch into two pieces.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: round 36 - part 5 - OCSP stapling server side protocol

This patch implements the libssl server side of OCSP stapling, this is what you had asked for in bug 360420 comment 141.

Comment 5

[Kai Engert [:KaiE:]]

Attachment: [for easy reviewing] [ignoring whitespace changes] round 36 part 5

Note there is one block of code that LOOKS like there many removals and additions.

This whitespace-ignoring diff shows, the existing block in CERT_CreateEncodedOCSPSuccessResponse remains unchanged - only new code being added.

Comment 6

[Kai Engert [:KaiE:]]

Attachment: round 36 - part 6 - server side tools and testing enhancements

Note - the two patches I've attached - part 5 and part 6 - are identical to the earlier combined patch.

Comment 7

[Robert Relyea]

Comment on attachment 703310 [details] [diff] [review]
round 36 - part 5 - OCSP stapling server side protocol

r+ but I have a question. You send the certificate status in both the server and cient handshakes. Was this intentional, and if so, should we change or clarify the comment above ssl3_Send CertificateStatus?

Comment 8

[Robert Relyea]

Comment on attachment 703314 [details] [diff] [review]
round 36 - part 6 - server side tools and testing enhancements

r+ to answer your question abiutbthe extended test.... its ok to skip them. We can revisit this when we wnat tonteapst multistapling.

Comment 9

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #7)
> Comment on attachment 703310 [details] [diff] [review]
> round 36 - part 5 - OCSP stapling server side protocol
> 
> r+ but I have a question. You send the certificate status in both the server
> and cient handshakes. Was this intentional

No, thanks for catching this mistake.
We don't want to send a cert status after sending the client side sends a client cert.

I'll remove this chunk from patch part 5:

@@ -6184,16 +6185,20 @@ ssl3_SendClientSecondRound(sslSocket *ss
 	if (rv != SECSuccess) {
 	    goto loser;	/* error code is set. */
     	}
     } else if (sendClientCert) {
 	rv = ssl3_SendCertificate(ss);
 	if (rv != SECSuccess) {
 	    goto loser;	/* error code is set. */
     	}
+	rv = ssl3_SendCertificateStatus(ss);
+	if (rv != SECSuccess) {
+	    return rv;	/* error code is set. */
+	}
     }

Comment 10

[Kai Engert [:KaiE:]]

Attachment: round 40 - part 5 - OCSP stapling server side protocol

Removed the incorrect duplicate block, as identified by Bob.
Otherwise identical to the patch that Bob gave r+ to

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 711547 [details] [diff] [review]
round 40 - part 5 - OCSP stapling server side protocol

Checking in security/nss/lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.79; previous revision: 1.78
done
Checking in security/nss/lib/certhigh/ocspi.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.14; previous revision: 1.13
done
Checking in security/nss/lib/certhigh/ocspsig.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspsig.c,v  <--  ocspsig.c
new revision: 1.10; previous revision: 1.9
done
Checking in security/nss/lib/ssl/ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.42; previous revision: 1.41
done
Checking in security/nss/lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.62; previous revision: 1.61
done
Checking in security/nss/lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.204; previous revision: 1.203
done
Checking in security/nss/lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.32; previous revision: 1.31
done
Checking in security/nss/lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.112; previous revision: 1.111
done
Checking in security/nss/lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.102; previous revision: 1.101
done
Checking in security/nss/lib/util/nssutil.def;
/cvsroot/mozilla/security/nss/lib/util/nssutil.def,v  <--  nssutil.def
new revision: 1.20; previous revision: 1.19
done
Checking in security/nss/lib/util/secitem.c;
/cvsroot/mozilla/security/nss/lib/util/secitem.c,v  <--  secitem.c
new revision: 1.20; previous revision: 1.19
done
Checking in security/nss/lib/util/secitem.h;
/cvsroot/mozilla/security/nss/lib/util/secitem.h,v  <--  secitem.h
new revision: 1.11; previous revision: 1.10
done

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 703314 [details] [diff] [review]
round 36 - part 6 - server side tools and testing enhancements

Checking in security/nss/cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.104; previous revision: 1.103
done
Checking in security/nss/cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.75; previous revision: 1.74
done
Checking in security/nss/cmd/tstclnt/tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.75; previous revision: 1.74
done
Checking in security/nss/tests/all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.60; previous revision: 1.59
done
Checking in security/nss/tests/cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.64; previous revision: 1.63
done
Checking in security/nss/tests/common/init.sh;
/cvsroot/mozilla/security/nss/tests/common/init.sh,v  <--  init.sh
new revision: 1.76; previous revision: 1.75
done
Checking in security/nss/tests/dbupgrade/dbupgrade.sh;
/cvsroot/mozilla/security/nss/tests/dbupgrade/dbupgrade.sh,v  <--  dbupgrade.sh
new revision: 1.6; previous revision: 1.5
done
Checking in security/nss/tests/ocsp/ocsp.sh;
/cvsroot/mozilla/security/nss/tests/ocsp/ocsp.sh,v  <--  ocsp.sh
new revision: 1.5; previous revision: 1.4
done
Checking in security/nss/tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.116; previous revision: 1.115
done

Comment 13

[Kai Engert [:KaiE:]]

fixed