I wanted a way to create a ssl server socket using the same server socket interfaces that exist in mozilla.

Comment on attachment 147555 [details] [diff] [review] netwerk patch this patch looks pretty good to me. i didn't want to have to change the server socket API, but i can definitely see why you'd want to do this. it is a big simplifier. please rev the IID, and maybe add a securityInfo getter on nsIServerSocket. that way folks can access the secinfo returned by NewSocket. do we need to provide security callbacks? see nsISocketTransport::securityCallbacks. that parameter is forwarded to the secinfo via nsISSLSocketControl. off hand i don't think a SSL server should need to perform any kind of callbacks, like UI... but perhaps it is a good idea to add the code so that PSM can be free to access the info in the callback if necessary. yeah, what is the correct value for the hostname field? does it even matter? PR_Bind is going to be called later, so PSM/NSS shouldn't need us to pass a valid hostname and port to NewSocket. maybe nsnull & -1 instead? (to make it explicit) no need for REQUIRES to include pipnss since this is only the JS component that is testing SSL server sockets.

I used the patch from Doug Turner, completed the implementation and made a few slight modifications. The work I did can be divided into three items: - Application of Doug Turner’s patch - Referencing security info structure from accepted sockets - Adding a new interface for handling certificates using security callbacks (for UI-less operation) Application of the Doug Turner’s patch: The patch was fully integrated with some small modifications. I added three hard-coded options for the server socket: SSL_OptionSet(fd, SSL_REQUIRE_CERTIFICATE, PR_FALSE); SSL_OptionSet(fd, SSL_REQUEST_CERTIFICATE, PR_TRUE); SSL_OptionSet(fd, SSL_NO_CACHE, PR_TRUE); The server was tested intensively in our application and works pretty well. The nsIServerSocket interface has been changed. The method now takes a socket type parameter. I’m not sure whether it is necessary to change the IID of the server socket interface. In the patch the IID is currently unchanged. Referencing security info structure from accepted sockets: I have written code which sets the callbacks of the accepted socket transfer. This is a bit of a hack right now; I'd like to discuss with you how to make a cleaner implementation. I have been attaching the security info for an accepted socket to the security callbacks member of the server socket. In the patch, you can see this code in the nsSSLIOLayerAccept method (file nsNSSIOLayer.cpp). In the upper layer (nsServerSocket), I retrieve the callbacks from the server socket’s security info and QI it for the security info of the accepted socket. This is possible because both the security callbacks and the security info implement nsIInterfaceRequestor. This works, but it is a hack because this information definitely doesn't belong in this member. I modified the prototype of the InitWithConnectedSocket method of the nsSocketTransport class to take the security info as a parameter. It is referenced by the transport, and user callbacks may be set by calling the SetSecurityCallbacks method and passing in the interface of the underlying socket. Adding a new interface for handling certificates using security callbacks (for UI-less operation): Another issue arose from the fact that we are using certain Mozilla components (XPCOM, Necko and NSS only) as standalone modules in our application. Normally a user decides whether to accept a certificate using a dialog box inside Firefox. In our case there is no Firefox UI. In addition, we needed more extensive authentication possibilities in our application since our client machines' identities are not tied to their domain or IP address. For this reason I implemented a new interface (something like a prompt callback) that provides makes it possible to specify acceptance behavior for certicates internally, without user interaction. This interface offers these methods: [noscript] PRBool PromptCertificatePassword(); [noscript] PRBool ConfirmPeerCommonName(); [noscript] PRInt32 ChooseClientCertificate(); PromptCertificatePassword may be used to retrieve the database password from some internal storage. ConfirmPeerCommonName is used in a very similar manner to the ConfirmMismatchDomain dialog box in Firefox. It appears in Firefox when the SSL server you connect to is not running at the same domain as the certificate that it was issued. We are building a P2P application where every peer is running an SSL server. The nature of the network is such that a peer's IP address can change at any time (e.g. it might be a laptop connected sometimes at the office and sometimes at home). Also, there might not even be a domain associated with a given peer (it might just be a raw IP address). The ConfirmPeerCommonName method is used to check the name of the remote peer according to some application-specific methodology. For example: we have presence information from a third party about “John” being connected on a computer accessible at 84.62.72.79. We therefore assume that we will reach John if we connect to this address. The common name returned in his certificate really is “John”. Mozilla’s NSS will reject the certificate although it should be accepted. This explains the need for our hook: we check the common name of the certificate confirm that it matches the expected common name ("John"). If so, the certificate is accepted. This is same mechanism as when the user accepts an invalid domain name for an HTTPS server. ChooseClientCertificate is used to choose the peer’s certificate. In Firefox there is a dialog box which lets the user choose a certificate manually to be used for client-side authentication. This callback lets this determination be performed internally without user intervention.

This is the patch

this is UI-less interface, it have to be placed into \netwerk\socket\base\ directory.

dougt: would you like to do the initial round of reviews on these SSL patches since they are a variation of your original work?

Comment on attachment 184481 [details] [diff] [review] SSL server patch, UI-less interface sure. These are the first set of problems i found with the patch. Some of them may be problems with my origanal work... In any case: When changing an interface, it is a good idea to change the associated UUID. nsIServerSocket needs a new UUID. There are tabs in the nsIServerSocket interface. That is a bad thing. This problem exists throughout the patch -- use exactly the same white spacing as the file does and never use tabs regardless of what the file does. The comment in the IDL is somewhat misleading (I applogize if it is my own patch). We should assert how the nsISocketProvider and the aSocketType string value are assocaiated here. A comment like this: + // This is hack... Is so scary that I don't even want to continue. Why is it a hack? What is the right thing to do? You do not need to test for mSecInfo. Basically, do_QueryInterface(NULL) == NULL. + nsCOMPtr<nsISSLSocketControl> secCtrl; + if (mSecInfo) + secCtrl = do_QueryInterface(mSecInfo); You should be able to simply pass |ir| into InitWithConnectedSocket. The query interface to nsISupport may not be needed. + nsCOMPtr<nsISocketProviderService> spserv = do_GetService(kSocketProviderServiceCID); + if (!spserv) + return NS_ERROR_UNEXPECTED; NS_ERROR_NOT_AVAILABLE or using the rv returned via do_getService might be a better error to return. + "localhost", // XXX ???? Do we care? Should this be a preference or something? + /*opt.option = PR_SockOpt_Reuseaddr; opt.value.reuse_addr = PR_TRUE; - PR_SetSocketOption(mFD, &opt); + PR_SetSocketOption(mFD, &opt);*/ I don't like commenting out stuff. Lets remove it if it isn't needed. however... it seams like not making the socket reuse_addr will have performance regressions, right -- this code path is for all sockets. use cvs -u10N so that you can have nsICertificatePrompt.idl included in the same patch file Remove the extra ws at the begining of netwerk/test/Makefile.in Why are you removing a bunch of stuff from netwerk/test/Makefile.in + rv = pIR->GetInterface(NS_GET_IID(nsICertificatePrompt), getter_AddRefs(certPrompt)); + + if (NS_SUCCEEDED(rv) && certPrompt != nsnull) You do not have to test both. Testing for certPrompt is enough. rv = getNSSDialogs((void**)&badCertHandler, NS_GET_IID(nsIBadCertListener), NS_BADCERTLISTENER_CONTRACTID); + + // This error is ignored if (NS_FAILED(rv)) - return PR_FALSE; + badCertHandler = nsnull; + /* return PR_FALSE;*/ + I do not know why this isn't fatal. Please attach another patch and ? me as the reviewer.

Comment on attachment 184482 [details] IU-less interface please look at how other IDL files are formated. You should mark the interface as [noscript] and remove this tag from the methods (unless you envision someone wanting to qi to this empty interface in a scripting language). It would be good to have more comments on what this interface does. Why does it exist. The comment "continue-errors internaly" doesn't make alot of sense to me. Thanks for looking at fixing this patch.

why is that interface not scriptable, though? maybe JS code would want to use it?

sure -- if scripting needs this interface, we need it scriptable. I just don't understand why this interface is needed.

Here is new version of the patch with some fixes and explanations: A comment like this: + // This is hack... Is so scary that I don't even want to continue. Why is it a hack? What is the right thing to do? I fixed this code. Now there is different correct approach getting security info from accepted socket. I use GetInterface of the listen socket to obtain it. + /*opt.option = PR_SockOpt_Reuseaddr; opt.value.reuse_addr = PR_TRUE; - PR_SetSocketOption(mFD, &opt); + PR_SetSocketOption(mFD, &opt);*/ I don't like commenting out stuff. Lets remove it if it isn't needed. however... it seams like not making the socket reuse_addr will have performance regressions, right -- this code path is for all sockets. This option has got into the patch by my mistake. I set this option to false from following reason: I want to use servers binded to ipv4 and ipv6 in parallel on one port. In mozilla there is not a way to recognize if the system supports ipv6. If I run server with socket binded to ipv6 family address on a system that do not support ipv6 the socket is internaly by NSPR changed to ipv4 binded socket. In that case there are two servers binded to ipv4 on the same port number – this is not correct for me. I will report new bug – “there is no way to recognize ipv6 support in OS”. The “reuse address” option still unchanged. + "localhost", // XXX ???? Do we care? Should this be a preference or something? There is no need to fill-in this value. I tested the server with an empty string and all working fine. All other fixes to your comment should be correct now. Interface nsICertificatePrompt is now well commented and added as new attachment. I cannot force my cvs to add it to the patch... Interface’s purpose is explained in comments in nsICertificatePrompt.idl file. It is not scripting because usage of this interface may be harmfull for mozilla nss when used from javascripts. It overrides ssl authentication process on lower level – there should not be possibility to use it from scripts because of possible security risks.

This is well-comented and well-formated version of the security callback interface.

removed myself from review. honza_b, could you post a new patch? I couldn't apply this patch (186586) cleanly. Bob, nelson -- this work will allow firefox to accept ssl connections. This might aide is p2p applications, etc. If you have time, would love your input.

Comment on attachment 147554 [details] [diff] [review] security/manager/ssl patch clearing old requests

mass reassigning to nobody.

Is there any progress in this area? It will be nice if nsServerSocket can do SSL.

Any progress? Will be nice to expose to JS so that extensions can create SSL servers if need be.

I am also interested in this feature to support an encrypted connection from desktop Firefox to Firefox OS phones for debugging via WiFi (bug 962308). For my use case, I would only need to support self-signed certs, and I would intend to handle certificate choosing, etc. silently (without a UI). I've edited the bug title, as it seems this was never about "HTTPS", but instead only the SSL / TLS layer. I'll take a look at this and see how far I can get.

Here's the interface I'm proposing for TLSServerSocket. It's intentionally quite minimal. While there are many possible TLS server settings that could be exposed, I'm only thinking about those needed for my WiFi debugging use case in bug 962308. Of course, the interface can always be expanded later if needed for other purposes. Honza, what do you think of this so far? David, I believe Honza mentioned you'd be interested in this too.

To add a little more detail, this approach extends the existing nsIServerSocket, and then adds a few more options to configure the extra TLS settings. Other than that, it's used just as you would use an nsIServerSocket instance today.

Comment on attachment 8440158 [details] [diff] [review] Part 1: TLSServerSocket interface Review of attachment 8440158 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a great start. Here's some things I imagine it would be useful to add later: SNI, OCSP stapling, maybe arbitrary extensions? Also, it would be nice to be able to specify a set of certificates to send (i.e. intermediates), not just the one.

(To clarify, I'm imagining OCSP stapling would consist of telling the socket "send these bytes I'm specifying as a stapled OCSP response" and leaving it up to the caller to generate the response.)

(In reply to David Keeler (:keeler) [use needinfo?] from comment #22) > Comment on attachment 8440158 [details] [diff] [review] > Part 1: TLSServerSocket interface > > Review of attachment 8440158 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks like a great start. Here's some things I imagine it would be useful to > add later: SNI, OCSP stapling, maybe arbitrary extensions? Also, it would be > nice to be able to specify a set of certificates to send (i.e. > intermediates), not just the one. Yes, those all seem like reasonable future extensions. Thanks for looking it over!

Comment on attachment 8440158 [details] [diff] [review] Part 1: TLSServerSocket interface Review of attachment 8440158 [details] [diff] [review]: ----------------------------------------------------------------- This needs some rational for the "listener" interface. ::: netwerk/base/public/nsITLSServerSocket.idl @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsIServerSocket.idl" > +#include "nsIX509Cert.idl" I think you just need to declare nsIX509Cert. if any of your .h/.cpp files fail to build, then include the nsIX509Cert.h there. @@ +28,5 @@ > + * > + * Sets a callback to be notified of various TLS handshake events as described > + * below in niITLSSecurityCallback. This is optional. > + */ > + void awaitSecurity(in nsITLSSecurityCallback callback); this should be setSecurityObserver and the nsITLSSecurityCallback should be named nsITLSServerObserver @@ +51,5 @@ > + * > + * Whether the server should require a client auth certificate from the > + * client. Defaults to REQUIRE_FIRST_HANDSHAKE. See values below. > + */ > + attribute unsigned long requireCertificate; why do you have requestCertificate then? REQUIRE_NEVER should be default IMO. @@ +82,5 @@ > + * caching or tickets allow TLS to skip the cert exchange. > + */ > + void onClientCertReceived(in nsITLSServerSocket server, > + in nsISocketTransport transport, > + in nsIX509Cert cert); isn't the peer (actually client here) cert exposed on the sec info object that can be obtained from the transport? If so, then I don't see a need for this callback. @@ +90,5 @@ > + * > + * Called when the TLS handshake is complete. > + */ > + void onHandshakeDone(in nsITLSServerSocket server, > + in nsISocketTransport transport); so, this is called before the socket is given the server listener? why do you have this callback at all?

(In reply to Honza Bambas (:mayhemer) from comment #25) > Comment on attachment 8440158 [details] [diff] [review] > Part 1: TLSServerSocket interface > > @@ +51,5 @@ > > + * > > + * Whether the server should require a client auth certificate from the > > + * client. Defaults to REQUIRE_FIRST_HANDSHAKE. See values below. > > + */ > > + attribute unsigned long requireCertificate; > > why do you have requestCertificate then? > > REQUIRE_NEVER should be default IMO. Mostly I am just offering the TLS options as they are exposed by NSS, which separately exposes request and require[1] options. I said defaults to REQUIRE_FIRST_HANDSHAKE since that's what NSS's default value for the option is[2]. From my reading of NSS, it looks like the value of |requireCertificate| does not end up being tested unless you also set |requestCertificate = true|. Anyway, I agree it's confusing. I am fine making the default REQUIRE_NEVER for this interface. > @@ +82,5 @@ > > + * caching or tickets allow TLS to skip the cert exchange. > > + */ > > + void onClientCertReceived(in nsITLSServerSocket server, > > + in nsISocketTransport transport, > > + in nsIX509Cert cert); > > isn't the peer (actually client here) cert exposed on the sec info object > that can be obtained from the transport? If so, then I don't see a need for > this callback. At the moment, I don't have a sec info object at all, because I was not planning to go through the nsNSSIOLayer path, which is what creates the sec info object. Instead, I am just doing things like SSL_ImportFD myself and setting a few NSS callbacks. nsNSSIOLayer is very complex, and (unsurprisingly) assumes you are a TLS client, not a server. So, rather than adding even more if blocks and complexity, I am just not using it at all. I could possibly make my own security info object just for servers, so the transport always retains access to the peer's cert. But even with that, I think this event is still useful, as it lets the server decide if the cert is acceptable right when it is received. What do you think? Should I instead be using nsNSSIOLayer? This may be hard to decide in the abstract from just this interface... Feel free to ping me on IRC if that would be easier. Eventually, I'll have an implementation to attach, but by that point there would be quite a bit of wasted work if I've gone the wrong direction. :) > @@ +90,5 @@ > > + * > > + * Called when the TLS handshake is complete. > > + */ > > + void onHandshakeDone(in nsITLSServerSocket server, > > + in nsISocketTransport transport); > > so, this is called before the socket is given the server listener? Are you asking "is this called before you have called |asyncListen(serverListener)|?" If so, then no, because the socket is not listening on the port before that step. If you are asking something else, I am not sure what it is. In my current experiments, the timeline looks like: 1. Call |asyncListen(serverListener)| to listen for new connections 2. When a client connects, PR_Accept is called, then |serverListener.onSocketAccepted| is called 3. Via an NSS hook, we may call |onClientCertReceived| if a client cert is received 4. Via an NSS hook, |onHandsakeDone| is called once the TLS handshake is completed > why do you have this callback at all? It is used to alert the server that the TLS handshake has fully completed. How this information is used seems application-defined, I would think. This callback is set by |SSL_HandshakeCallback|[3], which is currently used for a variety of reasons to inspect state of the connection, record connection stats, and other related activities. [1]: http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl.h#71,87 [2]: http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslsock.c#62 [3]: http://dxr.mozilla.org/mozilla-central/search?q=SSL_HandshakeCallback&case=true

(In reply to J. Ryan Stinnett [:jryans] from comment #26) > (In reply to Honza Bambas (:mayhemer) from comment #25) > > Comment on attachment 8440158 [details] [diff] [review] > > Part 1: TLSServerSocket interface > > > > @@ +51,5 @@ > > > + * > > > + * Whether the server should require a client auth certificate from the > > > + * client. Defaults to REQUIRE_FIRST_HANDSHAKE. See values below. > > > + */ > > > + attribute unsigned long requireCertificate; > > > > why do you have requestCertificate then? > > > > REQUIRE_NEVER should be default IMO. > > Mostly I am just offering the TLS options as they are exposed by NSS, which > separately exposes request and require[1] options. > > I said defaults to REQUIRE_FIRST_HANDSHAKE since that's what NSS's default > value for the option is[2]. From my reading of NSS, it looks like the value > of |requireCertificate| does not end up being tested unless you also set > |requestCertificate = true|. Yep, that is IIRC what NSS provides for options. > > Anyway, I agree it's confusing. I am fine making the default REQUIRE_NEVER > for this interface. Before deciding what to do here, i had to check what the NO_ERROR option actually means. According [1] and [2] it's: " Value 2 has always been the default. New Value 3 is appropriate for servers that want to re-request, but still not require, client-auth from a client with whom an SSL session is already established. " SSL_REQUIRE_FIRST_HANDSHAKE ((PRBool)2) SSL_REQUIRE_NO_ERROR ((PRBool)3) OK, looking closer and with this explanation I think we can go with this, but the default should be to don't request and there should be just a single attribute (for simplicity) with options: DONT_REQUEST (= request:false, require:N/A) // the default REQUEST_ON_FIRST_HANDSHAKE (= request:true, require:NEVER) REQUEST_ALWAYS (= request:true, require:NO_ERROR) REQUIRE_ON_FIRST_HANDSHAKE (= request:true, require:FIRST_HANDSHAKE) REQUIRE_ALWAYS (= request:true, require:ALWAYS) All of them needs very good explanatory comments, but names now seems to make more sense from just looking at them. Please arrange the attributes in the interface so that those belonging logically together are close each other. > > > @@ +82,5 @@ > > > + * caching or tickets allow TLS to skip the cert exchange. > > > + */ > > > + void onClientCertReceived(in nsITLSServerSocket server, > > > + in nsISocketTransport transport, > > > + in nsIX509Cert cert); > > > > isn't the peer (actually client here) cert exposed on the sec info object > > that can be obtained from the transport? If so, then I don't see a need for > > this callback. > > At the moment, I don't have a sec info object at all, because I was not > planning to go through the nsNSSIOLayer path, which is what creates the sec > info object. Instead, I am just doing things like SSL_ImportFD myself and > setting a few NSS callbacks. I don't say it's bad, just please explain why you do this. Is that you want to be independent of PSM layers? > > nsNSSIOLayer is very complex, and (unsurprisingly) assumes you are a TLS > client, not a server. So, rather than adding even more if blocks and > complexity, I am just not using it at all. OK, I see. > > I could possibly make my own security info object just for servers, so the > transport always retains access to the peer's cert. But even with that, I > think this event is still useful, as it lets the server decide if the cert > is acceptable right when it is received. > > What do you think? Should I instead be using nsNSSIOLayer? Depends, since this should be exposed in high application levels of Gecko, I was somewhat hoping it go via PSM, but that is not a strict requirement at all. I remember writing ssl servers based on Gecko for in add-on once and we end up throwing away even the whole Necko and write the socket layers from scratch :) And it worked! I'm looking at nsNSSSocketInfo object, it's designed as a client object apparently, so no use here. However, nsISocketTransport.securityInfo should be filled with something here. Maybe introduce a new interface and its implementation like nsIClientSSLStatus (the 'server mirror' to it is the existing nsISSLStatus). This new interface will provide information about cipher and its strength (like nsISSLStatus does) + the client certificate, if present: nsIClientSSLStatus { readonly attribute nsIX509Cert clientCert; // may be null readonly attribute string cipherName; readonly attribute unsigned long keyLength; readonly attribute unsigned long secretKeyLength; } > This may be > hard to decide in the abstract from just this interface... Feel free to > ping me on IRC if that would be easier. Eventually, I'll have an > implementation to attach, but by that point there would be quite a bit of > wasted work if I've gone the wrong direction. :) I thought you were so far designing the interface. And so far I'm not very persuaded it's yet correct. > > > @@ +90,5 @@ > > > + * > > > + * Called when the TLS handshake is complete. > > > + */ > > > + void onHandshakeDone(in nsITLSServerSocket server, > > > + in nsISocketTransport transport); > > > > so, this is called before the socket is given the server listener? > > Are you asking "is this called before you have called > |asyncListen(serverListener)|?" If so, then no, because the socket is not > listening on the port before that step. If you are asking something else, I > am not sure what it is. No, I'm asking if this has been called before onSocketAccepted of your server listener (aka the target consumer). > > In my current experiments, the timeline looks like: > > 1. Call |asyncListen(serverListener)| to listen for new connections > 2. When a client connects, PR_Accept is called, then > |serverListener.onSocketAccepted| is called Hmm.. so you provide the 'raw' socket first? That doesn't seem right to me, why would the consumer be interested in the raw socket before it's even supplied the ssl socket over it? That is pretty wrong to do. > 3. Via an NSS hook, we may call |onClientCertReceived| if a client cert is > received In your implementation of the NSS hook I'd only put the client cert to the appropriate ClientSecurityInfo object. Nothing more. > 4. Via an NSS hook, |onHandsakeDone| is called once the TLS handshake is > completed OK, this is IMO too granular. These hooks don't need to be handled at all, and mostly have use only in case of connecting to a server, not accepting a client. Unless you have a strong reason to expose these points in the handshake, I am not a big fun of this design. Please keep in mind that the handshake is driven by reading or writing the socket. Read and write return WOULD_BLOCK until the handshake is done. Then an error is returned or normal r/w operation passes. How are you going to process the handshake? I would pref, if simple enough, to drive the handshake inside your ssl server and only pass the socket to the target consumer after it's done (successfully or with an error). This way in onSocketAccepted all the necessary sec info will be available to the consumer. Note that there may be server implementations (consumers) that first send data out before client sends anything to the server. You can however always do PR_Write(0) on the socket to move forward with the handshake, we already do that in Necko on some place. See [3]. The missing required client cert will be exposed as a read error on the socket transport (or better said, on its streams) with error NO_CERTIFICATE. Any other error will be exposed similar way. The client cert (whether it's there or is the expected one) can be looked at the secInfo object you will introduce. If I'm missing something, please let me know. You also have to handle the renegotiation feature. But in that case it seems to me more reasonable to call some |bool onRenegotiating(nsISocketTransport, any more info...)| callback that may also veto when something doesn't smell good. BTW, the renegotiation option should also be exposed on the server socket interface, you may pass this callback along with flags using some nsISSLServerSocket.setupRenegotiation(flags, callback) if my idea is correct, of course :) > > > why do you have this callback at all? > > It is used to alert the server Please make clear what 'the server' means in this context. > that the TLS handshake has fully completed. > How this information is used seems application-defined, I would think. This > callback is set by |SSL_HandshakeCallback|[3], which is currently used for a > variety of reasons to inspect state of the connection, record connection > stats, and other related activities. Question is if the server actually needs that. Only reason to me seems to be a rejection of the connection at some more early stages. Does that your callback allow that? I think you are too tight to how NSS works. But on this level you should provide a very simple API to work with. Also note, that one day we *may* turn to use openSSL, and this should 'migratible' to it. [1] https://hg.mozilla.org/projects/nss/rev/f0eaa856b98e [2] http://hg.mozilla.org/mozilla-central/annotate/aab918af0f9c/security/nss/lib/ssl/ssl3con.c#l7840 [3] http://hg.mozilla.org/mozilla-central/annotate/aab918af0f9c/netwerk/protocol/http/nsHttpConnection.cpp#l299

Honza, thanks for your thorough comments so far, it's quite helpful! :) (In reply to Honza Bambas (:mayhemer) from comment #27) > DONT_REQUEST (= request:false, require:N/A) // the default > REQUEST_ON_FIRST_HANDSHAKE (= request:true, require:NEVER) > REQUEST_ALWAYS (= request:true, require:NO_ERROR) > REQUIRE_ON_FIRST_HANDSHAKE (= request:true, require:FIRST_HANDSHAKE) > REQUIRE_ALWAYS (= request:true, require:ALWAYS) > > All of them needs very good explanatory comments, but names now seems to > make more sense from just looking at them. > > Please arrange the attributes in the interface so that those belonging > logically together are close each other. Yes, this seems better to me too. > However, nsISocketTransport.securityInfo should be filled with something > here. Maybe introduce a new interface and its implementation like > nsIClientSSLStatus (the 'server mirror' to it is the existing nsISSLStatus). > This new interface will provide information about cipher and its strength > (like nsISSLStatus does) + the client certificate, if present: > > nsIClientSSLStatus { > readonly attribute nsIX509Cert clientCert; // may be null > readonly attribute string cipherName; > readonly attribute unsigned long keyLength; > readonly attribute unsigned long secretKeyLength; > } This approach sounds good to me as well. That way the client security info will always be accessible from the transport, just as it is when connecting Gecko connects to a TLS server today. > > > This may be > > hard to decide in the abstract from just this interface... Feel free to > > ping me on IRC if that would be easier. Eventually, I'll have an > > implementation to attach, but by that point there would be quite a bit of > > wasted work if I've gone the wrong direction. :) > > I thought you were so far designing the interface. And so far I'm not very > persuaded it's yet correct. Yes, I am convinced you are right. Thanks for your interface tips thus far! > > > @@ +90,5 @@ > > > > + * > > > > + * Called when the TLS handshake is complete. > > > > + */ > > > > + void onHandshakeDone(in nsITLSServerSocket server, > > > > + in nsISocketTransport transport); > > > > > > so, this is called before the socket is given the server listener? > > > > Are you asking "is this called before you have called > > |asyncListen(serverListener)|?" If so, then no, because the socket is not > > listening on the port before that step. If you are asking something else, I > > am not sure what it is. > > No, I'm asking if this has been called before onSocketAccepted of your > server listener (aka the target consumer). Ah, no, onSocketAccepted was called before onHandshakeDone. > > In my current experiments, the timeline looks like: > > > > 1. Call |asyncListen(serverListener)| to listen for new connections > > 2. When a client connects, PR_Accept is called, then > > |serverListener.onSocketAccepted| is called > > Hmm.. so you provide the 'raw' socket first? That doesn't seem right to me, > why would the consumer be interested in the raw socket before it's even > supplied the ssl socket over it? That is pretty wrong to do. No, it's not a raw socket. The main, listening socket is SSL imported (SSL_ImportFD), and then all sockets that come from it when clients connect start out already having the SSL layer. > > 3. Via an NSS hook, we may call |onClientCertReceived| if a client cert is > > received > > In your implementation of the NSS hook I'd only put the client cert to the > appropriate ClientSecurityInfo object. Nothing more. Okay, I think that seems fine. I thought about doing this as well, but was avoiding making the security info object at the time. Since we've agreed there should be security info now, just storing the cert there seems best. > > 4. Via an NSS hook, |onHandsakeDone| is called once the TLS handshake is > > completed > > OK, this is IMO too granular. These hooks don't need to be handled at all, > and mostly have use only in case of connecting to a server, not accepting a > client. Unless you have a strong reason to expose these points in the > handshake, I am not a big fun of this design. > > Please keep in mind that the handshake is driven by reading or writing the > socket. Read and write return WOULD_BLOCK until the handshake is done. > Then an error is returned or normal r/w operation passes. > > How are you going to process the handshake? I would pref, if simple enough, > to drive the handshake inside your ssl server and only pass the socket to > the target consumer after it's done (successfully or with an error). This > way in onSocketAccepted all the necessary sec info will be available to the > consumer. Okay, I'll try doing it this way (waiting until the handshake is done before alerting the consumer with onSocketAccepted). The consumer implementation can of course still close the connection to the client if the security info is not sufficient to match their requirements, etc. so I don't think there's any loss of flexibility this way. > Note that there may be server implementations (consumers) that first send > data out before client sends anything to the server. You can however always > do PR_Write(0) on the socket to move forward with the handshake, we already > do that in Necko on some place. See [3]. Aha, that's a good trick, thanks for pointing it out! That would have felt like a hack to me if I thought I was the first to do it, so I am glad to know it's considered an accepted approach. > The missing required client cert will be exposed as a read error on the > socket transport (or better said, on its streams) with error NO_CERTIFICATE. > Any other error will be exposed similar way. The client cert (whether it's > there or is the expected one) can be looked at the secInfo object you will > introduce. If I'm missing something, please let me know. This all sounds correct to me. > You also have to handle the renegotiation feature. But in that case it > seems to me more reasonable to call some |bool > onRenegotiating(nsISocketTransport, any more info...)| callback that may > also veto when something doesn't smell good. BTW, the renegotiation option > should also be exposed on the server socket interface, you may pass this > callback along with flags using some > nsISSLServerSocket.setupRenegotiation(flags, callback) if my idea is > correct, of course :) The feels like optional feature that could wait until a later follow-up. I'll consider it if it seems like it would be easy to add. Otherwise, I'll just disable renegotiation on these sockets for now. Either way, your API suggestions for it seem reasonable to me. > > > why do you have this callback at all? > > > > It is used to alert the server > > Please make clear what 'the server' means in this context. Here I meant the consumer of the TLSServerSocket interface, i.e. the application-level code is try to use the TLS socket for some purpose. > > that the TLS handshake has fully completed. > > How this information is used seems application-defined, I would think. This > > callback is set by |SSL_HandshakeCallback|[3], which is currently used for a > > variety of reasons to inspect state of the connection, record connection > > stats, and other related activities. > > Question is if the server actually needs that. Only reason to me seems to > be a rejection of the connection at some more early stages. Does that your > callback allow that? It's probably not necessary. As I write above, we can likely delay |onSocketAccepted| until after the handshake, and then let the server consumer code close the connection if it doesn't like something at that point. > I think you are too tight to how NSS works. But on this level you should > provide a very simple API to work with. Yes, I generally agree with you. :) I think with all your suggestions here so far, the API will be much better, and NSS won't bleed through.

(In reply to Honza Bambas (:mayhemer) from comment #27) > Please keep in mind that the handshake is driven by reading or writing the > socket. Read and write return WOULD_BLOCK until the handshake is done. > Then an error is returned or normal r/w operation passes. > > How are you going to process the handshake? I would pref, if simple enough, > to drive the handshake inside your ssl server and only pass the socket to > the target consumer after it's done (successfully or with an error). This > way in onSocketAccepted all the necessary sec info will be available to the > consumer. > > Note that there may be server implementations (consumers) that first send > data out before client sends anything to the server. You can however always > do PR_Write(0) on the socket to move forward with the handshake, we already > do that in Necko on some place. See [3]. So, the DevTools server that I am trying to support with this is such a server implementation that first sends data before the client sends anything to the server. I've tried using PR_Write(fd, "", 0) as you suggested right after PR_Accept is called (so the timing is similar to this point[1] in nsServerSocket). I can see from tracing the SSL methods that this does try to move the SSL state machine along, but it fails because it tries to receive the ClientHello message, and it's not yet available for reading from the client. So, what's a good way to wait for the client to connect, send ClientHello, and then finally trigger the server side to complete the SSL state machine? I don't really want to have to open the transport's streams in my server implementation to solve it, since that's what the server consumer should do (just like with nsServerSocket). My previous approach of notifying the consumer of onSocketAttached before SSL handshake completes was able to work around this, since the server consumer implementation writes its output right away. However, as you noted, this means the server doesn't yet know if the handshake will succeed before writing, can't check the security status until later, etc. I still prefer your approach here, so I hope there's a good place in the timing that I can hook into and get the server's SSL state machine to move along correctly. [1]: http://hg.mozilla.org/mozilla-central/annotate/74985b96c4c3/netwerk/base/src/nsServerSocket.cpp#l186

(In reply to J. Ryan Stinnett [:jryans] from comment #29) > (In reply to Honza Bambas (:mayhemer) from comment #27) > > Please keep in mind that the handshake is driven by reading or writing the > > socket. Read and write return WOULD_BLOCK until the handshake is done. > > Then an error is returned or normal r/w operation passes. > > > > How are you going to process the handshake? I would pref, if simple enough, > > to drive the handshake inside your ssl server and only pass the socket to > > the target consumer after it's done (successfully or with an error). This > > way in onSocketAccepted all the necessary sec info will be available to the > > consumer. > > > > Note that there may be server implementations (consumers) that first send > > data out before client sends anything to the server. You can however always > > do PR_Write(0) on the socket to move forward with the handshake, we already > > do that in Necko on some place. See [3]. > > So, the DevTools server that I am trying to support with this is such a > server implementation that first sends data before the client sends anything > to the server. > > I've tried using PR_Write(fd, "", 0) as you suggested right after PR_Accept > is called And ssl socket is layerd over the tcp socket, right? > (so the timing is similar to this point[1] in nsServerSocket). I > can see from tracing the SSL methods that this does try to move the SSL > state machine along, but it fails because it tries to receive the > ClientHello message, and it's not yet available for reading from the client. Hmm.. Then the client does something wring. The client expects to get some data from the server (as I understand from reading you description) and I assume it tries to read them, right? Hence the client should poll the socket for read, it should be getting 'socket readable' notifications (despite there are no data... I think it should work this way, if not, it's an nss or psm bug), try to read and get WOULD_BLOCK errors. Does this happen? > > So, what's a good way to wait for the client to connect, send ClientHello, > and then finally trigger the server side to complete the SSL state machine? > I don't really want to have to open the transport's streams in my server > implementation to solve it, since that's what the server consumer should do > (just like with nsServerSocket). My idea was to give the socket in onSocketAccepted of your server listener in a state with all the security information already checked/filled on it. I don't understand why you should open socket stream inside your server implementation. You have (must have) access to the PRFileDesc, you don't need to do it. Setup the socket, PR_Write(0) it, then give to the consumer when PR_Write doesn't return WOULD_BLOCK, done. > > My previous approach of notifying the consumer of onSocketAttached before > SSL handshake completes was able to work around this, since the server > consumer implementation writes its output right away. However, as you > noted, this means the server doesn't yet know if the handshake will succeed > before writing, can't check the security status until later, etc. Exactly, and PR_Write(0) must work for you as well. I don't see any difference why sending the read data (via socket transport APIs) works and why PR_Write(0) does not. Maybe submit the patch here so I can take a look, of course with STR to trigger the problem. > > I still prefer your approach here, so I hope there's a good place in the > timing that I can hook into and get the server's SSL state machine to move > along correctly. > > [1]: > http://hg.mozilla.org/mozilla-central/annotate/74985b96c4c3/netwerk/base/src/ > nsServerSocket.cpp#l186 You could potentially derive your ssl server socket from the nsServerSocket and adjust or split nsServerSocket to give enough virtualization hooks to achieve that nicely. I would not be against some nice ServerSocketBase class that you can easily build tpc and ssl server socket on.

I feel like I am not describing the issue well. It is not specific to the DevTools server or client. The sockets are correctly layered as TLS, yes. It is a timing problem. Here is the current timeline of events that takes place: 1. Client makes connection (socketTransportService.createTransport) to server 2. Client awaits input via the transport's input stream's |asyncWait| method 3. STS notifies the server's socket of a connection in OnSocketReady 4. Server's onSocketReady calls PR_Accept to accept the client connection 5. Server's onSocketReady tries to PR_Write(0) to advance TLS state (fails because it can't read ClientHello since it was not sent yet) 6. Client's event loop spins, and ClientHello is sent asynchronously So, I need to wait until the client has written ClientHello, and *then* use PR_Write(0). Assuming I do so, it should work just fine. What is a good approach for knowing when some data (hopefully the ClientHello) has arrived at the server, and so I should try PR_Write(0) again? I could use a timer. One example of PR_Write(0) does this[1]. Should I take the same approach, or is there something better you would prefer? [1]: http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/TunnelUtils.cpp#374

Patrick, could you review this since Honza is away? My hope is to land this in 34. The goal is to offer a basic TLS server that is accessible to chrome JS. I intend to use this as part of the WiFi debugging feature I am working on in DevTools. I don't attempt to offer all the possible options and features one might want with a TLS server, as there are quite a few. Those could be added later, if someone needs them. Try: https://tbpl.mozilla.org/?tree=Try&rev=32d68a1ced7c

Fixed a few try build and test issues. Try: https://tbpl.mozilla.org/?tree=Try&rev=1100818ab271

I can steal this back if you want.

Comment on attachment 8474628 [details] [diff] [review] Add a basic scriptable TLS server (v2) Review of attachment 8474628 [details] [diff] [review]: ----------------------------------------------------------------- I didn't read the whole patch, because we need to resolve this nudge thing. I see you've used the timer approach from TunnelUtils. That's not a good model for you - its a crazy corner case (N https streams multiplexed on top of another https stream) where the underlying TCP event driven I/O functions a] map poorly due to the mux and b] are scattered to multiple PR_FileDesc. Even with all that I would like to improve the situation, its just a fairly low priority corner case. So let's not make more of them :) You have a much more straightforward stack - a single prfiledesc with tls on top of tcp in the traditional way. I'm not convinced yet it requires such hackery. (and it was my hackery :() I think when honza says in comment 30: "My idea was to give the socket in onSocketAccepted of your server listener in a state with all the security information already checked/filled on it. I don't understand why you should open socket stream inside your server implementation. You have (must have) access to the PRFileDesc, you don't need to do it. Setup the socket, PR_Write(0) it, then give to the consumer when PR_Write doesn't return WOULD_BLOCK, done." he has it right. You can go forward with either honza or I as reviewer here.. honza has expressed an interest and that's cool - you might just check with him first to see what his queue looks like.

Patrick and I discussed one path to removing the timer is to bring back a "security observer" which is notified of TLS handshake completion, making it a separate step after accepting a new client socket. This allows the streams to be opened, moving the TLS state forward in a more natural way. We've now come full circle back to my earlier approach, but at least the timers are gone! :) Try: https://tbpl.mozilla.org/?tree=Try&rev=6aea2c7ad11f

Comment on attachment 8475366 [details] [diff] [review] Add a basic scriptable TLS server (v3) Review of attachment 8475366 [details] [diff] [review]: ----------------------------------------------------------------- jryans - thanks for doing the work! honza - I think the basic alignment here is good: its clear when the security information is valid thanks to the onHandshakeDone event, and the new IDL operates on a nsISocketTransport basis instead of streams, which is how nsIServerSocket works currently. (and pretty much how we work on the client side too). So I think this is a reasonable way to go.. let's discuss if it needs discussing :) honza - would you mind finishing up the other parts of the review? There is a bunch of details here and you're ahead of me in being familiar with it.. If you need to queue it due to more pressing reviews just toss it back at me and I'll get it tomorrow.

I could get to this tomorrow as well.

(In reply to Honza Bambas (:mayhemer) from comment #38) > I could get to this tomorrow as well. great!

J. Ryan, I've started the review now. In comment 36 it seems like there is again a need for the handshake-done callback. I was off the discussion, can you please outline shortly why? I think I understand, just want to be sure I'm on the same tune. Thanks!

(In reply to Honza Bambas (:mayhemer) from comment #40) > J. Ryan, I've started the review now. In comment 36 it seems like there is > again a need for the handshake-done callback. I was off the discussion, can > you please outline shortly why? I think I understand, just want to be sure > I'm on the same tune. Thanks! By adding |onHandshakeDone| instead of waiting until the security info is ready, we are able to alert the server consumer code of the new client immediately at PR_Accept time via |onSocketAttached|. Then, they will (typically) open the streams in their |onSocketAttached| handler. This is sufficient to keep the TLS handshake process moving along under the covers without further tricks, like PR_Write(0). If we were to leave out |onHandshakeDone| as you had suggested earlier on, it was not enough to just do PR_Write(0) at PR_Accept time, as the client has not yet transmitted it's side of the TLS handshake, even in a unit test, as it's not until several ticks later that these bytes are available for reading. So, we would need to somehow wait for the client to deliver data and PR_Write(0) again. I initially threw in a timer for this purpose, which works, but as Patrick states in comment 35, it's quite a hack. Another option would be for the server implementation (so inside TLSServerSocket.cpp, not in the consumer code) to open the streams and async wait on them. But, this goes against the pattern set out by nsIServerSocket, which is that the *consumer* opens the streams after the |onSocketAttached| handler is called. So, by having |onHandshakeDone| we get 3 benefits: * there is a defined point where the consumer is alerted to security info being available * no PR_Write(0) tricks are needed * we preserve the pattern on nsIServerSocket where the consumer opens the streams If you're still unsure, please ping me on IRC! :)

Comment on attachment 8475366 [details] [diff] [review] Add a basic scriptable TLS server (v3) Review of attachment 8475366 [details] [diff] [review]: ----------------------------------------------------------------- We are getting close hese! Still, few details need to handled. Sum: - some potential unprotected inter thread access on the server FD, or just missing access-only-before-asyncListen enforcement - generally missing comments on IDLs (!!!!) ::: netwerk/base/public/nsITLSServerSocket.idl @@ +35,5 @@ > + > + /** > + * sessionTickets > + * > + * Whether the server should use support session tickets. Defaults to true. use or support? @@ +46,5 @@ > + * Whether the server should request and/or require a client auth certificate > + * from the client. Defaults to REQUEST_NEVER. See the possible options > + * below. > + */ > + attribute unsigned long requestCertificate; call this "requestClientCertificate", it's confusing otherwise. @@ +50,5 @@ > + attribute unsigned long requestCertificate; > + > + /** > + * Values for requestCertificate > + */ nit, maybe put the constants before the attribute they relate to. @@ +64,5 @@ > + const unsigned long REQUIRE_ALWAYS = 4; > +}; > + > +[scriptable, uuid(19668ea4-e5ad-4182-9698-7e890d48f327)] > +interface nsITLSClientStatus : nsISupports comment what does this interface do. @@ +81,5 @@ > + * The version of TLS used by the connection. See values below. > + */ > + readonly attribute short tlsVersionUsed; > + > + /* These values are defined by TLS. */ nit, again, consts should be above the attr. @@ +112,5 @@ > + */ > + readonly attribute unsigned long macLength; > +}; > + > +interface nsISocketTransport; declare at the top with other interfaces @@ +115,5 @@ > + > +interface nsISocketTransport; > + > +[scriptable, uuid(64687740-e524-45ea-9786-aed078479d7a)] > +interface nsITLSServerConnectionInfo : nsISupports again, what is this? where do I get it? what are the values? when they are accessible? is it thread safe? so many comments needed here... @@ +132,5 @@ > + * This method is called once the TLS handshake is completed. This takes > + * place after |onSocketAccepted| has been called, which typically opens the > + * streams to keep things moving along. It's important to be aware that the > + * handshake has not completed at the point that |onSocketAccepted| is called, > + * so no security verification can be done until this method is called. hmm.. would not be the target consumer (the target object that grabs and reads/writes the socket) be more interested in this notification? I think yes, since it gives much more flexibility and code cleanness, if you don't have a counter use case. Hence, the observer should rather be set on the nsITLSClientStatus or nsITLSServerConnectionInfo and work as a per-socket callback rather then just globally for a single only callback. Few words on thread-safety needed. ::: netwerk/base/src/TLSServerSocket.cpp @@ +84,5 @@ > + > + RefPtr<TLSServerConnectionInfo> info = new TLSServerConnectionInfo(); > + info->mServerSocket = this; > + info->mTransport = trans; > + info->mClientFD = aClientFD; I'm missing the purpose of storing FD on the info object. It's BTW dangerous since there is no intelligent refcounting as on xpcom objects. You may well refer a dead object and a use on it is actually a very serious security bug. @@ +104,5 @@ > + // Notify the consumer of the new client so it can manage the streams. > + // Security details aren't known yet. The security observer will be notified > + // later when they are ready. > + nsCOMPtr<nsIServerSocket> serverSocket = > + do_QueryInterface(NS_ISUPPORTS_CAST(nsITLSServerSocket*, this)); really needed this QI dance? this should implicitly cast to nsIServerSocket IMO. @@ +141,5 @@ > + PRBool isServer) > +{ > + // Allow any client cert here, server consumer code can decide whether it's > + // okay after being notified of the new client socket. > + return SECSuccess; Only advantage to hook (by means of letting the consumer decide) here would be some possibility to save resources by killing the connection before the full handshake is done. But this approach seems to me simpler, also because for the decision some of the clients data payload may be needed. So, I'm OK with this approach :) @@ +157,5 @@ > + nsCOMPtr<nsIX509CertDB> certDB = > + do_GetService(NS_X509CERTDB_CONTRACTID, &rv); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + info->mTransport->Close(rv); > + return; I usually prefer to have an "Internal" helper method returning nsresult (or bool) and the calling method will cleanup on a failure result of that Internal method. something like: HandshakeCallback(..) { rv = HandshakeCallbackInternal(..); if (NS_FAILED(rv)) { info->mTransport->Close(); } return rv; } @@ +246,5 @@ > + }; > + > +private: > + nsMainThreadPtrHandle<nsITLSServerSecurityObserver> mListener; > + nsCOMPtr<nsIEventTarget> mTargetThread; hmm.. you use main thread holder, but also keep target thread. I'm confused. And no comments on members... @@ +275,5 @@ > +TLSServerSocket::SetSecurityObserver(nsITLSServerSecurityObserver* aObserver) > +{ > + { > + MutexAutoLock lock(mLock); > + mSecurityObserver = new TLSServerSecurityObserverProxy(aObserver); swap to a local com ptr that will release the old observer outside the lock, this is important! @@ +295,5 @@ > + > +NS_IMETHODIMP > +TLSServerSocket::SetServerCert(nsIX509Cert* aCert) > +{ > + mServerCert = aCert; do you want to limit this only to happen before AsyncListen? I would tend to and this then need to fail when called after it. Otherwise you have threading issues. @@ +302,5 @@ > + > +NS_IMETHODIMP > +TLSServerSocket::GetSessionCache(bool* aEnabled) > +{ > + return NS_ERROR_NOT_IMPLEMENTED; do you intend to implement them? if not, maybe don't have these as attributes but only as setXXX() methods on the interface. @@ +308,5 @@ > + > +NS_IMETHODIMP > +TLSServerSocket::SetSessionCache(bool aEnabled) > +{ > + SSL_OptionSet(mFD, SSL_NO_CACHE, !aEnabled); what about thread safety? or can it be used only before asyncListen? if so, I think this is OK but must be enforced. ::: netwerk/base/src/nsServerSocket.h @@ +8,5 @@ > > #include "nsASocketHandler.h" > #include "nsIServerSocket.h" > #include "mozilla/Mutex.h" > +#include "mozilla/net/DNS.h" what's this for? if needed in the cpp file, include there, not in the header ::: netwerk/base/src/nsSocketTransport2.cpp @@ +947,5 @@ > + const NetAddr* aAddr, > + nsISupports* aSecInfo) > +{ > + mSecInfo = aSecInfo; > + return InitWithConnectedSocket(aFD, aAddr); 4 spaces (keep the file style)

Thanks for the review! I'm glad we're able to make progress here. :) (In reply to Honza Bambas (:mayhemer) from comment #42) > ::: netwerk/base/public/nsITLSServerSocket.idl > @@ +35,5 @@ > > + > > + /** > > + * sessionTickets > > + * > > + * Whether the server should use support session tickets. Defaults to true. > > use or support? Changed to support. > @@ +46,5 @@ > > + * Whether the server should request and/or require a client auth certificate > > + * from the client. Defaults to REQUEST_NEVER. See the possible options > > + * below. > > + */ > > + attribute unsigned long requestCertificate; > > call this "requestClientCertificate", it's confusing otherwise. Okay, renamed. > @@ +50,5 @@ > > + attribute unsigned long requestCertificate; > > + > > + /** > > + * Values for requestCertificate > > + */ > > nit, maybe put the constants before the attribute they relate to. Okay, done. > @@ +64,5 @@ > > + const unsigned long REQUIRE_ALWAYS = 4; > > +}; > > + > > +[scriptable, uuid(19668ea4-e5ad-4182-9698-7e890d48f327)] > > +interface nsITLSClientStatus : nsISupports > > comment what does this interface do. Added comments on what it is, where to get, and when values are ready. > @@ +81,5 @@ > > + * The version of TLS used by the connection. See values below. > > + */ > > + readonly attribute short tlsVersionUsed; > > + > > + /* These values are defined by TLS. */ > > nit, again, consts should be above the attr. Okay, done. > @@ +112,5 @@ > > + */ > > + readonly attribute unsigned long macLength; > > +}; > > + > > +interface nsISocketTransport; > > declare at the top with other interfaces Moved. > @@ +115,5 @@ > > + > > +interface nsISocketTransport; > > + > > +[scriptable, uuid(64687740-e524-45ea-9786-aed078479d7a)] > > +interface nsITLSServerConnectionInfo : nsISupports > > again, what is this? where do I get it? what are the values? when they > are accessible? is it thread safe? so many comments needed here... Added various comments about the interface and its attributes. > @@ +132,5 @@ > > + * This method is called once the TLS handshake is completed. This takes > > + * place after |onSocketAccepted| has been called, which typically opens the > > + * streams to keep things moving along. It's important to be aware that the > > + * handshake has not completed at the point that |onSocketAccepted| is called, > > + * so no security verification can be done until this method is called. > > hmm.. would not be the target consumer (the target object that grabs and > reads/writes the socket) be more interested in this notification? I think > yes, since it gives much more flexibility and code cleanness, if you don't > have a counter use case. Hence, the observer should rather be set on the > nsITLSClientStatus or nsITLSServerConnectionInfo and work as a per-socket > callback rather then just globally for a single only callback. Okay, this seems reasonable to me. I have moved the observer to nsITLSServerConnectionInfo. > Few words on thread-safety needed. The thread safety of what exactly...? I am not sure what questions you have that you'd like the comments to answer here. I am not anticipating cross-thread problems here, but maybe you see something? > ::: netwerk/base/src/TLSServerSocket.cpp > @@ +84,5 @@ > > + > > + RefPtr<TLSServerConnectionInfo> info = new TLSServerConnectionInfo(); > > + info->mServerSocket = this; > > + info->mTransport = trans; > > + info->mClientFD = aClientFD; > > I'm missing the purpose of storing FD on the info object. It's BTW > dangerous since there is no intelligent refcounting as on xpcom objects. > You may well refer a dead object and a use on it is actually a very serious > security bug. I believe it was used in a previous version without the |onHandshakeDone| setup. Anyway, I've removed it as it's no longer used. > @@ +104,5 @@ > > + // Notify the consumer of the new client so it can manage the streams. > > + // Security details aren't known yet. The security observer will be notified > > + // later when they are ready. > > + nsCOMPtr<nsIServerSocket> serverSocket = > > + do_QueryInterface(NS_ISUPPORTS_CAST(nsITLSServerSocket*, this)); > > really needed this QI dance? this should implicitly cast to nsIServerSocket > IMO. As far as I can tell, it is needed. Here's what I get without it: TLSServerSocket.cpp:106:44: error: ambiguous conversion from derived class 'mozilla::net::TLSServerSocket' to base class 'nsIServerSocket': class mozilla::net::TLSServerSocket -> class nsServerSocket -> class nsIServerSocket class mozilla::net::TLSServerSocket -> class nsITLSServerSocket -> class nsIServerSocket nsCOMPtr<nsIServerSocket> serverSocket = this; > @@ +157,5 @@ > > + nsCOMPtr<nsIX509CertDB> certDB = > > + do_GetService(NS_X509CERTDB_CONTRACTID, &rv); > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + info->mTransport->Close(rv); > > + return; > > I usually prefer to have an "Internal" helper method returning nsresult (or > bool) and the calling method will cleanup on a failure result of that > Internal method. Makes sense, I've changed to this pattern. > @@ +246,5 @@ > > + }; > > + > > +private: > > + nsMainThreadPtrHandle<nsITLSServerSecurityObserver> mListener; > > + nsCOMPtr<nsIEventTarget> mTargetThread; > > hmm.. you use main thread holder, but also keep target thread. I'm > confused. And no comments on members... I was following the pattern of ServerSocketListenerProxy in nsServerSocket.cpp. But now that you mention it, I don't see why the target thread is needed here either, so I've removed it and now dispatch to the main thread directly. > @@ +275,5 @@ > > +TLSServerSocket::SetSecurityObserver(nsITLSServerSecurityObserver* aObserver) > > +{ > > + { > > + MutexAutoLock lock(mLock); > > + mSecurityObserver = new TLSServerSecurityObserverProxy(aObserver); > > swap to a local com ptr that will release the old observer outside the lock, > this is important! Sorry for missing this. I've added it to ~TLSServerConnectionInfo. > @@ +295,5 @@ > > + > > +NS_IMETHODIMP > > +TLSServerSocket::SetServerCert(nsIX509Cert* aCert) > > +{ > > + mServerCert = aCert; > > do you want to limit this only to happen before AsyncListen? I would tend > to and this then need to fail when called after it. Otherwise you have > threading issues. I've added a check for this case and now return an error. > @@ +302,5 @@ > > + > > +NS_IMETHODIMP > > +TLSServerSocket::GetSessionCache(bool* aEnabled) > > +{ > > + return NS_ERROR_NOT_IMPLEMENTED; > > do you intend to implement them? if not, maybe don't have these as > attributes but only as setXXX() methods on the interface. No, I do not. I've changed them to setter methods. > @@ +308,5 @@ > > + > > +NS_IMETHODIMP > > +TLSServerSocket::SetSessionCache(bool aEnabled) > > +{ > > + SSL_OptionSet(mFD, SSL_NO_CACHE, !aEnabled); > > what about thread safety? or can it be used only before asyncListen? if so, > I think this is OK but must be enforced. I think it's only logical to set these before |AsyncListen|. I've changed them to error if they are called after that. > ::: netwerk/base/src/nsServerSocket.h > @@ +8,5 @@ > > > > #include "nsASocketHandler.h" > > #include "nsIServerSocket.h" > > #include "mozilla/Mutex.h" > > +#include "mozilla/net/DNS.h" > > what's this for? if needed in the cpp file, include there, not in the header It appeared because I declared a new method using NetAddr. But, I was able to forward declare it and remove the include here. > ::: netwerk/base/src/nsSocketTransport2.cpp > @@ +947,5 @@ > > + const NetAddr* aAddr, > > + nsISupports* aSecInfo) > > +{ > > + mSecInfo = aSecInfo; > > + return InitWithConnectedSocket(aFD, aAddr); > > 4 spaces (keep the file style) Fixed. Try: https://tbpl.mozilla.org/?tree=Try&rev=ac00bacc0617

Comment on attachment 8478491 [details] [diff] [review] Add a basic scriptable TLS server (v4) Review of attachment 8478491 [details] [diff] [review]: ----------------------------------------------------------------- r- for leaks. ::: netwerk/base/public/nsITLSServerSocket.idl @@ +69,5 @@ > + * This is accessible through the security info object on the transport, which > + * will be an instance of |nsITLSServerConnectionInfo| (see below). > + * > + * The values of these attributes are available once the |onHandshakeDone| > + * method the security observer has been called (see method of the ...? @@ +181,5 @@ > + * handshake has not completed at the point that |onSocketAccepted| is called, > + * so no security verification can be done until this method is called. > + */ > + void onHandshakeDone(in nsITLSServerSocket aServer, > + in nsISocketTransport aTransport); might be more interesting to give the nsITLSClientStatus rather, which now is ready to use ::: netwerk/base/src/TLSServerSocket.cpp @@ +96,5 @@ > + SSL_AuthCertificateHook(aClientFD, AuthCertificateHook, nullptr); > + // Once the TLS handshake has completed, the server consumer is notified and > + // has access to various TLS state details. > + SSL_HandshakeCallback(aClientFD, TLSServerConnectionInfo::HandshakeCallback, > + info); please add a comment that info cannot go away, since it's referenced by the socket transport that is going to live as long as the socket. @@ +242,5 @@ > + { > + public: > + OnHandshakeDoneRunnable(const nsMainThreadPtrHandle<nsITLSServerSecurityObserver>& aListener, > + nsITLSServerSocket* aServer, > + nsISocketTransport* aTransport) nit: indent @@ +306,5 @@ > + > + nsITLSServerSecurityObserver* observer; > + { > + MutexAutoLock lock(mLock); > + mSecurityObserver.swap(observer); use forget(). this way you will release a random memory.. have you tested this code? no, you didn't because you leak.. @@ +412,5 @@ > +TLSServerConnectionInfo::HandshakeCallback(PRFileDesc* fd, void* arg) > +{ > + nsresult rv = HandshakeCallbackInternal(fd, arg); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + RefPtr<TLSServerConnectionInfo> info = use nsRefPtr, OTOH, why do you need to refer at all? @@ +419,5 @@ > + } > +} > + > +nsresult > +TLSServerConnectionInfo::HandshakeCallbackInternal(PRFileDesc* fd, void* arg) nit: slightly more prefered would be to take TLSServerConnectionInfo instead of void. @@ +433,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + > + nsCOMPtr<nsIX509Cert> nsClientCert; clientCertPSM (don't use the ns prefix)

recorded as: $ export XPCOM_MEM_BLOAT_LOG=leaks.log $ ./mach xpcshell-test netwerk/test/unit/test_tls_server.js --sequential

(In reply to Honza Bambas (:mayhemer) from comment #44) > ::: netwerk/base/public/nsITLSServerSocket.idl > @@ +69,5 @@ > > + * This is accessible through the security info object on the transport, which > > + * will be an instance of |nsITLSServerConnectionInfo| (see below). > > + * > > + * The values of these attributes are available once the |onHandshakeDone| > > + * method the security observer has been called (see > > method of the ...? Fixed. > @@ +181,5 @@ > > + * handshake has not completed at the point that |onSocketAccepted| is called, > > + * so no security verification can be done until this method is called. > > + */ > > + void onHandshakeDone(in nsITLSServerSocket aServer, > > + in nsISocketTransport aTransport); > > might be more interesting to give the nsITLSClientStatus rather, which now > is ready to use Okay, I've added nsITLSClientStatus as an additional parameter. I think it's still nice to make the socket and transport available too, in case the server consumer uses a singleton security observer for all client connections (so no other state is needed to close a bad connection, for example), so I've left those as parameters. > ::: netwerk/base/src/TLSServerSocket.cpp > @@ +96,5 @@ > > + SSL_AuthCertificateHook(aClientFD, AuthCertificateHook, nullptr); > > + // Once the TLS handshake has completed, the server consumer is notified and > > + // has access to various TLS state details. > > + SSL_HandshakeCallback(aClientFD, TLSServerConnectionInfo::HandshakeCallback, > > + info); > > please add a comment that info cannot go away, since it's referenced by the > socket transport that is going to live as long as the socket. Okay, I've added a note about this. > @@ +242,5 @@ > > + { > > + public: > > + OnHandshakeDoneRunnable(const nsMainThreadPtrHandle<nsITLSServerSecurityObserver>& aListener, > > + nsITLSServerSocket* aServer, > > + nsISocketTransport* aTransport) > > nit: indent Fixed. > @@ +306,5 @@ > > + > > + nsITLSServerSecurityObserver* observer; > > + { > > + MutexAutoLock lock(mLock); > > + mSecurityObserver.swap(observer); > > use forget(). this way you will release a random memory.. have you tested > this code? no, you didn't because you leak.. Okay, I've changed to forget(), which does seem more clear. However, this part itself was not the source of leaks. Thanks for pointing out the leak report! I wasn't sure how to check that before. The main issue was not this swap() call, but instead there was a cycle between nsSocketTransport and TLSServerConnectionInfo. I've changed TLSServerConnectionInfo to store a raw pointer (weak ref) to the transport, since the transport will hold the TLSServerConnectionInfo as |mSecInfo| and added a comment about this to the header. Now the leaks report looks much better (only has nsLocalFile and nsStringBuffer, which seems to happen for existing tests that I try). > @@ +412,5 @@ > > +TLSServerConnectionInfo::HandshakeCallback(PRFileDesc* fd, void* arg) > > +{ > > + nsresult rv = HandshakeCallbackInternal(fd, arg); > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + RefPtr<TLSServerConnectionInfo> info = > > use nsRefPtr, OTOH, why do you need to refer at all? Changed to nsRefPtr. I need to get the transport in case I need to close it, since this is a static method. Also, I now pass it into the HandshakeCallbackInternal method. > @@ +419,5 @@ > > + } > > +} > > + > > +nsresult > > +TLSServerConnectionInfo::HandshakeCallbackInternal(PRFileDesc* fd, void* arg) > > nit: slightly more prefered would be to take TLSServerConnectionInfo instead > of void. Changed. > @@ +433,5 @@ > > + if (NS_FAILED(rv)) { > > + return rv; > > + } > > + > > + nsCOMPtr<nsIX509Cert> nsClientCert; > > clientCertPSM (don't use the ns prefix) Renamed. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3fbd1c1bd596

(In reply to J. Ryan Stinnett [:jryans] from comment #46) > > use forget(). this way you will release a random memory.. have you tested > > this code? no, you didn't because you leak.. > > Okay, I've changed to forget(), which does seem more clear. However, this > part > itself was not the source of leaks. No. I wanted you to use forget() since otherwise it would cause a really bad crash. But thanks the leak, this code didn't even trigger. > > Thanks for pointing out the leak report! I wasn't sure how to check that > before. The main issue was not this swap() call, I know. > but instead there was a > cycle > between nsSocketTransport and TLSServerConnectionInfo. I've changed > TLSServerConnectionInfo to store a raw pointer (weak ref) to the transport, > since the transport will hold the TLSServerConnectionInfo as |mSecInfo| and > added a comment about this to the header. That's wrong, I'll comment in the patch. > > Now the leaks report looks much better (only has nsLocalFile and > nsStringBuffer, > which seems to happen for existing tests that I try). Hmm.. you must have 0 leaks report. Just run your single test, that is enough to check. If and only if that has no remaining objects in the leak report, you are OK.

Comment on attachment 8486188 [details] [diff] [review] Add a basic scriptable TLS server (v5) Review of attachment 8486188 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsITLSServerSocket.idl @@ +156,5 @@ > + * transport > + * > + * The nsISocketTransport used to communicate with the client. > + */ > + readonly attribute nsISocketTransport transport; remove this attribute, there is no need to have it. The transport is a redundant information and causes just problems with cycles. ::: netwerk/base/src/TLSServerSocket.cpp @@ +413,5 @@ > + return NS_OK; > +} > + > +void > +TLSServerConnectionInfo::HandshakeCallback(PRFileDesc* aFD, void* aArg) please mark ALL methods that are static us with // static comment over their definitions @@ +419,5 @@ > + nsRefPtr<TLSServerConnectionInfo> info = > + static_cast<TLSServerConnectionInfo*>(aArg); > + nsresult rv = HandshakeCallbackInternal(aFD, info); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + info->mTransport->Close(rv); don't do this, just close aFd with some reasonable error code. the transport's streams consumers will be notified on read/write with an error. @@ +425,5 @@ > +} > + > +nsresult > +TLSServerConnectionInfo::HandshakeCallbackInternal(PRFileDesc* aFD, > + TLSServerConnectionInfo* aInfo) I'd little bit more prefer this to be a normal member class rather then a static method. @@ +479,5 @@ > + nsCOMPtr<nsISocketTransport> transport; > + GetTransport(getter_AddRefs(transport)); > + nsCOMPtr<nsITLSServerSocket> serverSocket; > + GetServerSocket(getter_AddRefs(serverSocket)); > + mSecurityObserver->OnHandshakeDone(serverSocket, transport, this); please do the following here (missed the last time): - swap mSecurityObserver to a local nsRefPtr<> - use that local refptr to call OnHandshakeDone this way you safely nullify mSecurityObserver before the notification has been done and there is no need the observer should be referenced further after this point. ::: netwerk/base/src/TLSServerSocket.h @@ +62,5 @@ > + TLSServerConnectionInfo* aInfo); > + > + nsRefPtr<TLSServerSocket> mServerSocket; > + // weak ref to the transport, which owns this as mSecInfo > + nsISocketTransport* mTransport; This is absolutely wrong. The TLSServerConnectionInfo instance can live longer then the transports and here you will then hold a bad pointer accessible via a public getter. Very wrong. Actually I don't see a point why you should hold this as a member at all. If you want to have it here to provide the transport in your HandshakeDone callback, then drop it immediately after you called that callback.

(In reply to Honza Bambas (:mayhemer) from comment #47) > > > > Now the leaks report looks much better (only has nsLocalFile and > > nsStringBuffer, > > which seems to happen for existing tests that I try). > > Hmm.. you must have 0 leaks report. Just run your single test, that is > enough to check. If and only if that has no remaining objects in the leak > report, you are OK. Even without this patch applied, every XPCShell test I have tried leaks those two objects. My guess is that they contain the log output itself, or something like that. I am running on OS X. Perhaps this is a platform difference? In any case, considering that's what I get without my patch applied, I am treating it as the "baseline" goal. They remain the only objects that show up after this patch is applied. (In reply to Honza Bambas (:mayhemer) from comment #48) > Comment on attachment 8486188 [details] [diff] [review] > Add a basic scriptable TLS server (v5) > > Review of attachment 8486188 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/public/nsITLSServerSocket.idl > @@ +156,5 @@ > > + * transport > > + * > > + * The nsISocketTransport used to communicate with the client. > > + */ > > + readonly attribute nsISocketTransport transport; > > remove this attribute, there is no need to have it. The transport is a > redundant information and causes just problems with cycles. Okay, I've removed it. > ::: netwerk/base/src/TLSServerSocket.cpp > @@ +413,5 @@ > > + return NS_OK; > > +} > > + > > +void > > +TLSServerConnectionInfo::HandshakeCallback(PRFileDesc* aFD, void* aArg) > > please mark ALL methods that are static us with // static comment over their > definitions I've marked them all. > @@ +419,5 @@ > > + nsRefPtr<TLSServerConnectionInfo> info = > > + static_cast<TLSServerConnectionInfo*>(aArg); > > + nsresult rv = HandshakeCallbackInternal(aFD, info); > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + info->mTransport->Close(rv); > > don't do this, just close aFd with some reasonable error code. the > transport's streams consumers will be notified on read/write with an error. I changed this to PR_Close(aFD). Since this function has no return value, and I did not see a common pattern for setting an error code some other way after PR_Close, I have not set any kind of error code. Let me know if there's something else you intended here. > @@ +425,5 @@ > > +} > > + > > +nsresult > > +TLSServerConnectionInfo::HandshakeCallbackInternal(PRFileDesc* aFD, > > + TLSServerConnectionInfo* aInfo) > > I'd little bit more prefer this to be a normal member class rather then a > static method. That does seem more natural! I've made it a class member. > @@ +479,5 @@ > > + nsCOMPtr<nsISocketTransport> transport; > > + GetTransport(getter_AddRefs(transport)); > > + nsCOMPtr<nsITLSServerSocket> serverSocket; > > + GetServerSocket(getter_AddRefs(serverSocket)); > > + mSecurityObserver->OnHandshakeDone(serverSocket, transport, this); > > please do the following here (missed the last time): > > - swap mSecurityObserver to a local nsRefPtr<> > - use that local refptr to call OnHandshakeDone > > this way you safely nullify mSecurityObserver before the notification has > been done and there is no need the observer should be referenced further > after this point. Alright, I've made this change. > ::: netwerk/base/src/TLSServerSocket.h > @@ +62,5 @@ > > + TLSServerConnectionInfo* aInfo); > > + > > + nsRefPtr<TLSServerSocket> mServerSocket; > > + // weak ref to the transport, which owns this as mSecInfo > > + nsISocketTransport* mTransport; > > This is absolutely wrong. The TLSServerConnectionInfo instance can live > longer then the transports and here you will then hold a bad pointer > accessible via a public getter. Very wrong. > > Actually I don't see a point why you should hold this as a member at all. > If you want to have it here to provide the transport in your HandshakeDone > callback, then drop it immediately after you called that callback. So that we don't have to worry about the cycles, I chose to just remove the transport entirely. It would have been nice to have in the HandshakeDone callback, but this is simpler, so I'll just leave it out for now. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e53bcd8afa1c

Comment on attachment 8488899 [details] [diff] [review] Add a basic scriptable TLS server (v6) Review of attachment 8488899 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/TLSServerSocket.cpp @@ +404,5 @@ > + nsRefPtr<TLSServerConnectionInfo> info = > + static_cast<TLSServerConnectionInfo*>(aArg); > + nsresult rv = info->HandshakeCallbackInternal(aFD); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + PR_Close(aFD); I found out I didn't give you a good advice on this one, sorry. You cannot close the fd from here for the following reasons: 1) you cannot close the fd on the main thread because of possible thread races (this actually pops the ssl layer) 2) I think consumer will see this only as a graceful or interrupt shutdown, so hard to decide what was wrong It seems like closing the transport is the correct way to do here.. The thing is how to reach the transport from here. I'm not sure this callback is invoked even when the negotiation didn't pass, so you cannot strong refer the transport from the info. Weak ref is bad, you have already given the info away to the consumer. However, when the mTransport member on the info will be inaccessible (cannot be dereferred) other way then in this callback, we could go with it (keep the weak ref on the info, that will be nullified here and otherwise inaccessible). @@ +455,5 @@ > + return NS_OK; > +} > + > +void > +TLSServerConnectionInfo::OnHandshakeDone() what's purpose of having this method? I don't see a reason to have it. Just merge the code to TLSServerConnectionInfo::HandshakeCallbackInternal ::: netwerk/base/src/TLSServerSocket.h @@ +57,5 @@ > +private: > + virtual ~TLSServerConnectionInfo(); > + > + static void HandshakeCallback(PRFileDesc* aFD, void* aArg); > + nsresult HandshakeCallbackInternal(PRFileDesc* aFD); no longer needs to be called Internal

(In reply to Honza Bambas (:mayhemer) from comment #50) > Comment on attachment 8488899 [details] [diff] [review] > Add a basic scriptable TLS server (v6) > > Review of attachment 8488899 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/src/TLSServerSocket.cpp > @@ +404,5 @@ > > + nsRefPtr<TLSServerConnectionInfo> info = > > + static_cast<TLSServerConnectionInfo*>(aArg); > > + nsresult rv = info->HandshakeCallbackInternal(aFD); > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + PR_Close(aFD); > > I found out I didn't give you a good advice on this one, sorry. You cannot > close the fd from here for the following reasons: > 1) you cannot close the fd on the main thread because of possible thread > races (this actually pops the ssl layer) > 2) I think consumer will see this only as a graceful or interrupt shutdown, > so hard to decide what was wrong > > It seems like closing the transport is the correct way to do here.. The > thing is how to reach the transport from here. I'm not sure this callback > is invoked even when the negotiation didn't pass, so you cannot strong refer > the transport from the info. Weak ref is bad, you have already given the > info away to the consumer. However, when the mTransport member on the info > will be inaccessible (cannot be dereferred) other way then in this callback, > we could go with it (keep the weak ref on the info, that will be nullified > here and otherwise inaccessible). Ah, okay, makes sense. I've changed back to the weak ref just for this part, and then I null it out after. > @@ +455,5 @@ > > + return NS_OK; > > +} > > + > > +void > > +TLSServerConnectionInfo::OnHandshakeDone() > > what's purpose of having this method? I don't see a reason to have it. > Just merge the code to TLSServerConnectionInfo::HandshakeCallbackInternal Okay, I've merged it. > ::: netwerk/base/src/TLSServerSocket.h > @@ +57,5 @@ > > +private: > > + virtual ~TLSServerConnectionInfo(); > > + > > + static void HandshakeCallback(PRFileDesc* aFD, void* aArg); > > + nsresult HandshakeCallbackInternal(PRFileDesc* aFD); > > no longer needs to be called Internal Removed Internal from the name. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fea2c3df0465

Comment on attachment 8490399 [details] [diff] [review] Add a basic scriptable TLS server (v7) Review of attachment 8490399 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Looks good. r=honzab (pleas fix the commit message) ::: netwerk/base/src/TLSServerSocket.cpp @@ +409,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + info->mTransport->Close(rv); > + } > + // No longer needed, so clear the weak ref > + info->mTransport = nullptr; I'd a little bit more prefer to move mTransport to a local var first, then nullify mTransport, and call Close() via the local var. That's a nit, but would be nice to have.

(In reply to Honza Bambas (:mayhemer) from comment #52) > Comment on attachment 8490399 [details] [diff] [review] > Add a basic scriptable TLS server (v7) > > Review of attachment 8490399 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! Looks good. r=honzab (pleas fix the commit message) Thanks for the reviews! Updated reviewer name. > ::: netwerk/base/src/TLSServerSocket.cpp > @@ +409,5 @@ > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + info->mTransport->Close(rv); > > + } > > + // No longer needed, so clear the weak ref > > + info->mTransport = nullptr; > > I'd a little bit more prefer to move mTransport to a local var first, then > nullify mTransport, and call Close() via the local var. That's a nit, but > would be nice to have. Okay, I've made this change. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=31da9e77d295

sorry had to back out this change for breaking non-unified builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=48449691&tree=Mozilla-Inbound

Fixed non-unified build issues. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=74eb72f243cb