360421 - Implement TLS Server Name Indication for servers

Status: NEW

(NSS :: Libraries, enhancement, P4)

Description

[Nelson Bolyard (seldom reads bugmail)]

RFC 4366 defines a hello extension for TLS, named "Server Name Indication" 
or "SNI" for short.  NSS implements SNI for TLS clients.  It does not yet
implement SNI for servers.  This RFE asks that NSS implement SNI for servers.

Comment 1

[Alexei Volkov]

Attachment: initial patch - basic implementation. Testing in progress. Not for review.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

What happened to doing an API change review first?

Comment 3

[Alexei Volkov]

Nelson,
I wanted to make sure I'm not missing anything. Let's review the ssl.h file tomorrow. Whole api code is over there. Thanks, Alexei.

Comment 4

[Chris Newman]

From a usability viewpoint, I really dislike callback functions.  They're flexible, but require maintaining a lot of wrapper code to make the API comprehensible to the average server programmer and are rarely needed for the common case.

While the proposed callback model is very general, I'd expect 99% of all servers don't need that generality.  Why not make it easier for those servers to implement this correctly?

As an NSS consumer, I'd prefer an API like:

SSL_ConfigMultiHostServer(PRFileDesc *model, const char **hosttokennick,
  const char *defslot, const char *defnickname);

to use as a replacement for SSL_ConfigSecureServer().

Where "hosttokennick" is a NULL terminated array of string tuples translating from a hostname to a PK11 slot name and certificate nickname.  The defslot and defnickname parameters are the certificate to use if no SNI comes from the client.

I already have to have a wrapper module around NSS/NSPR to make it actually comprehensible and safe to use by the typical server implementer (doing things like turning off export grade ciphers by default, loading certificates, etc).  Anything that makes that wrapper module smaller (or even better makes it go away) is a positive change in my opinion.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Chris, 
You're suggesting an alternative in which the application pre-registers with 
the SSL library a list of certs and their private keys, and lets the SSL 
library do all the work.  That idea is not mutually exclusive with the 
present proposal.  It's possible to do them both.  

We've considered the preconfiguration possibility before.  It has the 
following issues:

1. The server application still needs some way to ask the SSL library which
DNS name was requested by the client, so that the application can configure
itself for that virtual host.

2. The certificates may contain wildcard names, and it may be that the 
server does not actually host all possible names that match the wildcard.  
For example, the cert may be for *.foo.com, and the server may implement 
smtp.foo.com, imap.foo.com, pop.foo.com and nntp.foo.com, but not mail.foo.com
The SSL library would have no way of knowing that, so either the SSL library
does handshakes for virtual hosts that are not actually offered, or else it 
is necessary for the server to tell the SSL library all the names of all the
virtual hosts that it serves.  

3. This doesn't scale very well.  It may be OK for (say) up to 50 virtual 
host names, but when you get into situations where there are thousands, or 
tens or hundreds of thousands of virtual host names, having to preconfigure
them all wastes memory.  

Perhaps the solution is to implement both, and set a limit on the number of
host names that can be registered through pre-configuration.  Still, there
needs to be a way for the application to ask: "OK, so which host name did 
you claim to be in that handshake?"

Comment 6

[Chris Newman]

I agree it's possible to do both; the callback is a fine API for the 1% uncommon case.

1. agreed -- not all applications care, but most do.  The ones that do would certainly prefer a "what domain name did the client specify" API to a nasty callback where they have to squirrel away data in some void * context and read it back later.
2. my strawman (tuples) works fine -- some of the tuples can have different host names pointing to the same slot name and certificate nickname.
3. It depends on the implementation.  An implementation which digs all the possible certificates and keys out of their respective tokens at startup does not scale.  But just holding a copy of an array (or hash) of tuples isn't going to create any significant barriers to scalability on a virtual hosting server as long as access to the actual certificates and keys is deferred until needed.

I do accept that is it harder to design and implement a "common case is easy-to-use, uncommon case is possible" API then a "minimally support all the cases" API.  I just wanted to speak in favor of the former.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Chris, 
Please describe for us the case(s) that you think will commonly be of interest 
to your users.  In particular, I wonder about the number of virtual hosts 
and certificates for those virtual hosts that you think might be typical,
and/or maximal for an installation of your products. 

We frequently hear from people who run large scale web & mail hosting 
services.  They often host thousands of virtual domains from a single 
physical host.  They tell us that they'd like to be able to have separate
certificates for each virtual host (or set of hosts in a virtual domain).

I have the impression that your product is commonly used in large scale 
deployments, so I expected that the concerns of those virtual domain hosting
services would be similar to your own.  Are they not?

Comment 8

[Alexei Volkov]

Attachment: ssl api extension to handle SNI

Includes declaration of the SSL_ConfigMultiHostServer that can be used to configure ssl server with basic parameters(server name, cert, privater key) for  multiple virtual names support.

Comment 9

[Alexei Volkov]

Attachment: Patch v1 - initial patch.

Initial patch for handling SNI tls extension.
Passed all.sh

Comment 10

[Alexei Volkov]

Attachment: libSSL api extension to handle SNI

Chris, please review.

Comment 11

[Chris Newman]

Review of attachment id 415432:

Thank you for your work on this, looks good!

Design:

I like the two-level design where there's a low-level power user model (the callback) and a higher level API suitable for the common case (that uses and thus tests the lower level API).  This is an excellent API design, IMHO.

It's a bit unclear at first glance that "srvNames" and "srvNamesLen" is an array and array count.  I like the name "srvNameArr" and "srvNameArrLen" better.

I wonder if the callback should distinguish between an "unrecognized_name" alert and some sort of internal server error (e.g. permission error trying to open the token in the callback or whatnot).  I wonder if clients will behave differently if they get an "unrecognized_name" than if they get some other error.  If it would be good for them to do so, it would be good for the server to distinguish.
If not, then good diagnostic logging when NSS debugging is turned on would be sufficient.

It might be helpful to say that SSL_ConfigMultiHostServer is called multiple times, once for each triplet.

One other question that's a bit unclear from the API design.  As a server implementer, I will sometimes listen on multiple TCP/IP interfaces and have different default certificates for different interfaces.  Would I just call SSL_ConfigMultiHostServer() on the post-accept PRFileDesc to change the default based on the interface?

Nits:

I note one inconsistency:

+typedef PRInt32 (PR_CALLBACK *SSLSNISocketConfig)(PRFileDesc *fd,
+                                            const SECItem *srvNames,
+                                                  PRUint32 srvNamesLen,
+                                                  void *arg);
                                                            ^^^^^^^^^^^

+** SSLSNISocketConfig should return an index within 0 and srvNameArrLen-1
                                                           ^^^^^^^^^^^^^

Are these two identifiers meant to be the same?


This looks like a cut&paste error:

+** for a specific name. Otherwise it may return two values that indicate
+** for a specific name. There are two other allowed return values. One
+** values are:*/

Spelling error:

+** Set application implemented SNISockeConfig callback.
                                   ^^^^^^
                                   Socket

Clarity:

+** "slotName". Caller may specify multiple certificates for the same virtual
+** name, such as RSA, DSA, or ECC.

Try:
    Caller may specify multiple certificates for the same srvName as long as
    no two certificates use the same algorithm (e.g., RSA, DSA, ECC).
(I think that's correct.  My understanding of NSS is a bit fuzzy in this area.)

Q: if two certificates with the same algorithm are provided, what is the behavior?  First wins, last wins, undefined or error when setting the duplicate?


Awkward wording:

+** extension. In case when SNI extension was used but no match
+** were found, this server configuration will send "unrecognized_name" alert.

Try:
               When the SNI extension was used but no match was found, the
    server configuration will send an unrecognized_name alert.


Spelling:

+** Use this function to uncofigure server for a specific names. To do this
                         ^^^^^^^^^^
alternative wording:
    A specific srvName may be removed from the supported list by calling this
    function with a NULL slotName or certName.

Grammar:

 ** Configure a secure servers session-id cache. Define the maximum number
                       ^^^^^^^
                       server's

Comment 12

[Alexei Volkov]

Thanks for the review, Chris.

> It's a bit unclear at first glance that "srvNames" and "srvNamesLen" is an
> array and array count.  I like the name "srvNameArr" and "srvNameArrLen"
> better.
Agreed. These looks more readable. Will use them.

> I wonder if the callback should distinguish between an "unrecognized_name"
> alert and some sort of internal server error (e.g. permission error trying to
> open the token in the callback or whatnot).  I wonder if clients will behave
> differently if they get an "unrecognized_name" than if they get some other
> error.  If it would be good for them to do so, it would be good for the server
> to distinguish.
> If not, then good diagnostic logging when NSS debugging is turned on would be
> sufficient.
From what I read in RFC, if SNI extension is present in client hello, when there are only three ways how the server can inform the client about the state of the exchange with regards to this extension. They are: SNI extension in server hello(server name accepted and the socket is reconfigured for the name), no extension(extension is not recognized), or unrecognized_name fatal or non-fatal alert. Client wont be able to correlate other responses to the name that was used in the extension. I'm not sure that it would be valuable to send any other alerts.

> 
> It might be helpful to say that SSL_ConfigMultiHostServer is called multiple
> times, once for each triplet.
Will do.
 
> One other question that's a bit unclear from the API design.  As a server
> implementer, I will sometimes listen on multiple TCP/IP interfaces and have
> different default certificates for different interfaces.  Would I just call
> SSL_ConfigMultiHostServer() on the post-accept PRFileDesc to change the default
> based on the interface?
How about this adding another function the the API set?
/*
** Changes default configuration of the socket model if socket modes was
** configured for multi-hosting by use of SSL_ConfigMultiHostServer function.
**/
SSL_IMPORT SECStatus SSL_SetDefaultConfig(PRFileDesc *model,
                                    const char *      srvName);


> 
> +** "slotName". Caller may specify multiple certificates for the same virtual
> +** name, such as RSA, DSA, or ECC.
> 
> Try:
>     Caller may specify multiple certificates for the same srvName as long as
>     no two certificates use the same algorithm (e.g., RSA, DSA, ECC).
We will be able to handle multiple certs with the same nicks as long as whey are user certs, etc have corresponding private key.

> (I think that's correct.  My understanding of NSS is a bit fuzzy in this area.)
> 
> Q: if two certificates with the same algorithm are provided, what is the
> behavior?  First wins, last wins, undefined or error when setting the
> duplicate?
The last one wins. I'll added a few lines to the description.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Regarding the multiple interface issue, I question whether any new function
is needed.  This is why model sockets exist.  You'd have a separate model
socket for each interface, and use SSL_ImportFD to import the appropriate
model socket onto the newly accepted socket.  Hopefully the server is already
working this way.  

If it would hasten this process, perhaps we could have a conference call about 
this this afternoon, say, at 4 PM PST?  Please write me privately if interested.

Comment 14

[Alexei Volkov]

Attachment: Patch v2p1 - changes to ssl library

Comment 15

[Alexei Volkov]

Attachment: Patch v2p2 - changes to certhigh, nss utils and tests

Comment 16

[Alexei Volkov]

Attachment: Patch v3p1 - changes to ssl library

fix to pass stress tests

Comment 17

[Alexei Volkov]

Attachment: Patch v4p1 - changes to ssl library

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 417507 [details] [diff] [review]
Patch v4p1 - changes to ssl library

I reviewed this patch with Alexei before Xmas.  I gave it a negative review 
at that time, but didn't mark it here until now.  There's one problem with 
it.  The following proposed new API function's arguments are insufficient 
to uniquely identify a cert within a slot.  

>+SSL_IMPORT SECStatus SSL_ConfigMultiHostServer(PRFileDesc *model,
>+                                               SSLSniNameType nameType,
>+                                         const char *      srvName,
>+                                         const char *      slotName,
>+                                         const char *      certName,
>+                                               PRBool      defCfg);

It's possible that the slot might contain multiple certs with different types 
of keys (RSA, ECC) all with the same nickname.  We need some way to identify 
a particular cert uniquely, yet one that doesn't necessarily occupy kilobytes 
of NSS memory for each cert (as a CERTCertificate would).  I think a PKCS#11
object handle is probably the most compact and portable identifier possible
and that's what I have suggested Alexei use instead of a nickname.  AIUI,
Alexei is going to run that idea by his internal "customers" for the new API.

Comment 19

[Alexei Volkov]

> It's possible that the slot might contain multiple certs with different types 
> of keys (RSA, ECC) all with the same nickname. 
Still, it can be possible to handle ambiguity when there are two certs of a different key types if we assume that in this case user wants to use both certificates in this case. This assumption leads us to enabling an extra set of cipher suites. I see why one can argue, that it is desirable, and I see it can not be suitable.
However, there is another situation, when there are two certs with the same nick that have same key type but with a different key size, or different validity time, or different key usages. In this case it is hard to predict what would be the correct cert. 

> I think a PKCS#11
> object handle is probably the most compact and portable identifier possible
> and that's what I have suggested Alexei use instead of a nickname.  
This was suggested as a way to internally store cert info for multi-host configuration. I'll plan to run an experiment to measure cert retrieval time.
But for API I would propose to use exact cert and key to configure a virtual server name. See example at http://mxr.mozilla.org/security/source/security/nss/lib/ssl/ssl.h#293.

Comment 20

[Alexei Volkov]

Attachment: v2 - libSSL api extension to handle SNI

Chris, please review these changes.

Cert nicknames are just a short cats to certs subject names. If we would use an earlier proposed API, then NSS wouldn't be able to correctly identify a cert that user intended to use without applying some critical assumption that may or may not be right depending on a case. So this proposal defines the way to configure multi-host data by providing a references to exact cert and key.

We will add another call to nss library that allow to find all certs on a slot with a particular cert nick name. But it will be up to application to decide, which certs to use.

I understand that with such API the wrapper layer code size will be increased as well as it's maintenance, but moving cert selection into application code feels right for several reasons: removes ambiguity from SSL socket configuration; provides finer configuration control;
and ability to give feedback to user about server configuration.

Comment 21

[Chris Newman]

If you're going to change SSL_ConfigMultiHostServer in this way, you may as well just remove the API altogether.  The additional complexity it adds to learning the NSS API's capabilities is probably not worth the now trivial code/friendliness savings over the callback, particularly since it looks like the application is now expected to walk all the slots manually with the nasty PK11 APIs and dig out all the certs & keys at application start-up time rather than leaving open the possibility for that to happen on a lazy basis (under the hood) as the previous API did.

Hint: The 90%-95% case of SSL/TLS consuming applications do not want to ever go digging around in certificate databases or slots.  We want users to provide strings in the configuration that we pass down to NSS and the NSS does the work for us.  The more it works that way, the more NSS applications can behave consistently and the less likely they are to have security or functionality bugs at the app-NSS API layer.

Personally, I think it would be _far_ better to stick with the much more elegant previous string-based API and just document the limitation that if two or more certs of different types are given the same nickname, they are all activated.  If some foolish application developer wants to allow with the same name to not be activated at the same time (and option I would _never_ expose to customers in my application as it seems far too confusing to be useful), that application developer is free to use the callback-based API for that power-user feature.  That's what's great about a two-level API -- the higher level one solves the 90-95% case and is really easy for callers to use, and the lower level one can deal with the other 5-10% case and can be as annoying for callers to use as necessary to achieve that capability.

Comment 22

[Alexei Volkov]

(In reply to comment #21)
> If you're going to change SSL_ConfigMultiHostServer in this way, you may as
> well just remove the API altogether.  The additional complexity it adds to
> learning the NSS API's capabilities is probably not worth the now trivial
> code/friendliness savings over the callback,

Look at the patch https://bugzilla.mozilla.org/attachment.cgi?id=417507. File ssl3sncfg.c. Cert and key look up code is only a hundred lines long. The rest of the code ~ 500 lines does house keeping work needed for multi-host server.
The code takes care of data duplication and socket reconfiguration. You don't think that it is worth of savings?

> particularly since it looks like
> the application is now expected to walk all the slots manually with the nasty
> PK11 APIs and dig out all the certs & keys at application start-up time
This is not a way what we a proposing. We do not push you to use pkcs11 api to manually walk through slots. NSS has number of function that would find certs and keys for you in one call. We will add another function that will look at a particular slot for the certs and keys, and will return a list of certificates.
It will be up to application to deal with the list of certs, if it would have more then one cert in it.

> 
> Hint: The 90%-95% case of SSL/TLS consuming applications do not want to ever go
> digging around in certificate databases or slots.  We want users to provide
> strings in the configuration that we pass down to NSS and the NSS does the work
> for us.
Well, then the name of the cert is not the right "string of the configuration" since it does not ping point to exact configuration, leaving library guessing what a user had in mind. Using such parameters is wrong for library, wrong for application and bad for a user that will be unhappy, that application with a help of ssl library has used something, that user didn't want and application configuration does not have a way fix it, except removing dup certs from a db.

> Personally, I think it would be _far_ better to stick with the much more
> elegant previous string-based API
It is elegant from an application point of view. It creates mess and uncertainty in some cases on the library side, and I believe in those cases an application administrator need to be informed. An application can inform an admin and provide enough information if it would examine the certs by itself. But SSL library can only return an error in case of ambiguity. Application would not be able to deliver a detailed information of what went wrong.

> and just document the limitation that if two
> or more certs of different types are given the same nickname, they are all
> activated.  If some foolish application developer wants to allow with the same
> name to not be activated at the same time
This case is not an issue. The current code uses all certs of a different key types to activate all defined cipher suites.
The most concerning case is when a user has multiple certs of the same name and same key type. In this case, regardless of what we do, I would not bet that the cert that we picked is the right one.

IMHO, there is only one way to deal with such case on the library side: return an error code, that would indicate that multiple certs have been found and library can not make a decision which one to use. 

Chris, I don't know the user reaction, but I think that a user will be unhappy about the fact, that application configuration can not deal with ambiguities and refuses to use the correct cert.

Comment 23

[Chris Newman]

> ssl3sncfg.c. Cert and key look up code is only a hundred lines long. The rest
> of the code ~ 500 lines does house keeping work needed for multi-host server.
> The code takes care of data duplication and socket reconfiguration. You don't
> think that it is worth of savings?

Not only are those 100 lines of code a significant barrier to NSS consumer-friendliness, the fact the newer API design forces all the certs to be loaded on startup rather than as needed is a bad design.  Today ever server presently consuming NSS has to maintain that 100 lines of code (and it's all subtly and pointlessly different between the servers).  That's 100 lines of security sensitive code that makes NSS appear cumbersome and hard-to-use relative to a library with a simpler programmer's API.  I'm a rarity among application developers in that I'm actually interested in security protocols, their capabilities, design and functionality.  Most application developers will spend as little time and brain power on security library integration as their customers allow.  So design of security library API needs nearly as much consideration of the human factors as a security GUI requires.  The API needs all the key design human interface design principles like "secure by default", "common case is easy, uncommon case is possible", "avoid loaded guns pointing at foot", etc.

> Well, then the name of the cert is not the right "string of the
> configuration" since it does not ping point to exact configuration,
> leaving library guessing what a user had in mind.

I'm open to alternate proposals for the string or strings that correctly identify a certificate to use in a product's configuration, but there will be such a string and it will be the slot name + cert nickname unless there is an alternate proposal.  I've always been puzzled by the fact a certificate nickname is not a unique reference in NSS; I'm unsure of the purpose of a "nickname" if not to uniquely identify the cert/key.  However, that's the string reference to certs that NSS consumers I'm familiar with have always used.  And it's what I will continue to use in my product's configuration until there is a better alternative.  When customers have a problem and the answer is "your configuration is ambiguous, do X or Y to make it not ambiguous", I've never heard a complaint.  Certainly a design where ambiguous configuration is impossible is better, but such a design was not chosen when it came to certificate nicknames (probably for the worse, but that's water under the bridge).

So back to the API design change -- all you've done is move 100 lines of code from one place (in NSS) to many places (every consuming application) without removing the ambiguity problem as the consuming applications will continue to use the certificate nickname as they have in the past (and it wouldn't surprise me if different NSS-based servers produce different results when an ambiguous nickname is used).

If you want to actually eliminate the ambiguity, provide an API with a string reference that uniquely identifies the certificate.  I don't particularly care what that string is, as long as it's a string a human admin can be expected to know and type (or select) in the config UI.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Chris, 
The original design did not require apps to pre-load any info about certs 
and keys.  Instead it used a callback, which only required the app to 
supply the info as needed, on demand, just in time.  You didn't like that,
and asked for a callback-free API.  So, Alexei wrote one for you, and now
you're criticizing it because it requires pre-loading, which naturally, 
being callback-free, it will.  

Regardng nicknames, nicknames are one of numerous certificate attributes 
by which applications can search for certificates using NSS API functions.
It happens that, in the past, numerous applications chose to use nicknames 
as the primary (indeed, sole) means of recording their choice of certificate, 
despite the fact that nickname is insufficient to uniquely identify a particular certificate.  Administrative applications would enumerate the available certificates, show them to the admin user, let him pick one (or 
more) and then record his choice, but only record it by recording the cert
nickname, rather than by recording other attributes that could more uniquely
identify certificates.  It may have been (I suspect was) the case that some
app developer used that means because he knew of no other means, and other
app developers blindly copied it.  That doesn't make it a limitation of NSS.

Nicknames are what they are, and have been since before I started working on
NSS 13 years ago.  Given that they're not what you wish they were, namely 
unique cert identifiers, and it's FAR too late to change that now, we need 
to find some suitable alternative means by which the application can tell
NSS "I want THIS cert".  That is, or certainly can be, conceptually separate 
from the means by which the application records for itself, in its own configuration files (say), that information.  

At this point, NSS's libSSL has one callback-based API which (I believe) 
is sufficient for libSSL to be able to fully implement server side SNI.

Since you want a callback-free API, Alexei has attempted to provide an 
additional API that is callback-free, but so far, his attempts have been 
unable to satisfy all your requirements, which we seem to be progressively
discovering with each attempt, and which may be conflicting (e.g. no callbacks, not also no pre-loading).  This iterative process of discovering the
requirements is taking a long time, and is adding schedule risk for 3.12.6.

Therefore, I suggest to Alexei as follows:
Separate the project into to phases.  Phase 1 would just have the callback based API.  Phase 2 will have the non-callback API, and can be done later, 
when the requirements for that API have been well and truly identified.  
I suggest that you not attempt any more designs for that API until the requirements are well defined, and focus the efforts for phase 2 on defining the requirements first.  But I would try to complete phase 1 before working
on phase 2.

Comment 25

[Alexei Volkov]

We have time constrain, so splitting the project on two parts is the right thing to do.

Regarding changes to API: we can add an extra string parameter to the string based api that would carry hexadecimal value of the key identifier, which will  uniquely represent a cert. 

So it would be something like:
>+SSL_IMPORT SECStatus SSL_ConfigMultiHostServer(PRFileDesc *model,
>+                                               SSLSniNameType nameType,
>+                                         const char *      srvName,
>+                                         const char *      slotName,
>+                                         const char *      certName,
>+                                         const char *      keyId,
>+                                               PRBool      defCfg);

Comment 26

[Chris Newman]

Alexei's proposal in comment 25 would satisfy my requirements, as would that function without the "keyId" argument.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Alexei, please explain your keyId proposal in more detail.  
What does this string contain, exactly?  
How does it uniquely identify a certificate?

Comment 28

[Alexei Volkov]

Attachment: Patch v5p1 - changes to ssl library: split into two independent projects. Patch for project #1.

I've removed implementation SSL_SNISocketConfigHook and it's helper functions. Focusing only on one API function and functionality at a time.

This patch has fixes for the problems that were found during previous reviews.

Comment 29

[Alexei Volkov]

Attachment: Patch v6p1 - changes to ssl library: previous patch was not built.

Comment 30

[Alexei Volkov]

Attachment: Patch v7p1 - changes to ssl library: patch p5v1 was not built.

Comment 31

[Alexei Volkov]

Comment on attachment 420441 [details] [diff] [review]
Patch v7p1 - changes to ssl library: patch p5v1 was not built.

Ok, a few things to work on: run MP tests to check cache sharing between multiple processes; test if host name correctly propagates when Session Ticket Extension is used to restart the session; write a function similar to SSL_ConfigServerSessionID with additional options that would be able to configure session cache for single and multiple process apps.

Comment 32

[Alexei Volkov]

Attachment: Patch v8p1 - changes to ssl library: address last review comments

Comment 33

[Alexei Volkov]

Attachment: Patch v3p2 - changes to certhigh, nss utils and tests: address review comments

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 420825 [details] [diff] [review]
Patch v8p1 - changes to ssl library: address last review comments

r=nelson

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 420826 [details] [diff] [review]
Patch v3p2 - changes to certhigh, nss utils and tests: address review comments

r=nelson

Comment 36

[Alexei Volkov]

Attachment: make session ticket carry negotiated server name info

Session ticket str extended with SECItem to carry server info type and name.
If name was not negotiated, type is set to be TLS_STE_NO_SERVER_NAME(-1).

ssl test extended with an extra test that verifies that a specific name was negotiated on the first handshake and restored from the cache/STE on others.

Comment 37

[Robert Relyea]

Comment on attachment 422645 [details] [diff] [review]
make session ticket carry negotiated server name info

r+

Fix the following comments:
  len bites -> len bytes
  name len (in long addition string) -> name len + length field.

I had almost r- the patch since it missed srvNameLen = srvname->len + .


I presume this matches with the appropriate standard (I didn't see any discussion in the bug).

bob

Unless I am misunderstanding the code,

Comment 38

[Alexei Volkov]

Thank for the review, Bob.
> I had almost r- the patch since it missed srvNameLen = srvname->len + .

Not quite understand what you mean. The total number of bytes that will be transferred in regards to a negotiated name are number of bytes to transfer name type, name length, and the name itself. So it is 1 + 2 + "name len".

What am I missing?

Comment 39

[Alexei Volkov]

Wan-teh found the problem when initial connection is made with SSL2 and renegotiation is done with TLS+SNI. I also think that the same would not work with SSL3/TLS+SNI.

See https://bugzilla.mozilla.org/show_bug.cgi?id=537356#c74

Comment 40

[Alexei Volkov]

Checking  in attachment 422645 [details] [diff] [review]:

/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.93; previous revision: 1.92
done
Checking in cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.65; previous revision: 1.64
done
Checking in lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/ssl/ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.18; previous revision: 1.17
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.75; previous revision: 1.74
done
Checking in tests/ssl/sslstress.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslstress.txt,v  <--  sslstress.txt
new revision: 1.18; previous revision: 1.17
done

Comment 41

[Alexei Volkov]

Ready for 3.12.6

Comment 42

[Alexei Volkov]

Attachment: Patch v1 - fix SNI problems for SSL2<->SSL3 renegotiation

Fix in selfserv.c to avoid indication of virtual server name change when client negotiates default server name.

Comment 43

[Robert Relyea]

Comment on attachment 427151 [details] [diff] [review]
Patch v1 - fix SNI problems for SSL2<->SSL3 renegotiation

r+ rrelyea

Comment 44

[Tomas Hoger]

While testing renegotiation fix, we ran into another issue that seems to be caused by this patch.  Running tstclnt with '-2 -r 2' against selfserv (both using NSS 3.12.6) causes server to report error "SSL peer has no certificate for the requested DNS name." during the 2nd renegotiation (i.e. 3rd handshake).

Problem seems to be caused by a bug in session cache initialization.  selfserv calls SSL_ConfigServerSessionIDCache (or SSL_ConfigMPServerSIDCache), that results in InitCache being called with maxSrvNameCacheEntries == -1.

InitCache treats negative values as special:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsnce.c&rev=1.52&mark=1107-1108#1105

plus an attempt to use default cache size when negative value is provided:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsnce.c&rev=1.52&mark=1171-1173#1169

However, the second check does not use original value -1, rather 0 assigned in the first one.  Additionally, cache->numSrvNameCacheEntries is unsigned, so '< 0' test should be optimized-out as 'always false' by the compiler.

So maxSrvNameCacheEntries == -1 seems to always lead to numSrvNameCacheEntries == 0, which result in CacheSrvName not working when expected.

Feel free to correct me if I'm mis-reading the code.  Thanks!

Comment 45

[Alexei Volkov]

Tomas, thanks for the report. 

I agree, that negotiated server name cache has initialization problem that  leaves server name cache uninitialized when default options are used. This can lead to the described problem when other then a default name was negotiated.
I don't think that renegotiation when only default server name is used has this problem.

Default name is always indicated by having NULL value in ssl3CipherSpec.srvVirtName for possibility to renegotiated with SSL2 and SSL3. Not having server name cache means that a server will always have NULL as a value for the previously negotiated name and will fail any renegotiation that tries to renegotiate any other than a default names.

I guess you are not trying to renegotiate to any other then a default name. Your test case does SSL3 on the first HS and renegotiation with TLS on the second and third HS. This test case is simple and works for me. I run 

selfserv -D -p 10443 -d ../server -n host -B -s           -w nss  -i ../tests_pid.86502 -v 

tstclnt -r 2 -p 11443 -h host -f -d ../client -v -B -s         -2  -w nss -n TestUser

Could you please provide your complete selfserv and tstclnt arguments?

Comment 46

[Tomas Hoger]

(In reply to comment #45)

> Could you please provide your complete selfserv and tstclnt arguments?

There are no other arguments besides mandatory -d/-p/-h/-n and '-2 -r 2' for tstclnt as mentioned above:

  selfserv -d /path/to/nss/ -n www.example.com -p 4433 -v
  tstclnt -d /path/to/nss/ -h www.example.com -p 4433 -2 -r 2

I'm not changing name on rehandshake or use SSLv2 initial hello (as suggested in bug #537356#c85).

This is what I'm seeing:

On initial HS, sid is created and added to cache, but server name is not cached properly in CacheSrvName, as explained above.

On first reHS, sid is looked up from the cache (it contains no server name now) and following code patch is followed:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.136&mark=6278-6447#6274

It contains:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.136&mark=6383-6399#6380

As sid contains no server name, no name is copied to pending spec.

sid also gets uncached:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.136&mark=6287-6294#6286

On second reHS, cache lookup returns NULL, so the code pointed out in bug #537356#c74 is reached with current spec containing no name.

Comment 47

[Tomas Hoger]

Attachment: InitCache fix to properly initialize numSrvNameCacheEntries(integrated)

(In reply to comment #44)
> However, the second check does not use original value -1, rather 0 assigned in
> the first one.  Additionally, cache->numSrvNameCacheEntries is unsigned, so '<
> 0' test should be optimized-out as 'always false' by the compiler.
> 
> So maxSrvNameCacheEntries == -1 seems to always lead to numSrvNameCacheEntries
> == 0, which result in CacheSrvName not working when expected.

This patch should initialize numSrvNameCacheEntries in the way that was probably intended - DEF_NAME_CACHE_ENTRIES is used when maxSrvNameCacheEntries is negative.

With this patch, tstclient can renegotiate more than once.

It does not attempt to address uncache on rehandshake mentioned in comment #46.

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 434498 [details] [diff] [review]
InitCache fix to properly initialize numSrvNameCacheEntries(integrated)

Alexei, please review.
If you give it r+, please commit it.

Comment 49

[Alexei Volkov]

Comment on attachment 434498 [details] [diff] [review]
InitCache fix to properly initialize numSrvNameCacheEntries(integrated)

r=alexei

Comment 50

[Christophe Ravel]

Alexei:
Could you give a status on this bug and what still needs to be done to resolve this bug ?

Comment 51

[Alexei Volkov]

Attachment: Patch part3v1 - implement SNI cert and key cache(not for review)

Comment 52

[Alexei Volkov]

Attachment: Patch part3v2 - implement SNI cert and key cache. Pass all.sh. Need cleanup.(not for review)

Comment 53

[Alexei Volkov]

Attachment: Patch part3v3 - implement SNI cert and key cache. Pass all.sh. Need cleanup.(not for review)

Extra parameter is added to pass alternative trusted ca name list.

Comment 54

[u238590]

I have some requirements

1) I found a bug in keyid iteration code. In GetCertAndKey and PK11_TraverseCertsForNicknameInSlot functions, it finds one certificate with keyid and but then it goes to find again and returns SEC_ERROR_INVALID_ARGS error.

2) Web Server needs an iterator to go over all certificates (to log its expiry date, check whether its a server certificate or not, to check if we can use PKCS11 bypass or not etc). Current iterator needs public structures to be exposed like : ssl3NameToCert, ssl_CERT_SET_ARRAY_SIZE, sslServerCerts, ssl3KeyPair

3) These two flags support (should it be per FD i.e. listener or server wide?)
a) strict-sni : If strict-sni is false, then send the default certificate to clients which do not support SNI extension. If its set to true return error. This is similar to what Apache has.
b) use-default-sni-cert - If SNI certificate is not registered for that domain, return the default certificate if this flag is true. If its set to false return an error.

4) Also some people in my team have reservations to use keyid as its an NSS internal stuff. They prefer using issuer and serial #. So for that I need  ONLY ONE of the three given below 
a) A function which will get KeyID from a given CERTCertifcate structure. Something similar to char * Get_KeyIDFromCert(CERTCertificate * cert);  OR
b) A function which will get KeyID if we provide slot, nickname, issuer, serial. Something like char * Get_KeyIDFromCert(PK11SlotInfo * slot, const char * nickname, const char * issuer, char * serial);  OR
c) Expose SECKEY_DestroyPrivateKey(...); - in this case I will copy keyid related functions from Alexei's patch into Web Server code.

Comment 55

[u238590]

We do not need iterator as we asked for in point (2) above. We can write code and get CERTCertificate structure from certificate nickname. 

What we need :
1) Bug fix of that keyid issue I found
2) These two flags support at server level(should it be per FD i.e. listener)
a) strict-sni : If strict-sni is false, then send the default certificate to clients which do not support SNI extension. If its set to true return error.
b) use-default-sni-cert - If SNI certificate is not registered for that domain, return the default certificate if this flag is true. If its set to false return an error.
3) ONLY ONE of the two given below 
a) A function which will get KeyID from a given CERTCertificate structure. Something similar to char * Get_KeyIDFromCert(CERTCertificate * cert);
b) A function which will get KeyID if we provide slot, nickname, issuer, serial. Something like char * Get_KeyIDFromCert(PK11SlotInfo * slot, const char * nickname, const char * issuer, char * serial);
4) For reconfig support, we need a list of domain names for which certificates have been already added for a particular FD(listener).
5) For reconfig support, can we get an API to delete all certificates? 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 
Some questions Arvind had :
    1) When SSL_configureSecureServer had CERTCertificate as a parameter so why SSL_ConfigureMultiHostServer was designed with nickname, issuer, serial parameters?
    2) How are Administrators using SNI ? Is there any product using SNI?  will they have a usecase where they add a single certificate for multiple domains? 
    3) Why can't SSL_ConfigureMultiHostServer take NULL in domain name parameter?
    4) Can one RSA and ECC certificate have the same nickname and can they have the same keyids?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
My question (may be for future releases not now) : domain on which we register the server is matched with the host client sends using strcmp. Would it make sense to have a wildcard support?

Comment 56

[u238590]

Can you please delete/ignore my last two comments. We are probably going to write our own callback function.

Comment 57

[Kai Engert (:KaiE:)]

Comment on attachment 422645 [details] [diff] [review]
make session ticket carry negotiated server name info

>@@ -191,6 +191,7 @@ Usage(const char *progName)
> "-E means disable export ciphersuites and SSL step down key gen\n"
> "-R means disable detection of rollback from TLS to SSL3\n"
> "-a configure server for SNI.\n"
>+"-k expected name negotiated on server sockets"
> "-b means try binding to the port and exit\n"
> "-m means test the model-socket feature of SSL_ImportFD.\n"
> "-r flag is interepreted as follows:\n"


This patch missed the newline \n at the end of the help output.
Current selfserv help output prints two help lines on a single line.

Given it's not a functional change and an obvious fix, I've checked it in.

Checking in selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.101; previous revision: 1.100
done

Comment 58

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.