Brian wrote: > With SSLBYPASS, the environment variable is checked the first time a socket > is created. That means that if the application called SSL_OptionSetDefault > before creating a socket, the environment variable will override the > explicit call to SSL_OptionSetDefault. ... > ... I will file a separate bug > about more clearly documenting the advantages/disadvantages of using > SSL_OptionSetDefault as it related to environment variables. Wan-Teh wrote: Thank you for the explanation. This is the bug I feared. SSL_OptionSetDefault should override environment variables. We should fix this bug for all environment variables with a separate patch.

Also, SSL_OptionGetDefault will return the wrong results until a socket is created.

This should go into 3.12.11 so that applications can use SSL_OptionSetDefault to properly control the new option from bug 665814.

I believe in yesterday's conference call, Brian explained why this is indeed needed, so we're waiting for a review from Wan-Teh.

I object. The documented behavior of these environment variables is that they override all application behavior. IMO, the environment variables should trump all other calls. I've always envisioned them this way. They exist to allow users to change the behaviors of applications, even those that call "...SetDefault".

Nelson, we cannot make the environment variables trump SSL_OptionSet because that would break backward compatibility. I am happy to let them continue to override SSL_OptionSetDefault. I can just change Gecko to always use SSL_OptionSet and never SSL_OptionSetDefault. (I would like to make this change anyway.) But, SSL_OptionSet must continue to work the same way it does now.

I don't know where the precedence is documented. My expected precedence is as follows, from high to low: SSL_OptionSet SSL_OptionSetDefault environment variables initializers for the static variable 'ssl_defaults' Environment variables change the defaults of the NSS library. Applications should then be able to change an option to the desired value. I believe this is what the current libSSL code intends to do, but it does that imperfectly; the side effect of an SSL_OptionSetDefault call before the first ssl_Socket call is discarded. To implement the behavior Nelson described in comment 5, SSL_OptionSetDefault needs to honor the defaults specified by environment variables, but this is only done for the SSL_NO_LOCKS option using the SSLFORCELOCKS environmet variable. It is not done for the other three environment variables Nelson added: SSLBYPASS, NSS_SSL_ENABLE_RENEGOTIATION, and NSS_SSL_REQUIRE_SAFE_NEGOTIATION.

Comment on attachment 545006 [details] [diff] [review] Make SSL_OptionSetDefault and SSL_OptionGetDefault inspect environment r=wtc. This patch is fine by me, but please wait for Nelson and Bob's review. Please make a reasonable effort to follow Nelson and Bob's distinctive coding styles when modifying their code. For example, Nelson often aligns the variable names in variable declarations, etc. > /* forward declarations. */ > static sslSocket *ssl_NewSocket(PRBool makeLocks); > static SECStatus ssl_MakeLocks(sslSocket *ss); >+static void ssl_SetDefaultsFromEnvironment(); > static PRStatus ssl_PushIOLayer(sslSocket *ns, PRFileDesc *stack, > PRDescIdentity id); So, here you should align ssl_SetDefaultsFromEnvironment with the other function names. >+static void >+ssl_SetDefaultsFromEnvironment() Nit: please add a short comment to document what this function does.

In reply to comment 6: Perhaps I was unclear. I meant that environment variables override SetDefault. (Did I not write that?) I do NOT propose that they override OptionSet.

Comment on attachment 545006 [details] [diff] [review] Make SSL_OptionSetDefault and SSL_OptionGetDefault inspect environment I agree in principle with this patch. The code looks good, too. :)

(In reply to comment #9) > > I meant that environment variables override SetDefault. Nelson, I am still confused. Brian's patch enables SSL_OptionSetDefault to override environment variables. For an environment to override SSL_OptionSetDefault, we need code like this: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsock.c&rev=1.70&mark=964,969-970#964 There is such code only for the SSLFORCELOCKS environment variable.

I edited Brian's patch (add the "void" argument list, required for C) and checked it in on the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH (NSS 3.12.11). Checking in sslsock.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c new revision: 1.71; previous revision: 1.70 done Checking in sslsock.c; /cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v <-- sslsock.c new revision: 1.67.2.3; previous revision: 1.67.2.2 done