Last Comment Bug 127960 - SSL force handshake function should take a timeout
: SSL force handshake function should take a timeout
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4
: All All
: P2 enhancement (vote)
: 3.11.4
Assigned To: Nelson Bolyard (seldom reads bugmail)
: Jason Reid
: 124337 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2002-02-26 17:51 PST by Julien Pierre
Modified: 2006-11-14 16:19 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout (9.03 KB, patch)
2005-09-02 18:25 PDT, Julien Pierre
nelson: review+
Details | Diff | Splinter Review
patch to nss.def for the 3.11 branch (535 bytes, patch)
2006-11-03 20:42 PST, Nelson Bolyard (seldom reads bugmail)
alvolkov.bgs: review+
wtc: superreview+
Details | Diff | Splinter Review

Description Julien Pierre 2002-02-26 17:51:56 PST
In the SSL code, when forcing an SSL handshake to occur through
SSL_ForceHandshake / SSL_ReHandshake / SSL_ReDoHanshake, the SSL library uses
the timeout argument from the previous PR_Recv or PR_Send operation done on the
socket. In some cases, such as for the client auth in the web server, we want to
control the handshake timeout separately from the regular socket I/O timeout. We
would like to have a new API to force the handshake to occur, taking an extra
timeout argument.
Comment 1 Julien Pierre 2002-02-27 15:23:26 PST
*** Bug 124337 has been marked as a duplicate of this bug. ***
Comment 2 Julien Pierre 2002-02-27 17:08:53 PST
Actually, this argument would not truly accomplish what's needed here. It would
only work for forced handshakes. In the case of the first full handshake, there
would be no way to control it. The initial PR_Recv timeout would probably be
used. I don't know if this is desireable or not.
I think an option set via SSL_OptionSet that would work for all cases would be
better. If that option is not set, the current behavior would apply. Otherwise
the set timeout would be used.

Comment 3 Wan-Teh Chang 2002-04-25 16:30:40 PDT
Changed the QA contact to Bishakha.
Comment 4 Wan-Teh Chang 2002-05-22 12:02:51 PDT
Target 3.6, priority P2.
Comment 5 Julien Pierre 2005-04-29 14:42:32 PDT
Unsetting target.
Comment 6 Julien Pierre 2005-09-02 18:25:17 PDT
This bug has been open for years. We have several internal customers interested
in the fix .

There are two problems with the approaches I suggested earlier

1) The type for the value of SSL socket options is declared as a PRBool.

This makes it somewhat ugly to implement an SSL option with a timeout value,
which would need to be a PRIntervalTime, itself declared as PRUint32 . 

A PRBool is a PRIntn, which is a platform-specific int, and has a minimum
requirement of only 2 bytes in prtypes.h . I believe there are no longer any
useful platforms that have less than 32-bit integers, but it would still be
preferable to have an API that took the right type for the value of the SSL
option, or maybe simply a void*, which would always be good enough for any kind
of option we set in the future.

2) the rTimeout and wTimeout within the SSL socket structure are constantly
being reset by NSPR I/O calls . If we had a global timeout property exclusively
for handshakes, we would need to change the SSL I/O code to figure out if the
I/O is happening within a handshake. This is quite complicated to do.

Because of these issues, I'm taking the simpler approach of just adding two new
ReHandshake / ForceHandshake APIs with the additional PRIntervalTime argument.
The 2 new APIs  will simply set the timeout in the SSL structure prior to
calling the old functions. I'm not providing an enhancement version of
SSL_ReDoHandshake, since that API has been marked as obsolete.

Patch forthcoming.
Comment 7 Julien Pierre 2005-09-02 18:25:57 PDT
Created attachment 194729 [details] [diff] [review]
Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout
Comment 8 Wan-Teh Chang 2005-09-06 14:31:40 PDT
If the interface of SSL_OptionSet is not approriate for
non-Boolean options, you can add new functions to set the
option, e.g., SSL_SetHandshakeTimeout.

In any case, I'll let you and Nelson decide the proper fix.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2005-09-15 19:56:11 PDT
Comment on attachment 194729 [details] [diff] [review]
Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout

This patch adds two new public SSL functions.  I'd like more time to think
about the correctness of the implementation, but I believe the function names
and signatures are fine as is, so I'm OK with adding them to libSSL for 3.11.
Comment 10 Julien Pierre 2005-09-16 13:34:05 PDT
Thanks for the review, Nelson. I have checked in the patch so that these new
APIs don't miss the beta . It would be harder to add them afterwards. If there
is a problem with the implementation, I can produce another patch.

Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.41; previous revision: 1.40
Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.33; previous revision: 1.32
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.41; previous revision: 1.40
Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.24; previous revision: 1.23
Comment 11 Julien Pierre 2005-09-16 14:29:07 PDT
The initial checkin broke the build. I have checked in an update. Sorry about that.

Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.42; previous revision: 1.41
Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.34; previous revision: 1.33
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.42; previous revision: 1.41
Comment 12 Nelson Bolyard (seldom reads bugmail) 2005-09-28 17:48:25 PDT
Julien, Is this bug now resolved/fixed?  
Comment 13 Julien Pierre 2005-09-28 17:55:31 PDT

You said in comment #10 that you wanted to provide comments about the
implementation. If you have none, you can mark the bug FIXED.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2006-11-03 20:31:06 PST
Reopening.  This new feature implementation was/is incomplete.
The new functions were not exported from the shared libraries. 
The new symbols need to be added to ssl.def.
When that is done, this bug can be marked resolved/fixed again.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2006-11-03 20:42:14 PST
Created attachment 244649 [details] [diff] [review]
patch to nss.def for the 3.11 branch

I set the release version number to 3.11.4, but maybe it should be 3.11.5 ?
Comment 16 Wan-Teh Chang 2006-11-04 07:40:41 PST
Comment on attachment 244649 [details] [diff] [review]
patch to nss.def for the 3.11 branch

NSS 3.11.4 is the right version number for NSS_3_11_BRANCH.
I guess the export of these two functions can't wait until
NSS 3.12.
Comment 17 Wan-Teh Chang 2006-11-04 08:00:32 PST
Comment on attachment 194729 [details] [diff] [review]
Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout

The use of locks in ssl_SetTimeout is strange.

By studying the use of SSL_LOCK_READER and
SSL_LOCK_WRITER, I concluded that they seem to
be intended for serializing ss->ops->recv and
ss->ops->send calls, respectively.

ssl_Recv may modify ss->wTimeout with only the

ssl_Send may modify ss->rTimeout with only the

also protect ss->rTimeout and ss->wTimeout is
not clear and cannot be inferred by existing

Also, since ssl_SetTimeout doesn't call ss->ops->recv
or ss->ops->send, Julien's decision to always call
is true seems ad hoc rather than following some
rule or existing usage.
Comment 18 Nelson Bolyard (seldom reads bugmail) 2006-11-14 16:19:34 PST
Checked in on trunk and NSS_3_11_BRANCH for NSS 3.11.4
Checking in ssl.def; new revision: 1.18;      previous revision: 1.17
Checking in ssl.def; new revision:; previous revision: 1.17

Note You need to log in before you can comment on or make changes to this bug.