search

david-maw / StreamSSL

The StreamSSL sample described in CodeProject
Other
47 stars 24 forks source link

Combine client and server in one static library, removed dependencies on MFC #90

Closed jacgoudsmit closed 2 years ago

jacgoudsmit commented 2 years ago

Why this change?

I wanted to combine the SSLClient and SSLServer projects in a project but found out that this was impossible: Both those libraries contain common modules so the linker generated Multiple-Defined Symbol errors.

I tried overriding this (force the linker to generate an executable even though symbols are defined more than once) and ran into weird problems such as uninitialized data. Apparently the linker really meant it when it said it couldn't generate a valid .exe file.

Combined client/server library project

So I created a project combining the common source files with the client-specific files from SSLClient and the server-specific files from SSLServer. And I ran into a problem that the CredentialTraits::Close and SecurityContext::Close functions had different implementations between SSLServer and SSLClient.

Moved the SSPI security function table to a common class

The Close functions of CredentialTraits and SecurityContext basically call a function from the SSPI security function table; the SSLServer and SSLClient class both retrieved that table as a static variable. So the solution was to create a new class SSLCommon, which contains the SSPI secure function table and the SSPI() function to get access to it. I removed the g_SSPI member from the SSLClient and SSLServer classes and modified both classes to use SSLCommon as public base class so no other code that used the SSPI() function would have to be changed.

Dependencies on MFC

The dependencies on MFC had already been removed from the SSLClient project but not from the SSLServer project. So I did the following:

Miscellaneous other changes

Toolkit Versions

I modified the projects to build under Visual Studio 2017 because that's the version I use. This mostly manifests itself as a change from platform toolset v142 to v141. If this is undesirable, it's easy to undo by loading the solution file in a late Visual Studio and telling it to upgrade the projects to the latest. It should ask when you load it the first time.

I updated the target platform to 10.0.19014 because that's the one I have (and I think it's the latest at this time). This should also be easy to change if not desired.

Build system changes

I changed the Configuration Manager so that each configuration (Debug, Debug Ansi, and Release) use only their own project builds.

I changed the project files so they all generate their output to $(SolutionDir)$(Platform)\$(Configuration)\ and their intermediate files to $(SolutionDir)$(Platform)\$(Configuration)\_intermediate_files_\$(ProjectName)\. This puts all output files into two subfolders in the base directory, called Win32 and x64. Visual Studio tends to scatter intermediate files all over the place which is annoying when you have to manually clean things up. With these changes, everything ends up in basically one place.

I changed the project files so that the libraries generate a .pdb file in the output directory. Having a .pdb file makes debugging easier, even if the libraries are built in release mode with optimization.

I changed some other options to eliminate compiler and linker warnings. I tested the build in all platform (Win32/x64) and build config (Debug, Debug Ansi, Release) combinations, and ran the StreamServer example (which itself starts the StreamClient example) to make sure that the result was the same every time. I noticed the Debug Ansi build wasn't excluded in .gitignore so I added it.

Conclusion

I changed the sample programs so they use the new StreamSSL library. I didn't make any changes to basic functionality.

It should be possible to eliminate the SSLClient and SSLServer projects altogether. Whenever you write a program that uses only the SSLServer classes or only the SSLClient classes, and links against StreamSSL (which has both), the .exe will still only contain what's necessary. The linker will ignore the modules in the library that aren't referenced.

Thanks for considering this PR!

===Jac

david-maw commented 2 years ago

Thanks Jac, you've obviously put a lot of work into this. It'll probably take me a few days to review the changes but based on your description none of them seem unreasonable and many represent a significant improvement.

jacgoudsmit commented 2 years ago

Thank you, David.

jacgoudsmit commented 2 years ago

No major comments, but you obviously thought:

      if (false && FAILED(Status))
          Status = CertFindClientCertificate(pCertContext); // Select any valid certificate

ought to be replaced with a comment as below

//        if (FAILED(Status))
//            Status = CertFindClientCertificate(pCertContext); // Select any valid certificate

I didn't do it that way because I wanted the compiler to check the syntax for me and allow this call to be located as a reference. Because the compiler doesn't parse comments I used 'if (false)' instead, not a big deal, but I thought I'd explain my reasoning.

I totally understand your reasoning. The reason I commented it out and changed the expression was that it was generating a compiler warning because your if-expression always evaluated to false. I prefer all my code to build without warnings (I think so do you; I think I noticed the option to not generate a binary in case of warnings was switched on and I left it that way).

I agree there are better ways to do this. Instead of "false" you could use a bool member variable, for example. I myself might have used a virtual function or something.

Anyway: this particular change was not intended as a "you're doing it all wrong" thing; it was just a quick way to fix that compiler warning. Please don't take it (or any other changes) personal, and feel free to use or ignore them as you please.

Thanks!

===Jac

david-maw commented 2 years ago

You are correct, I also prefer code to build without warnings and it's weird I didn't do something about that one - I'll revisit it when I have time. Hopefully, I can use a mechanism that's able to be optimized out by the compiler while avoiding the warning (which I suspect might be impossible, alas). Anyway, I'm fine with releasing it this way and changing it later if I can produce something better. Thanks again for the improvements.