Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new OpenSSL IO Handler for OpenSSL 1.1.1 #299

Open
wants to merge 63 commits into
base: master
Choose a base branch
from

Conversation

mezen
Copy link
Contributor

@mezen mezen commented Apr 29, 2020

Complete new io handler which supports OpenSSL 1.1.1. The existing io handler for OpenSSL 1.0.2 is untouched and should still work.
Only IdCTypes.pas and IdGlobal.pas are modified (the last files in the diff): I added some more types.

The Binaries from http://wiki.overbyte.eu/wiki/index.php/ICS_Download#Download_OpenSSL_Binaries_.28required_for_SSL-enabled_components.29 are used, but at the beginning I tested with slWebPro.

The complete io handler is written with Delphi Berlin 10.1.2, I tried to support older Delphi versions, but due lack of Installations I havent tested it.
Also only Win32 and Win64 are tested, no MacOS, no mobile.
Test are done in small test project and a big (three tier) real world application with tcp server/client, smtp/imap/pop client, ftp client and http client.

This should fix #213, #224 (ok, no event, but virtual method for overriding), #248 (not on purpose for this issue, but for myself - found out later that issue exists^^) and of course #183.

@rlebeau rlebeau self-requested a review May 6, 2020 00:59
@rlebeau rlebeau self-assigned this May 6, 2020
@rlebeau rlebeau added Type: Enhancement Issue is proposing a new feature/enhancement Element: I/O Handlers Issues related to TIdIOHandler and descendants Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants labels May 6, 2020
@rlebeau
Copy link
Member

rlebeau commented Jun 25, 2020

Just an FYI, when TIdFTP is preparing a new data connection, it Clone()'s the SSLIOHandler assigned to the command channel. When TIdSSLIOHandlerSocketOpenSSL is cloned, it does reuse the same session ID as the original SSLIOHandler, but currently only when connecting an outbound connection (ie, for an FTP passive-mode transfer), not when accepting an inbound connection (ie, for an FTP active-mode transfer). So make sure that your new 1.1.x SSLIOHandler handles session ID reuse when cloned, if it doesn't already.

@mezen
Copy link
Contributor Author

mezen commented Jun 26, 2020

My last commit ensures that with the Clone() call, the session is always passed to the clone, so the later connection direction is irrelevant.

@JedrzejczykRobert
Copy link

JedrzejczykRobert commented Aug 4, 2020

Modified source for OpenSSL 1.1.1

OpenSSL.zip

Compile and tested with:

Delphi 7 + Win XP sp2
Delphi 2007 + Win XP sp3
Delphi XE 7 + Win 7
Delphi XE 10.4 + Win 10, Linux64, MacOS-10.15
Lazarus 2.0.10 + Win10, Win 7, WinXP, Ubuntu-18, MacOS-10.15

@xjikka
Copy link

xjikka commented Aug 17, 2020

Very impressive work. It is working by me on XE6, Delphi 10.2, 10.3, 10.4. To avoid update Indy and avoid changes in Indy IdGlobal.pas and Idctypes.pas, I've added new IdOpenSSL_IdCTypesEx.pas with missing consts and types. To make it working in Linux (tested on Raspbian) in Lazarus, I had to edit / replace SafeLoadLibrary with HackLoad in IdOpenSSLLoader.pas

  {$IFDEF MSWINDOWS}
  LLibCrypto := SafeLoadLibrary(FOpenSSLPath + CLibCrypto);
  LLibSSL := SafeLoadLibrary(FOpenSSLPath + CLibSSL);
  {$ELSE}
  LLibCrypto := HMODULE(HackLoad(FOpenSSLPath + CLibCryptoLinuxRaw,SSLDLLVers));
  LLibSSL := HMODULE(HackLoad(FOpenSSLPath + CLibSSLLinuxRaw,SSLDLLVers));
  {$ENDIF} 

and add constants to IdOpenSSLConsts.pas

CLibCryptoLinuxRaw = 'libcrypto';
CLibSSLLinuxRaw = 'libssl';
SSLDLLVers : array [0..1] of string = ('','.1.1');

@rlebeau
Copy link
Member

rlebeau commented Aug 17, 2020

To avoid update Indy and avoid changes in Indy IdGlobal.pas and Idctypes.pas, I've added new IdOpenSSL_IdCTypesEx.pas with missing consts and types.

Why would you want to avoid that, if that is where they belong? What exactly did you add that needed to be put into its own unit?

@xjikka
Copy link

xjikka commented Aug 17, 2020

To avoid update Indy and avoid changes in Indy IdGlobal.pas and Idctypes.pas, I've added new IdOpenSSL_IdCTypesEx.pas with missing consts and types.

Why would you want to avoid that, if that is where they belong? What exactly did you add that needed to be put into its own unit?

Normally I would do it as You do, but I can't upgrade / change Indy source in all versions of Delphi I have. Because of that, I made a separate IdOpenSSL_IdCTypesEx.pas in which the missing types and procedures are added, and therefore I don't need to change and recompile Embardadero\Studio\xx.0\source\Indy10. I have also added IdOpenSSL_IdCTypesEx.pas to "uses" in units, where needed.
Finally, to implement support of new TLS13 into existing instalation of XE6, 10.2, 10.3 and Lazarus I must only add path to patched OpenSSL source directory in delphi-options-language-delphi-librarypath and nothing more.

IdOpenSSL_IdCTypesEx.zip Updated 18.8.2020

@SlMaker
Copy link

SlMaker commented Aug 17, 2020

Any ETA for the official merge to master?

@xjikka
Copy link

xjikka commented Aug 17, 2020

Any ETA for the official merge to master?

I'm sorry, I have no experience with Git at all, but if You're interested, I can provide You changed source (zipped).

@grahamegrieve
Copy link

grahamegrieve commented Oct 24, 2020

I needed to make some changes to the code for FPC (v3.3.1):

  • %line% creates a string constant, so %LINE% -> %LINENUM% IdOpenSSLHeaders_crypto
  • parameters can't be called "out" - IdOpenSSLHeaders_cmac and IdOpenSSLHeaders_conf
  • System.Types --> Types in IdOpenSSLHeaders_x509v3 and IdOpenSSLHeaders_crypto

I should update this PR?

@grahamegrieve
Copy link

grahamegrieve commented Oct 24, 2020

And on Linux:

  • IdOpenSSLHeaders_ssl case of x509 is wrong
  • IndyCheckWindowsVersion in IdOpenSSLIOHandlerClientBase needs to be $IFDEF MSWINDOWS

Also, there's a number of getProcAddress that fail on the standard lib

@grahamegrieve
Copy link

Failed to load list below. These are pretty long lists that will generate lots of confusion. Should we $IFDEF these out?

Windows + Linux (ubuntu 20):
BIO_s_datagram_sctp
BIO_new_dgram_sctp
BIO_dgram_is_sctp
BIO_dgram_sctp_wait_for_dry
BIO_dgram_sctp_msg_waiting
BIO_f_zlib
_CONF_new_section
_CONF_get_section
_CONF_add_string
_CONF_get_string
_CONF_get_number
_CONF_new_data
_CONF_free_data
CRYPTO_mem_debug_push
CRYPTO_mem_debug_pop
CRYPTO_get_alloc_counts
CRYPTO_mem_debug_malloc
CRYPTO_mem_debug_realloc
CRYPTO_mem_debug_free
CRYPTO_mem_leaks_cb
CRYPTO_mem_leaks
ebcdic2ascii
ascii2ebcdic
BIO_set_md
EVP_rc5_32_12_16_cbc
EVP_rc5_32_12_16_ecb
EVP_rc5_32_12_16_cfb64
EVP_rc5_32_12_16_ofb
RAND_query_egd_bytes
RAND_egd
RAND_egd_bytes
SSL_CTX_set_tlsext_use_srtp
SSL_set_tlsext_use_srtp
SSL_get_selected_srtp_profile
SSL_trace

Windows only:
EC_GFp_nistp224_method
EC_GFp_nistp256_method
EC_GFp_nistp521_method

linux only:
EVP_idea_ecb
EVP_idea_cfb64
EVP_idea_ofb
EVP_idea_cbc
IDEA_options
IDEA_ecb_encrypt
IDEA_set_encrypt_key
IDEA_set_decrypt_key
IDEA_cbc_encrypt
IDEA_cfb64_encrypt
IDEA_ofb64_encrypt
IDEA_encrypt

@grahamegrieve
Copy link

@mezen This doesn't appear to work as a server for TIdHTTPServer - any connection generates

'error:140940F4:SSL routines:ssl3_read_bytes:unexpected message'.

The server set up code is

FServer := TIdHTTPServer.Create(Nil);
FServer.Scheduler := TIdSchedulerOfThreadPool.Create(nil);
FServer.DefaultPort := FActualSSLPort;
FServer.KeepAlive := SECURE_KEEP_ALIVE;
FIOHandler := TIdOpenSSLIOHandlerServer.Create(Nil);
FServer.IOHandler := FIOHandler;

FIOHandler.Options.CertFile := FCertFile;
FIOHandler.Options.CertKey := FKeyFile;
FIOHandler.Options.VerifyCertificate := FRootCertFile;
FIOHandler.Options.OnGetPassword := SSLPassword;

It doesn't matter what options I set. I think that that SSL negotiation is working ok - if I set to verify the client certificate that part works ok

@SlMaker
Copy link

SlMaker commented Oct 24, 2020

Failed to load list below. These are pretty long lists that will generate lots of confusion. Should we $IFDEF these out?

Windows + Linux (ubuntu 20):
_CONF_new_section
_CONF_get_section
_CONF_add_string
_CONF_get_string
_CONF_get_number
_CONF_new_data
_CONF_free_data
CRYPTO_mem_debug_push
CRYPTO_mem_debug_pop
CRYPTO_get_alloc_counts
CRYPTO_mem_debug_malloc
CRYPTO_mem_debug_realloc
CRYPTO_mem_debug_free
CRYPTO_mem_leaks_cb
CRYPTO_mem_leaks

  1. _CONF could be removed, sounds like it's nothing you can ever use in code
  2. The debug ones are probably only available if you've compiled OpenSSL with debug option.

Windows only:
EC_GFp_nistp224_method
EC_GFp_nistp256_method
EC_GFp_nistp521_method

OpenSSL documentation: "Note, however, that these implementations are not available on all platforms."

Think it's not a major problem as all of them are not needed by Indy code thus the programmer should check if it was loaded before using it in own code.

@grahamegrieve
Copy link

Agree that it's not a major problem but the problem is that routine practice in the old code is to check FailedToLoad and raise an exception if it's not empty. That fact that this list is not empty normally will generate a stream of questions in all the forums

@grahamegrieve
Copy link

grahamegrieve commented Oct 24, 2020

@mezen dsa_st and rsa_st are defined in both IdOpenSSLHeaders_ossl_typ and IdOpenSSLHeaders_evp and the definitions of EVP_PKEY_get0_RSA etc appear wrong - should be prsa not prsa_st etc

@grahamegrieve
Copy link

It appears that the definitions of EVP_PKEY_set1_RSA and EVP_PKEY_set1_DSA are wrong:

from openSSL doco:

int EVP_PKEY_set1_RSA(EVP_PKEY *pkey, RSA *key);
int EVP_PKEY_set1_DSA(EVP_PKEY *pkey, DSA *key);

proposed pascal code:

EVP_PKEY_set1_RSA: function(pkey: PEVP_PKEY): TIdC_INT cdecl = nil;
EVP_PKEY_set1_DSA: function(pkey: PEVP_PKEY): TIdC_INT cdecl = nil;

@grahamegrieve
Copy link

grahamegrieve commented Oct 25, 2020

Also this code should be in the loader (see attachment)
indy-fips.txt

(uses IdGlobal, IdCTypes, IdFIPS, IdOpenSSLHeaders_ossl_typ, IdOpenSSLHeaders_rsa, IdOpenSSLHeaders_dsa, IdOpenSSLHeaders_bn, IdOpenSSLHeaders_bio, IdOpenSSLHeaders_hmac, IdOpenSSLHeaders_pem, IdOpenSSLHeaders_err, IdOpenSSLHeaders_x509, IdOpenSSLHeaders_evp, IdOpenSSLHeaders_crypto, IdOpenSSLVersion)

@grahamegrieve
Copy link

And the types are wrong here:

EVP_DigestSignInit: function(ctx: PEVP_MD_CTX; pctx: PPEVP_PKEY_CTX; const &type: PEVP_MD; e: PENGINE; pkey: PEVP_PKEY): TIdC_INT cdecl = nil;
EVP_DigestSignFinal: function(ctx: PEVP_MD_CTX; sigret: PByte; siglen: PIdC_SIZET): TIdC_INT cdecl = nil;

@mezen
Copy link
Contributor Author

mezen commented Oct 28, 2020

Ok, now some feedback came in, so one after the other:

@JedrzejczykRobert I have implemented most of your changes

@xjikka also adopted

@grahamegrieve here it gets a bit longer

  • "%LINE% -> %LINENUM%" adopted
  • parameters can't be called "out" -> already noted and adopted by @JedrzejczykRobert with his changes. In fact you can already call it out if you prefix it with &. What I didn't know, that it didn't work in older Delphi versions
  • "System.Types -> Types" also already included in @JedrzejczykRobert
  • "IdOpenSSLHeaders_ssl case of x509 is wrong" you can enter a line number, I can't find what you mean
  • "IndyCheckWindowsVersion in IdOpenSSLIOHandlerClientBase" adopted
  • Not successful loaded functions: As my previous posters have already written, it depends a lot on the conditions your binary was built with - unless @rlebeau explicitly wants it to be different, I would rather leave it that way, since it is not a bug and works. The list FailedToLoad is rather interesting if you want to use OpenSSL outside of the IO handler
  • "dsa_st and rsa_st are defined in both IdOpenSSLHeaders_ossl_typ and IdOpenSSLHeaders_evp" yep, corrected
  • "EVP_PKEY_set1_RSA wrong" not after the correct type is used. In the set you cannot simply omit what is set in the 'EVP_PKEY'!
  • "EVP_DigestSignInit & EVP_DigestSignFinal are wrong" I have looked at https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/include/openssl/evp.h#L634 again and the definition looks so correct, can you describe the error in more detail?

@grahamegrieve
Copy link

grahamegrieve commented Oct 28, 2020

  • "IdOpenSSLHeaders_ssl case of x509 is wrong" line 59 - text is "IdOpenSSLHeaders_X509", must be "IdOpenSSLHeaders_x509" (matters for FPC on OSX)
  • Not successfully loaded: we should at a minimum document that this is not expected to be empty. It would be useful to add a list of functions that are nil for the standard distributions, maybe. That might depend on how many problems it causes
  • EVP_PKEY_set1_RSA wrong: the doco for openSSL:

int EVP_PKEY_set1_RSA(EVP_PKEY *pkey, RSA *key);

your header:

EVP_PKEY_set1_RSA: function(pkey: PEVP_PKEY): TIdC_INT cdecl = nil;

where is the RSA Key? You can't use this function without this parameter, and I don't understand your comment. This is the correct header that actually makes signing possible:

EVP_PKEY_set1_RSA: function(pkey: PEVP_PKEY; key : PRSA): TIdC_INT cdecl = nil;

  • EVP_DigestSignInit & EVP_DigestSignFinal are wrong

the openSSL doco:

int EVP_DigestSignInit(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
                        const EVP_MD *type, ENGINE *e, EVP_PKEY *pkey);
 int EVP_DigestSignFinal(EVP_MD_CTX *ctx, unsigned char *sig, size_t *siglen);

your header:

  EVP_DigestSignInit: function(ctx: PEVP_CIPHER_CTX; pctx: PPEVP_PKEY_CTX; const type_: PEVP_MD; e: PENGINE; pkey: PEVP_PKEY): TIdC_INT cdecl = nil;
  EVP_DigestSignFinal: function(ctx: PEVP_CIPHER_CTX; sigret: PByte; siglen: PIdC_SIZET): TIdC_INT cdecl = nil;

Correct header:

  EVP_DigestSignInit: function(ctx: PEVP_MD_CTX; pctx: PPEVP_PKEY_CTX; const &type: PEVP_MD; e: PENGINE; pkey: PEVP_PKEY): TIdC_INT cdecl = nil;
  EVP_DigestSignFinal: function(ctx: PEVP_MD_CTX; sigret: PByte; siglen: PIdC_SIZET): TIdC_INT cdecl = nil;

It's the type of the first parameter

@grahamegrieve
Copy link

And @mezen you did not add the fips loading routines?

@rlebeau
Copy link
Member

rlebeau commented Oct 29, 2020

And @mezen you did not add the fips loading routines?

@grahamegrieve AFAIK, FIPS support is not available in OpenSSL 1.1.x, that will come in OpenSSL 3.0.

https://www.openssl.org/docs/fips.html

https://wiki.openssl.org/index.php/FIPS_modules

https://keypair.us/2020/10/openssl-3-0-fips-timeline/

@grahamegrieve
Copy link

Well, at least some of it is working for me now. I use and test:

IsHMACAvail := OpenSSLIsHMACAvail;
IsHMACSHA256Avail := OpenSSLIsHMACSHA256Avail;
GetHMACSHA256HashInst:= OpenSSLGetHMACSHA256Inst;
UpdateHMACInst := OpenSSLUpdateHMACInst;
FinalHMACInst := OpenSSLFinalHMACInst;

@rapa4362
Copy link

With recent IdCustomTCPServer code is no longer disconnecting in TIdCustomTCPServer.DoTerminateContext(AContext: TIdContext), but instead closes socket with AContext.Binding.CloseSocket. This then causes SSL_shutdown in TIdOpenSSLSocket.Shutdown to return -1 that leads to EIdOpenSSLShutdownError. Original OpenSSL handler calls SSL_shutdown and gets -1 in the same way but doesn't check the result and no exception is raised. Does it really need to raise exception on server shutdown? As noted by RLebeau calling disconnect might cause AVs (and I was getting AV with AContext.Connection.Disconnect), but at least it was not raising exception on every shutdown.

@MWASoftware
Copy link

MWASoftware commented Jan 17, 2024

I've been testing the proposed patch out under Linux/Lazarus/fpc. It seems to work, with the following comments:

  1. In order to pick up the new units, I needed to add to the unit path, in addition to the usual Lib/Core, Lib/Protocols and Lib/System, the

Lib/Protocols/OpenSSL

Lib/Protocols/OpenSSL/dynamic

directories, and adding to the include path

/Lib/FCL

These are missing from indylaz.lpk hence have to be added manually to the project.

  1. Used the IdOpenSSLIOHandlerClient unit instead of the IdSSL,IdSSLOpenSSL units (guessed this - no documentation that I could find).

  2. For an http client using the TIdOpenSSLIOHandlerClient class instead of the TIdSSLIOHandlerSocketOpenSSL class as the httpclient's SSLHandler (no apparent need for any options). Again, determined by code inspection rather than documentation.

  3. When compiling there appeared to be a compile time bug (stray ':' after an "out" - or out needs to be changed to out_ to avoid conflict with a reserved word) in IdOpenSSLHeaders_conf.

Otherwise, all works well and impressed by the work.

@rlebeau
Copy link
Member

rlebeau commented Jan 17, 2024

adding to the include path

/Lib/FCL

That is a legacy folder, Indy stopped using that folder many years ago. You don't need to refer to it.

When compiling there appeared to be a compile time bug (stray ':' after an "out" - or out needs to be changed to out_ to avoid conflict with a reserved word) in IdOpenSSLHeaders_conf.

Can you report that to https://github.com/mezen/Indy/issues, or maybe even fix it and push it to https://github.com/mezen/Indy/pulls ?

@MWASoftware
Copy link

adding to the include path
/Lib/FCL

That is a legacy folder, Indy stopped using that folder many years ago. You don't need to refer to it.

The problem I had here was to get a command line program to compile. That is, I was not using Lazarus, and anyway indylaz.lpk has not been updated to include the OpenSSL directories. I thus had to guess which unit and include directories to use.

All of the files in the OpenSSL directory include the directive {$i IdCompilerDefines.inc}. However, this include file is not within the OpenSSL directory hierarchy. A quick "find" found four occurrences:

/Lib/FCL/IdCompilerDefines.inc
./Lib/SuperCore/IdCompilerDefines.inc
./Lib/Core/IdCompilerDefines.inc
./Lib/System/IdCompilerDefines.inc

I guessed the first.

Indylaz.lpk includes the line

which means that ./Lib/Core/IdCompilerDefines.inc ends being used in all cases. Hence, you are saying that the include path should be
./Lib/Core

Is that correct?

@rlebeau
Copy link
Member

rlebeau commented Jan 18, 2024

All of the files in the OpenSSL directory include the directive {$i IdCompilerDefines.inc}. However, this include file is not within the OpenSSL directory hierarchy. A quick "find" found four occurrences:

Five, actually.

/Lib/FCL/IdCompilerDefines.inc ./Lib/SuperCore/IdCompilerDefines.inc ./Lib/Core/IdCompilerDefines.inc ./Lib/System/IdCompilerDefines.inc

And also ./Lib/Protocols/IdCompilerDefines.inc, which is the one the OpenSSL units should be using, as it is underneath the Protocols folder.

Indylaz.lpk includes the line

which means that ./Lib/Core/IdCompilerDefines.inc ends being used in all cases.

That is fine, as all 5 copies have the same content.

Hence, you are saying that the include path should be ./Lib/Core

Is that correct?

Actually, no. For FPC/Lazarus, generally one copies all of Indy's source files into a single folder before then compiling Indy.

Also, see https://wiki.freepascal.org/Indy_with_Lazarus

@MWASoftware
Copy link

When compiling there appeared to be a compile time bug (stray ':' after an "out" - or out needs to be changed to out_ to avoid conflict with a reserved word) in IdOpenSSLHeaders_conf.

Can you report that to https://github.com/mezen/Indy/issues, or maybe even fix it and push it to https://github.com/mezen/Indy/pulls ?

I have raised an issue. I also want suggest some fixes to make the patch more Lazarus/FPC friendly and have forked this repo to MWASofware/Indy. Branch NewOpenSSL_PR_FPC has been added with the proposed compile time fix. I hope to add some build files later.

@Laurensvanrun
Copy link

Any progress on this so far?

@MWASoftware
Copy link

Any progress on this so far?

I cannot speak for anyone else and have no idea whether @mezen has done any further work, but in absence of anyone else appearing to step forward I have been working on my own solution. It all looks very encouraging and I hope to release the first version this week, Only final readme's and packaging are needed before I press the button.

While @mezen's work was very useful, I had two criticisms of it, which is why I decided to go down my own path. The first is that the proposal for 1.1.1 did not use the existing Indy OpenSSL classes. The second is that Openssl 3.x was not yet supported.

I have taken @mezen's new openssl loader, and intermediate code OpenSSL header files, added a couple more header files and written a new code processor (a sed script) to process the intermediate code into usable unit files. With a few mods to IdSSLOpenSSL, I now have a working set of code supporting:

  • OpenSSL 1.0.2, 1.1.1 and 3.x which dynamically adjusts to the version it finds. it probably works with earlier versions - but not tested.

  • The IdSSLOpenSSL components are unchanged except for the addition of a couple of useful properties e.g. to report the TLS protocol in use (ideally always TLS 1.3). There is also a new feature to automatically use the Windoes crypto API so that the Windows Root Certificate store can be used for validation on Windows hosts (the default - but can be disabled).

  • Three link models are supported.

    • Dynamic Library Load (the default and the approach used in previous versions)
    • compile time linkage to a shared (.so or .dll) library (OpenSSL 3.x only)
    • compile time linkage to a static library (FPC only with gcc compiled OpenSSL).

I have tested on both Windows and Linux with both Lazarus/FPC (3.2.2) and Delphi (Berlin 32 bit only). There are two test programs used for validation:

  1. Https Get from a public website with certification validation
  2. Https Get from an embedded TIdHttpServer using a private PKI and password protected keys.

Hopefully the result will be found useful.

@CodehunterWorks
Copy link

I follow this discussion since a very long time. Thanks to all who had worked on OSSL3 support! But it is all senseless if it will not be merged into the main branch. Most users wil not use an "hacked" version in any productive environment. Or with the words from our chief developer: Only the GetIt version is on the build server.

@Laurensvanrun
Copy link

I cannot speak for anyone else and have no idea whether @mezen has done any further work, but in absence of anyone else appearing to step forward I have been working on my own solution. It all looks very encouraging and I hope to release the first version this week, Only final readme's and packaging are needed before I press the button.

That is really great! Are you planning to merge it with the master Indy repository?

@MWASoftware
Copy link

That is really great! Are you planning to merge it with the master Indy repository?
Yes. That's the only reason I am not publishing today. The work is otherwise finished. I need to prepare a pull request - and of course, that needs to be accepted.

My main development environment is Lazarus/fpc under Linux and I have worked on an extract of the Indy source tree better presented for Lazarus. I now need to merge the files back into the main source tree and test. I have also split the Lazarus package into separate design and runtime packages to better support console mode applications. However, apart from testing the source code with Delphi (both test apps seem to be work OK) I have not and probably will not do any work on updating the Delphi packages. Someone else, with a Delphi licence will need to do this.

@Delphi-FPC-Lazarus
Copy link

Link to almost latest statement of @mezen
#299 (comment)

I cannot speak for anyone else and have no idea whether @mezen has done any further work, but in absence of anyone else appearing to step forward I have been working on my own solution. It all looks very encouraging and I hope to release the first version this week, Only final readme's and packaging are needed before I press the button.

many people has his own solutions, some "hack" solutions too... it's not the best way but we have no choice cause openssl eol

That is really great! Are you planning to merge it with the master Indy repository?

we're all (i guess anyone here) is waiting for it...

@rlebeau
Copy link
Member

rlebeau commented Mar 18, 2024

While @mezen's work was very useful, I had two criticisms of it, which is why I decided to go down my own path. The first is that the proposal for 1.1.1 did not use the existing Indy OpenSSL classes. The second is that Openssl 3.x was not yet supported.

The main problem with using the existing Indy OpenSSL components is that they are tied to OpenSSL 1.0.2. There were major API changes/breakages in OpenSSL 1.1, making it hard to use the existing code for future versions. If you found a way around that, then good on you! I would personally prefer that the existing components be updated to handle newer OpenSSL versions without requiring users to re-write their existing code. Not to put down @mezen's work, because he did a lot of great work, but its such a massive PR that I just never got to review it all, which is why it never got merged.

I have taken @mezen's new openssl loader, and intermediate code OpenSSL header files, added a couple more header files and written a new code processor (a sed script) to process the intermediate code into usable unit files.

Would that require me/Indy users to maintain/use that script whenever Indy is recompiled? Or when OpenSSL is updated? Indy's already difficult enough to update/reinstall, I'm just wondering what extra work this will add?

With a few mods to IdSSLOpenSSL, I now have a working set of code supporting:

That sounds cool. My only real concern to to make sure IdFIPS.pas and IdSSLOpenSSLHeaders_static.pas are handled properly. IdFIPS is used to connect OpenSSL to various pieces of Indy, and IdSSLOpenSSLHeaders_static is used for Indy on iOS. Also, issue #376 needs a solution, as well.

There is also a new feature to automatically use the Windoes crypto API so that the Windows Root Certificate store can be used for validation on Windows hosts (the default - but can be disabled).

That I'm a little concerned about. I know using CryptoAPI/SChannel is a hot item for Windows users (issue #49), but CryptoAPI is a completely separate API from OpenSSL that it really doesn't belong in the OpenSSL components, it should be kept separate. Unless you are referring to OpenSSL's own use of CryptoAPI on Windows?

I have tested on both Windows and Linux with both Lazarus/FPC (3.2.2) and Delphi (Berlin 32 bit only).

Keep in mind that Indy also supports Android and iOS in Delphi, as well as quite a few other platforms in FPC. So any new OpenSSL work will have to be usable in a wide array of platforms that OpenSSL supports.

@CodehunterWorks
Copy link

That I'm a little concerned about. I know using CryptoAPI/SChannel is a hot item for Windows users (issue #49), but CryptoAPI is a completely separate API from OpenSSL that it really doesn't belong in the OpenSSL components, it should be kept separate. Unless you are referring to OpenSSL's own use of CryptoAPI on Windows?
...
Keep in mind that Indy also supports Android and iOS in Delphi, as well as quite a few other platforms in FPC. So any new OpenSSL work will have to be usable in a wide array of platforms that OpenSSL supports.

Thats why we use Indy instead of Wininet components. We wont bound us to this proprietary piece and we do not trust it.

  1. MS has proven time and again that they are not able to deliver stable updates. There was already two cases when MS had patched broken the certificate store and next day our support wires had been glowing. This cant happen with OpenSSL because we have the control when we update the DLLs.
  2. Our code should be platform-independent as most as possible.
  3. I like Indys flexibility of Interceptor architecture, for example to write down NSS files for debugging purposes to use with Wireshark. I dont know any way to do this with Wininet.

So please make sure that OpenSSL and Wininet use is never mingled in Indy. We want to have the control to decide which backend is used in our products.

@mezen
Copy link
Contributor Author

mezen commented Mar 19, 2024

Any progress on this so far?

Currently I am not working on this PR anymore.

End of the development on this PR

I stop any development on this PR. The source will still be available. But this PR will be closed without a merge. Currently I am on developing a new IO Handler using OpenSSL 3. In fact, this change is a backport. I hope the new IO Handler will be finished before EOL of OpenSSL 1.1.1. That IO Handler will be no revolution, only a evolution of this one, so a lot of code will be very familiar.

and for the reboot of a new PR

Still work in progress... currently a little bit on hold due other topics. Maybe waiting for OpenSSL 3.2 which is currently in beta

Unfortunately, OpenSSL 1.1.1 has since gone EOL without me being able to complete my work beforehand. Time, the most precious commodity and always in short supply.

My biggest obstacle is actually the header translations.
The translations in this PR only contain a few selected units, but these are almost complete. I had selected these units beforehand. Then I had the help of a few other people, so it "only" took us 3 to 4 months. However, this also included a few reviews and checks to ensure that the translations were as correct as possible.

The original translations using OpenSSL 1.0.2 (and before) were very selective and only what was necessary was translated.
My translations for OpenSSL 1.1.1 were much more extensive, entire units were translated and not just individual functions that were needed. In fact, I also use OpenSSL in other places outside the IO handler (e.g. complete certificate management) and I wanted to make these functions available to others.

My latest approach is no longer a manual translation, but a technical one. However, all popular automatic translation tools failed due to the complexity of the OpenSSL headers, so I started to write my own tool. Knowing that it will never be able to translate everything, but if it could do 95% of the work for me, it would be a great help.

And indeed, this is my current status: the tool is not yet finished.
However, to come back to the time problem, I don't have the time to continue working on it at the moment, so the project is paused.

@rlebeau

There is also a new feature to automatically use the Windoes crypto API so that the Windows Root Certificate store can be used for validation on Windows hosts (the default - but can be disabled).

That I'm a little concerned about. I know using CryptoAPI/SChannel is a hot item for Windows users (issue #49), but CryptoAPI is a completely separate API from OpenSSL that it really doesn't belong in the OpenSSL components, it should be kept separate. Unless you are referring to OpenSSL's own use of CryptoAPI on Windows?

OpenSSL 3.2 has builtin support for the Windows system certificate store, so no big import of dependancies are needed.

Support for using the Windows system certificate store as a source of trusted root certificates This is not yet enabled by default and must be activated using an environment variable. This is likely to become enabled by default in a future feature release.

(source Openssl 3.2 release notes)

@MWASoftware
If you improved the OpenSSL Loader, great work :)

To save time, I have even put the dynamic loading of functions on the side in the current approach.
I am currently more of a fan of delayed loading, which combines the simple writing of static loading and only loading the functions that are needed when they are needed of dynamic loading.

@AdrianSRU
Copy link

Any progress on this so far?

Currently I am not working on this PR anymore.

End of the development on this PR

I stop any development on this PR. The source will still be available. But this PR will be closed without a merge. Currently I am on developing a new IO Handler using OpenSSL 3. In fact, this change is a backport. I hope the new IO Handler will be finished before EOL of OpenSSL 1.1.1. That IO Handler will be no revolution, only a evolution of this one, so a lot of code will be very familiar.

and for the reboot of a new PR

Still work in progress... currently a little bit on hold due other topics. Maybe waiting for OpenSSL 3.2 which is currently in beta

Unfortunately, OpenSSL 1.1.1 has since gone EOL without me being able to complete my work beforehand. Time, the most precious commodity and always in short supply.

My biggest obstacle is actually the header translations. The translations in this PR only contain a few selected units, but these are almost complete. I had selected these units beforehand. Then I had the help of a few other people, so it "only" took us 3 to 4 months. However, this also included a few reviews and checks to ensure that the translations were as correct as possible.

The original translations using OpenSSL 1.0.2 (and before) were very selective and only what was necessary was translated. My translations for OpenSSL 1.1.1 were much more extensive, entire units were translated and not just individual functions that were needed. In fact, I also use OpenSSL in other places outside the IO handler (e.g. complete certificate management) and I wanted to make these functions available to others.

My latest approach is no longer a manual translation, but a technical one. However, all popular automatic translation tools failed due to the complexity of the OpenSSL headers, so I started to write my own tool. Knowing that it will never be able to translate everything, but if it could do 95% of the work for me, it would be a great help.

And indeed, this is my current status: the tool is not yet finished. However, to come back to the time problem, I don't have the time to continue working on it at the moment, so the project is paused.

@rlebeau

There is also a new feature to automatically use the Windoes crypto API so that the Windows Root Certificate store can be used for validation on Windows hosts (the default - but can be disabled).

That I'm a little concerned about. I know using CryptoAPI/SChannel is a hot item for Windows users (issue #49), but CryptoAPI is a completely separate API from OpenSSL that it really doesn't belong in the OpenSSL components, it should be kept separate. Unless you are referring to OpenSSL's own use of CryptoAPI on Windows?

OpenSSL 3.2 has builtin support for the Windows system certificate store, so no big import of dependancies are needed.

Support for using the Windows system certificate store as a source of trusted root certificates This is not yet enabled by default and must be activated using an environment variable. This is likely to become enabled by default in a future feature release.

(source Openssl 3.2 release notes)

@MWASoftware If you improved the OpenSSL Loader, great work :)

To save time, I have even put the dynamic loading of functions on the side in the current approach. I am currently more of a fan of delayed loading, which combines the simple writing of static loading and only loading the functions that are needed when they are needed of dynamic loading.

@mezen I'd like to volunteer my assistance in this effort, if you are interested and it would be helpful. I have no particular experience with OpenSSL at this low level but it sounds like what is needed is work on the header translations. If I can help I would like to do so, please let me know.

@MWASoftware
Copy link

I have now published the proposed update to https://github.com/MWASoftware/Indy.proposedUpdate
Although it should be in the form of a pull request, I have not yet done this and would like comments on the work before doing this. My guess is that there will be issues with platforms that I have not tested on. Please read the README.proposed.update in the root directory.

@MWASoftware
Copy link

Would that require me/Indy users to maintain/use that script whenever Indy is recompiled? Or when OpenSSL is updated? Indy's already difficult enough to update/reinstall, I'm just wondering what extra work this will add?

The only time the script needs to be run is when the OpenSSL header files are updated. Otherwise, no one needs to touch the script.

@MWASoftware
Copy link

That sounds cool. My only real concern to to make sure IdFIPS.pas and IdSSLOpenSSLHeaders_static.pas are handled properly. IdFIPS is used to connect OpenSSL to various pieces of Indy, and IdSSLOpenSSLHeaders_static is used for Indy on iOS. Also, issue #376 needs a solution, as well.

I don't see any OpenSSL dependencies in IdFIPS.pas. But then again, I am not a .dot net user.

IdSSLOpenSSLHeaders_static.pas has been superseded by embedding the static headers as a conditional compilation part of each header file. Let me know if there is a problem that I had not spotted.

@rlebeau
Copy link
Member

rlebeau commented Mar 20, 2024

To save time, I have even put the dynamic loading of functions on the side in the current approach. I am currently more of a fan of delayed loading, which combines the simple writing of static loading and only loading the functions that are needed when they are needed of dynamic loading.

There is an open ticket (#9) for using delayed in Indy, but it would have to be IFDEF'ed for FPC and pre-D2010 versions.

I don't see any OpenSSL dependencies in IdFIPS.pas. But then again, I am not a .dot net user.

This has nothing to do with .NET. IdFIPS.pas contains a bunch of function pointers, mainly for creating hashes at runtime. Various areas of Indy use these pointers to call hashing functions. IdSSLOpenSSLHeaders.pas sets those pointers at runtime to implementations that use OpenSSL's hashing functions. This way Indy itself doesn't have to implement the hashes, it can rely on external implmentations.

@MWASoftware
Copy link

I don't see any OpenSSL dependencies in IdFIPS.pas. But then again, I am not a .dot net user.

This has nothing to do with .NET. IdFIPS.pas contains a bunch of function pointers, mainly for creating hashes at runtime. Various areas of Indy use these pointers to call hashing functions. IdSSLOpenSSLHeaders.pas sets those pointers at runtime to implementations that use OpenSSL's hashing functions. This way Indy itself doesn't have to implement the hashes, it can rely on external implmentations.

Looks like I missed that one. Will investigate.

@MWASoftware
Copy link

I don't see any OpenSSL dependencies in IdFIPS.pas. But then again, I am not a .dot net user.

This has nothing to do with .NET. IdFIPS.pas contains a bunch of function pointers, mainly for creating hashes at runtime. Various areas of Indy use these pointers to call hashing functions. IdSSLOpenSSLHeaders.pas sets those pointers at runtime to implementations that use OpenSSL's hashing functions. This way Indy itself doesn't have to implement the hashes, it can rely on external implmentations.

Looks like I missed that one. Will investigate.

I can see what needs doing. Easy enough to add back in the initialisation of the functions in IdFIPS. Will update my repo.

@MWASoftware
Copy link

OpenSSL version 3.3 alpha 1 was released today.

I have downloaded a copy and compiled it on 64-bit Linux and did a quick test of my test client and server. All seems OK. The programs gave the expected result. I am thus reasonably confident that my proposed update will support OpenSSL 3.3.

@MWASoftware
Copy link

This has nothing to do with .NET. IdFIPS.pas contains a bunch of function pointers, mainly for creating hashes at runtime. Various areas of Indy use these pointers to call hashing functions. IdSSLOpenSSLHeaders.pas sets those pointers at runtime to implementations that use OpenSSL's hashing functions. This way Indy itself doesn't have to implement the hashes, it can rely on external implmentations.

I have updated the proposed update to include FIPS support using OpenSSL. There is a new unit IdSSLOpenSSLFIPS contained mostly the original code from the OpenSSLHeaders unit, but modified to work with OpenSSL 3 as well as earlier versions. See the README.OpenSSL.FIPS for more information.

@MWASoftware
Copy link

I have not had any feedback since I made the update available. It would be useful to know if my proposed update is of any use or whether I should just keep it to myself. Comments both good and bad would be appreciated.

See https://github.com/MWASoftware/Indy.proposedUpdate

@MWASoftware
Copy link

I am currently more of a fan of delayed loading, which combines the simple writing of static loading and only loading the functions that are needed when they are needed of dynamic loading.

There is an open ticket (#9) for using delayed in Indy, but it would have to be IFDEF'ed for FPC and pre-D2010 versions.

I have added a simple solution to the proposed update for delayed loading - but hopefully sufficient. In this update, if Indy is compiled with the OPENSSL_USE_DELAYED_LOADING defined symbol then the static loading of a shared library link model is implied and each OpenSSL library external declaration includes the "delayed" directive. This should result in each such function only being loaded immediately before it is first used.

@rlebeau
Copy link
Member

rlebeau commented Mar 26, 2024

I have added a simple solution to the proposed update for delayed loading - but hopefully sufficient. In this update, if Indy is compiled with the OPENSSL_USE_DELAYED_LOADING defined symbol then the static loading of a shared library link model is implied and each OpenSSL library external declaration includes the "delayed" directive. This should result in each such function only being loaded immediately before it is first used.

I don't have a problem with delay-loading functions at runtime. There are several units in Indy which do that.

However, Indy 10 still supports Delphi versions all the way back to Delphi 5. And it supports FreePascal. delayed is not available at all in FPC, and is available in Delphi only in 2010+. And even when Indy drops support for pre-Unicode compilers in Indy 11, it will still have to support FPC and Delphi 2009, neither of which have delayed.

Also, there are several areas of Indy where it can report which imports actually failed to load at runtime, and doing that with delayed requires the global DliFailureHook2 callback, which would prevent user code from also being able to use that same callback for any error handling of their own delayed imports.

That's the main reasons why delayed has not been implemented anywhere in Indy yet. So, if you are going to handle delay loading and have any hope of seeing this merged in, it needs to be done via (Safe)LoadLibrary()/HackLoad()+LoadLibFunction() for now. There needs to be 1 unified mechanism used throughout the entire Indy library.

Although, I have been thinking about changing IdSSLOpenSSLHeaders.pas to use per-function stubs, similar to how IdWinSock2.pas and IdZLibHeaders.pas currently do, for example. But that is still based on LoadLibFunction().

@MWASoftware
Copy link

I have added a simple solution to the proposed update for delayed loading - but hopefully sufficient. In this update, if Indy is compiled with the OPENSSL_USE_DELAYED_LOADING defined symbol then the static loading of a shared library link model is implied and each OpenSSL library external declaration includes the "delayed" directive. This should result in each such function only being loaded immediately before it is first used.

I don't have a problem with delay-loading functions at runtime. There are several units in Indy which do that.

However, Indy 10 still supports Delphi versions all the way back to Delphi 5. And it supports FreePascal. delayed is not available at all in FPC, and is available in Delphi only in 2010+. And even when Indy drops support for pre-Unicode compilers in Indy 11, it will still have to support FPC and Delphi 2009, neither of which have delayed.

That's why I made "delayed" a conditional compilation and put in a trap so that it gives a meaningful error message if you try to compile with FPC. Probably should add a similar one for old versions of Delphi.

Also, there are several areas of Indy where it can report which imports actually failed to load at runtime, and doing that with delayed requires the global DliFailureHook2 callback, which would prevent user code from also being able to use that same callback for any error handling of their own delayed imports.

Looks like you need a framework for handling the callback.

That's the main reasons why delayed has not been implemented anywhere in Indy yet. So, if you are going to handle delay loading and have any hope of seeing this merged in, it needs to be done via (Safe)LoadLibrary()/HackLoad()+LoadLibFunction() for now. There needs to be 1 unified mechanism used throughout the entire Indy library.

You seem to be saying that Indy should implement its own version of delay loading - certainly more generic. Doable, especially when using a code generator for the header files but whether you can extend this throughout Indy is more problematic. The need for OpenSSL 3 is much more pressing that a minor optimisation of library load.

Although, I have been thinking about changing IdSSLOpenSSLHeaders.pas to use per-function stubs, similar to how IdWinSock2.pas and IdZLibHeaders.pas currently do, for example. But that is still based on LoadLibFunction().

That would be how you would do it with a code generator. The code generator creates a stub for each function which is called the first time user code calls a given function. The stub then loads the actual function address, replaces the function variable with the newly loaded function address and calls it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Element: I/O Handlers Issues related to TIdIOHandler and descendants Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Type: Enhancement Issue is proposing a new feature/enhancement
Projects
None yet