9.03 KB, patch
Nelson Bolyard (seldom reads bugmail): review+
|Details | Diff | Splinter Review|
535 bytes, patch
Alexei Volkov: review+
Wan-Teh Chang: superreview+
|Details | Diff | Splinter Review|
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.
*** Bug 124337 has been marked as a duplicate of this bug. ***
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.
Changed the QA contact to Bishakha.
Target 3.6, priority P2.
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.
Created attachment 194729 [details] [diff] [review] Add SSL_ReHandshakeWithTimeout and SSL_ForceHandshakeWithTimeout
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 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.
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 done Checking in sslsecur.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v <-- sslsecur.c new revision: 1.33; previous revision: 1.32 done Checking in sslsock.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c new revision: 1.41; previous revision: 1.40 done Checking in ssl.h; /cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v <-- ssl.h new revision: 1.24; previous revision: 1.23 done
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 done Checking in sslsecur.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v <-- sslsecur.c new revision: 1.34; previous revision: 1.33 done Checking in sslsock.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c new revision: 1.42; previous revision: 1.41 done
Julien, Is this bug now resolved/fixed?
Nelson, You said in comment #10 that you wanted to provide comments about the implementation. If you have none, you can mark the bug FIXED.
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.
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 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 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_LOCK_READER held. ssl_Send may modify ss->rTimeout with only the SSL_LOCK_WRITER held. So whether SSL_LOCK_READER and SSL_LOCK_WRITER also protect ss->rTimeout and ss->wTimeout is not clear and cannot be inferred by existing usage. Also, since ssl_SetTimeout doesn't call ss->ops->recv or ss->ops->send, Julien's decision to always call SSL_LOCK_READER and call SSL_LOCK_WRITER if ss->fdx is true seems ad hoc rather than following some rule or existing usage.
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: 188.8.131.52; previous revision: 1.17