Closed
Bug 226271
Opened 21 years ago
Closed 18 years ago
implement RFC 3546 (TLS v1.0 extensions)
Categories
(NSS :: Libraries, enhancement, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.1
People
(Reporter: bugzilla, Assigned: nelson)
References
()
Details
(Whiteboard: ECC)
Attachments
(2 files, 1 obsolete file)
4.48 KB,
patch
|
julien.pierre
:
review+
rrelyea
:
review+
|
Details | Diff | Splinter Review |
15.48 KB,
patch
|
vipul.gupta
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.6a) Gecko/20031025 Build Identifier: RFC 3546 (from June 2003) describes extensions to TLS v1.0 that are very usable, of which the most important seems to be: - Allow TLS clients to provide to the TLS server the name of the server they are contacting. This functionality is desirable to facilitate secure connections to servers that host multiple 'virtual' servers at a single underlying network address. it is now difficult/impossible to host multiple hosts on the same IP if you want to use SSL. this extension makes that possible. i'm not sure whether someone has already implemented this server-side to test it on. Reproducible: Always Steps to Reproduce: 1. 2. 3.
Comment 1•21 years ago
|
||
To NSS.
Assignee: ssaux → wchang0222
Component: Client Library → Libraries
Product: PSM → NSS
QA Contact: bmartin → bishakhabanerjee
Comment 2•20 years ago
|
||
Bug 116168 was filed as an RFE for the "server name indication" extension . I marked it as depending on this bug, rather than making this a dupe This bug should be used for adding support to all the extensions in RFC3546, not just the server name indication.
Assignee | ||
Updated•19 years ago
|
QA Contact: bishakhabanerjee → jason.m.reid
Updated•19 years ago
|
Assignee: wtchang → nelson
Priority: -- → P1
Target Milestone: --- → 3.12
Assignee | ||
Comment 3•19 years ago
|
||
I think it is possible that we will implement some, but not all, of the extensions defined in that RFC, Just as we do not implement all of the defined cipher suites. As always, immediate utility in the mozilla client products, and in the various NSS-based servers is likely to drive this work. I will use this bug to drive the addition of a general TLS extension framework, to which code for specific extensions can be added. I expect the framework will define several new callbacks for libSSL. I expect to begin that work soon.
Assignee | ||
Updated•18 years ago
|
Target Milestone: 3.12 → 3.11.1
Version: unspecified → 3.11
Assignee | ||
Updated•18 years ago
|
Whiteboard: ECC
Assignee | ||
Updated•18 years ago
|
QA Contact: jason.m.reid → libraries
Assignee | ||
Comment 4•18 years ago
|
||
This is preparatory work for the TLS ECC hello extensions.
Attachment #217085 -
Flags: superreview?(rrelyea)
Attachment #217085 -
Flags: review?(julien.pierre.bugs)
Comment 5•18 years ago
|
||
Comment on attachment 217085 [details] [diff] [review] Add SSL error codes and alerts for TLS extensions The name of this error has a typo : +ER3(SSL_ERROR_CERTIFICATE_UNAVAILALBLE_ALERT , (SSL_ERROR_BASE + 105), Also in : + certificate_unavailalble = 111, +SSL_ERROR_CERTIFICATE_UNAVAILALBLE_ALERT = (SSL_ERROR_BASE + 105), This is not a comment on this patch, but why do we have two copies of SSLerrs.h ? Can't SSLsample just use the file from cmd/lib ?
Attachment #217085 -
Flags: review?(julien.pierre.bugs) → review-
Comment 6•18 years ago
|
||
Comment on attachment 217085 [details] [diff] [review] Add SSL error codes and alerts for TLS extensions Also, RFC3546 uses "certificate_unobtainable" as the name for alert 111 rather than "certificate_unavailable" . We should probably use the same name as the RFC.
Assignee | ||
Comment 7•18 years ago
|
||
Well, at least I was consistent with my typos! :) Thanks for catching those.
Attachment #217085 -
Attachment is obsolete: true
Attachment #217090 -
Flags: review?(julien.pierre.bugs)
Attachment #217085 -
Flags: superreview?(rrelyea)
Assignee | ||
Comment 8•18 years ago
|
||
This second patch also changed some undesirable tabs to spaces. In answer to your questoin about why two copies of sslerrs.h, the answer is: ask the person who wrote sslsample. Fixing sslsample is about P5 right now.
Updated•18 years ago
|
Attachment #217090 -
Flags: review?(julien.pierre.bugs) → review+
Assignee | ||
Comment 9•18 years ago
|
||
Comment on attachment 217090 [details] [diff] [review] patch v2 (checked in) request second review for 3.11 branch.
Attachment #217090 -
Flags: review?(rrelyea)
Assignee | ||
Comment 10•18 years ago
|
||
Comment on attachment 217090 [details] [diff] [review] patch v2 (checked in) cmd/SSLsample/SSLerrs.h; new revision: 1.3; previous revision: 1.2 cmd/lib/SSLerrs.h; new revision: 1.4; previous revision: 1.3 lib/ssl/ssl3prot.h; new revision: 1.12; previous revision: 1.11 lib/ssl/sslerr.h; new revision: 1.5; previous revision: 1.4
Attachment #217090 -
Attachment description: patch v2 → patch v2 (checked in)
Assignee | ||
Comment 11•18 years ago
|
||
This patch adds: - handling of new alerts - ssl3_CallHelloExtensionSenders() which goes through table of registered functions to format extensions, and calls them to append their extensions, or to merely report the length they *would* append if asked to do so. This function gets called twice, once to get the lengths, and again to append. This function is used to send both client and server hello extensions. It is called for client hellos ONLY if TLS is enabled. Presently, the table of senders of client hello extensions is static. Dynamic registration is not done. But dynamic registration *is* done for the server side. - ssl3_HandleClientHelloExtensions(), goes through the received client hello extensions, distributing them to functions registered for each type, if any. Presently the table of registerd client hello extension handlers is static. There is not yet a table of server hello extension handlers. Server hello extensions are ignored. - ssl3_SendServerNameIndicationExtension() formats and appends an SNI extension only if ECC cipher suites are enabled. Uses the "URL" name that the app registers with libSSL, provided that it is not a dotted decimal value. - ssl3_HandleServerNameIndicationExtension() handles a received SNI extension by discarding it. (Room for enhancement here. :) - ssl3_RegisterServerHelloExtensionSender() allows dynamic registration of sender functions for server hello extensions. Used to send replies only to the extensions received from the client. - ssl3_AppendHandshakeNumber() and ssl3_ConsumeHandshakeNumber() are no longer static to ssl3con.c, so that the extension senders and handlers in ssl3ecc.c can use them. I will ask Vipul and Bob to review, but others are welcome.
Attachment #217133 -
Flags: superreview?(rrelyea)
Attachment #217133 -
Flags: review?(vipul.gupta)
Comment 12•18 years ago
|
||
Comment on attachment 217133 [details] [diff] [review] Generic extension implementation, v1 (checked in) Due to time constraints, I haven't been able to spend a lot of time on the review. Having said that, here are a few questions/comments: 1. In this pach, the extension handler doesn't really do much with the extension (it ignores it). Typically, the extension appended to the ServerHello will depend on the extensions included with the incoming Client Hello. Am I correct in assuming that any state that needs to be saved between the time the incoming extension is processed and the response extension is generated will be stored in the sslSocket structure? 2. What happens if sslv2 is enabled? Will a ClientHello in v2 format still carry the TLS extensions (SSLv2 does not have a notion of hello extensions). 3. Typo in one of the comments inside ssl3_HandleClientHelloExtensions: "find extension_type *in* (instead of *is*)" vipul
Attachment #217133 -
Flags: review?(vipul.gupta) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #217090 -
Attachment description: patch v2 (checked in) → patch v2 (checked in on trunk only)
Assignee | ||
Comment 13•18 years ago
|
||
Comment on attachment 217133 [details] [diff] [review] Generic extension implementation, v1 (checked in) Checked in, with Vipul's suggested changes, on trunk only. Checking in ssl3con.c; new revision: 1.85; previous revision: 1.84 Checking in ssl3ecc.c; new revision: 1.6; previous revision: 1.5 Checking in sslimpl.h; new revision: 1.48; previous revision: 1.47
Attachment #217133 -
Attachment description: Generic extension implementation, v1 → Generic extension implementation, v1 (checked in on trunk)
Comment 14•18 years ago
|
||
Comment on attachment 217090 [details] [diff] [review] patch v2 (checked in) r+=relyea
Attachment #217090 -
Flags: review?(rrelyea) → review+
Comment 15•18 years ago
|
||
Comment on attachment 217133 [details] [diff] [review] Generic extension implementation, v1 (checked in) r=relyea Minor comments, do not need to be fixed before checkin.. > PRUint32 maxBytes = 65535; /* 2^16 - 1 */ > PRInt32 extLen; > > extLen = ssl3_CallHelloExtensionSenders(ss, PR_FALSE, maxBytes, NULL); > if (extLen < 0) { > return SECFailure; > } > > maxBytes -= extLen; > The subtraction above is unnecessary as maxBytes goes out of scope in 3 more lines without being used. --- At some point it would be nice to allow the register function to be accessible to the application. At that point we would have to make the static array dynamic. That is not the goal of this code, so the comment does not indicate a need to change anything. bob
Attachment #217133 -
Flags: superreview?(rrelyea) → superreview+
Assignee | ||
Comment 16•18 years ago
|
||
Comment on attachment 217090 [details] [diff] [review] patch v2 (checked in) Checked in on branch cmd/SSLsample/SSLerrs.h; new revision: 1.2.28.1; previous revision: 1.2 cmd/lib/SSLerrs.h; new revision: 1.3.28.1; previous revision: 1.3 lib/ssl/ssl3prot.h; new revision: 1.10.2.2; previous revision: 1.10.2.1 lib/ssl/sslerr.h; new revision: 1.4.28.1; previous revision: 1.4
Attachment #217090 -
Attachment description: patch v2 (checked in on trunk only) → patch v2 (checked in)
Assignee | ||
Comment 17•18 years ago
|
||
Comment on attachment 217133 [details] [diff] [review] Generic extension implementation, v1 (checked in) Checked in on 3.11 branch. Checking in ssl3con.c; new revision: 1.76.2.9; previous revision: 1.76.2.8 Checking in ssl3ecc.c; new revision: 1.3.2.3; previous revision: 1.3.2.2 Checking in sslimpl.h; new revision: 1.42.2.5; previous revision: 1.42.2.4
Attachment #217133 -
Attachment description: Generic extension implementation, v1 (checked in on trunk) → Generic extension implementation, v1 (checked in)
Assignee | ||
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•