SSLContext::authenticate peer name validation doesn't work

[jmccl]

The 'peerName' parameter sets the 'peerFixedName_' member, which is unused.

[yfeldblum]

It is returned in SSLContext::peerFixedName().

[jmccl]

The comment on the authenticate method is:

@param peerName If non-empty, validate that the certificate common
name of peer matches the given string (altername
name(s) are not used in this case).

This does not occur. Yes, one can access the member via the method you reference, but that's not the extent of what the documentation suggests.

I can re-submit, or you can re-open if you prefer.

[yfeldblum]

@djwatson @ngoyal

[ngoyal]

I'm tempted to remove this parameter. Any objections? This can be done by adding a handshake verifier on the AsyncSSLSocket.

It doesn't appear that checkPeerName is used anywhere either, but can be for logging from the application I guess.

[jmccl]

I think that makes sense, fwiw. It looks like a signature and comment got out ahead of an implementation that never happened.

If you’re going to change the signature you might want to consider removing the second parameter (‘checkPeerName’) as well. It appears to be similarly unused. The only downside I can see is it will break compilation of the wangle client SSL example code.

https://github.com/facebook/wangle/blob/be6d5481fed1e35fab15853f1315d5eebcac0ff7/wangle/example/ssl/Client.cpp#L109

Cleaning the meaningless ‘bool’ up is probably a good thing though.

I have one last observation, which is that the interaction between ‘authenticate’ and ‘setVerificationMode’ isn’t clear, at least to me. My reading of the code is that ‘setVerificationMode’ overrides the ‘authenticate’ behavior only if the SSLContext user (such as AsyncSSLSocket) is properly coded; otherwise the ‘authenticate’ behavior is used. But I could easily be misreading what’s going on. If the interaction happens to be obvious to you updating the comments would be appreciated.