Various Remarks

[jens-diewald]

Hi!
First of all, thanks for putting together this very useful library.
I have been using beast+asio(and hence openssl)+djarek/certify for a simple https client so far.
But i do prefer your approach and would like to replace openssl+certify by your library.
Here are a few notes from first tests:

1. Server hostnames are not verified during certificate validation. (Testcase: wrong.host.badssl.com:443)
   This should be a relatively easy fix if i am not missing something. I can add a small pull request later for further details.
2. There are several more (probably less important) test cases on badssl.com, that are not handled as the site suggests they should be.
   Checking a revocation list should be relatively straight forward, not sure about the other cases.
3. Trying to connect to client.badssl.com:443 yields a failed assert due to client certificates not being implemented. I think it would be nicer to throw an exception in such a case, so the caller can recover from the error. (Since there is Feature: Client SSL Validation #55 now, this particular case is probably not a problem for long though :) )
4. I needed to link against secur32.lib and crypt32.lib to use the library (You have listed even two more in your cmake config). This should be documented somewhere. Maybe we can even have some pragma comment lib for convenience?
5. I am not sure how far you are aiming for compability with asio/ssl, but i ran into a few differences. This is no big issue for me, but a higher compatibility would be nice to allow simple cross platform code to use wintls on windows and asio with openssl on unix systems.
   • wintls has context::use_default_certificates(bool) instead of context::set_default_verify_paths(). I think use_default_certificates is the better (more general) name and i suppose windows does not really have "verify paths". But maybe the other function could be introduced as an alias for compability?
   • wintls has context::verify_server_certificate(bool) instead of context::set_verify_mode(verify_mode). Here the latter is more general, but as i understand it wintls does not (yet) have all the functionality that asio provides here. It would be nice to somewhat align this with asio in the future. (As also mentioned here: [Feature Request] Verify client ssl in server #53 (comment))
   • wintls uses some scoped enums (wintls::method, wintls::handshake_type) which are different from the asio equivalents. I generally like the wintls way much better, not sure what could be done here...
   • There is no wintls::error::stream_truncated. The asio equivalent is a common error that i have to handle. (Not sure what wintls yields in these cases yet.)
   • Many wintls classes have their move constructors explicitly deleted. Is there a good reason for that? At first glance, your usage of unique_ptr should prevent any problems with moves? (Why i noticed: In my original code i had a separate function to initialize the ssl::context, set some parameters as verify_mode, etc. and return it. I had to solve this differently to work with wintls.)

[laudrup]

Hi! First of all, thanks for putting together this very useful library. I have been using beast+asio(and hence openssl)+djarek/certify for a simple https client so far. But i do prefer your approach and would like to replace openssl+certify by your library. Here are a few notes from first tests:

Thanks a lot for the kind words. I'm happy that this is useful.

It's not really something I have any use for myself so it's quite important for me to get any kind of feedback.

1. Server hostnames are not verified during certificate validation. (Testcase: wrong.host.badssl.com:443)
   This should be a relatively easy fix if i am not missing something. I can add a small pull request later for further details.

i've seen you PR. Thanks a lot for that.

2. There are several more (probably less important) test cases on badssl.com, that are not handled as the site suggests they should be.
   Checking a revocation list should be relatively straight forward, not sure about the other cases.

Me neither. I have thought about trying to automate testing against badssl.com one way or another but haven't really had the time to implement it.

Any ideas on how this could be done would be appreciated.

3. Trying to connect to client.badssl.com:443 yields a failed assert due to client certificates not being implemented. I think it would be nicer to throw an exception in such a case, so the caller can recover from the error. (Since there is Feature: Client SSL Validation #55 now, this particular case is probably not a problem for long though :) )

Of course crashing with an assert is not the nice way to handle this. I think I made the assumption that I would implement client certification very soon and left the assertion as a reminder :-)

Anyway, it thankfully seems like youyuanwu is making good progress implementing that.

4. I needed to link against secur32.lib and crypt32.lib to use the library (You have listed even two more in your cmake config). This should be documented somewhere. Maybe we can even have some pragma comment lib for convenience?

Indeed. Well spotted. Since I claim that this library is "header only" I should at least document that these libraries are needed.

I don't really like compiler specific pragmas but I can see that some other boost libraries do this (in an ifdef for msvc) so I could do that as well.

5. I am not sure how far you are aiming for compability with asio/ssl, but i ran into a few differences. This is no big issue for me, but a higher compatibility would be nice to allow simple cross platform code to use wintls on windows and asio with openssl on unix systems.

I originally intended this library to be a "drop in" replacement for asio.ssl but it turned out that this was not really feasible as there are too many differences so I decided to make the context class specific for the library but hopefully the stream class should work the same.

It would probably be a good idea to add an example of how this library can be used alongside asio.ssl. I will consider updating the examples to show how this can be done (and fix the code if it doesn't work).

• wintls has context::use_default_certificates(bool) instead of context::set_default_verify_paths(). I think use_default_certificates is the better (more general) name and i suppose windows does not really have "verify paths". But maybe the other function could be introduced as an alias for compability?

I can see the point in this but since wintls::context and asio::ssl::context are not the same anyway I don't think such an alias would make much sense.

• wintls has context::verify_server_certificate(bool) instead of context::set_verify_mode(verify_mode). Here the latter is more general, but as i understand it wintls does not (yet) have all the functionality that asio provides here. It would be nice to somewhat align this with asio in the future. (As also mentioned here: [Feature Request] Verify client ssl in server #53 (comment))

Indeed that's something that needs to be implemented. Hopefully that shouldn't be too hard to do.

• wintls uses some scoped enums (wintls::method, wintls::handshake_type) which are different from the asio equivalents. I generally like the wintls way much better, not sure what could be done here...

I definitely prefer C++11 scoped enums as well but I think you have a point that it would be nice if the stream class could be used the same way with a simple type alias to either boost::asio::ssl::stream or boost::wintls::stream. Extending the examples as mentioned previously would clarify that. Thanks for pointing this out.

• There is no wintls::error::stream_truncated. The asio equivalent is a common error that i have to handle. (Not sure what wintls yields in these cases yet.)

I have to look into what stream_truncated actually does to be ably comment on this :-)

• Many wintls classes have their move constructors explicitly deleted. Is there a good reason for that? At first glance, your usage of unique_ptr should prevent any problems with moves? (Why i noticed: In my original code i had a separate function to initialize the ssl::context, set some parameters as verify_mode, etc. and return it. I had to solve this differently to work with wintls.)

I think that's simply because I have a habit of deleting constructors unless I'm certain they actually work as intended. I should probably look into that. Thanks.

Once again thanks for your comments. They are very useful.

You have definitely given me something I need to look into. The only issue is that I don't have as much time to work on this library as I would like so of course pull request are also very welcome :-)

[laudrup]

@jens-diewald

I assume we can close this now?

[jens-diewald]

Hi, there are a few small points in here that are still relevant i think:

• Deleted move constructors - i just looked into this a bit and found, that context and stream should indeed be movable without problems. (Actually the move constructor for stream is defaulted, while the move assignment is deleted, i suppose this is not intended?)
  -> I will add two small pull requests to remove the deleting of move operations for these two classes.
• An example how to use wintls alongside asio.ssl as you proposed above would be helpful. I can see if i can add a variant of the https_client example based on my code.
• I did not get to testing regarding asio::ssl::error::stream_truncated. The error is described as The underlying stream closed before the ssl stream gracefully shut down.. This seems to happen in many cases as closing an ssl connection properly is relatively complicated. Maybe there is nothing to do here, but at least for the use case of using wintls alongside asio.ssl it would be nice to know what wintls does in such cases.

[laudrup]

@jens-diewald

• i've merged your PRs that introduced the move cosntructors. Thanks for that. A small but relevant improvement.

• It would be nice to add an example of how to use wintls alongside asio.ssl. A PR for this would be much appreciated.

• I haven't really looked into the details of shutting down the stream correctly although I know there are some tests for that. Maybe those tests could be extended to document what happens?

[jens-diewald]

I have opened #75 for the stream_truncated issue.
I have also done some work on a simple syncronous https client example to show the usage of WinTLS beside Asio.SSL. I am not super happy with it, but i may add a PR for this soon.
Anyhow, i think this can be closed now :)