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

Remove dependancies on IdSSLOpenSSLHeaders and IdSSLOpenSSL units #376

Open
rlebeau opened this issue Sep 13, 2021 · 7 comments
Open

Remove dependancies on IdSSLOpenSSLHeaders and IdSSLOpenSSL units #376

rlebeau opened this issue Sep 13, 2021 · 7 comments
Labels
Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Status: Review Needed Issue needs further review to decide next status Type: Maintenance Issue is proposing maintenance of existing code

Comments

@rlebeau
Copy link
Member

rlebeau commented Sep 13, 2021

Several units depend directly on the IdSSLOpenSSLHeaders and IdSSLOpenSSL units, namely:

IdAuthenticationNTLM.pas
IdDsnRegister.pas
IdNTLM.pas
IdNTLMv2.pas
IdRegister.pas

This means that when new SSLIOHandlers are used, such as the one being worked on for #299, the old TIdSSLIOHandlerSocketOpenSSL is also being used/registered, which causes issues for TIdHTTP for instance (default SSLIOHandler creation, use of TIdAuthenticationNTLM, etc).

In IdCompilerDefines.inc there is a USE_OPENSSL conditional, but it is currently being ignored by some of the above units (the relevant IFDEFs are commented out).

So, this makes it very difficult to upgrade Indy to support OpenSSL 1.1.x/3.0.0 when 1.0.2 is still being depended on in places.

Need to expose new function pointers to abstract away all access to the older IdSSLOpenSSL... units. Maybe add them to the IdFIPS.pas unit, with the existing hash function pointers? Or maybe in another unit would make more sense, perhaps IdSSL.pas?

@rlebeau rlebeau added Type: Enhancement Issue is proposing a new feature/enhancement Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants labels Sep 13, 2021
@CodehunterWorks
Copy link

Hello Remy, since I had chased and found the SSL3_STATE.write_mac_secret field issue I know how much complex the TLS implementation is. Therefore my toughts may be sound a little bit easier said than done but I suggest to not integrate TLS 1.1.x and 3.0.0 into the old 1.0.x stuff. It might be better to encapsulate all of them into each individual "universes", selectable by an compiler switch like {$IFDEF USE_TLS_102}, {$IFDEF USE_TLS_111} and {$IFDEF USE_TLS_300}. With a view to 3.0.0 i mean: Sometimes a total demolition and new construction is easier and cleaner than an addition, in particular for the future evolvement of OpenSSL.

@rlebeau
Copy link
Member Author

rlebeau commented Sep 27, 2021

I never said the new OpenSSL 1.1.x/3.0 stuff was being incorporated into the old 1.0.x stuff. Indeed, it is being kept separate, the new 1.1.x/3.0 stuff is being implemented in a new set of units and SSLIOHandler classes. This ticket is meant for removing some hard-coded dependencies on the older units.

I'll consider the IFDEF approach, but I don't think it will work out, at least initially. AFAIK, the new SSLIOHandler classes are not a drop-in replacement for the old classes, so there will be a transition period where people will have to re-write existing code, so the 2 sets of code will have to co-exist for awhile. Eventually, the old classes will have to be deprecated and then removed. But all of this is moot at the moment since the newer units aren't even finalized and incorporated into the main code yet.

@CodehunterWorks
Copy link

Ah ok, then I'd misunderstand you :) Generally I prefer separate Units. And, BTW: I suggest to change the name for the new Handler from TIdSSLIOHandlerSocketOpenSSL to TIdTLSIOHandlerSocket. Simply because we do not have another external crypto but OpenSSL and the "SSL" part is a little bit outdated.

@rlebeau
Copy link
Member Author

rlebeau commented Sep 28, 2021

TIdSSLIOHandlerSocketOpenSSL is the old OpenSSL 1.0.2 handler. The new OpenSSL 1.1.x/3.0 handler is named TIdOpenSSLIOHandlerClient. Have you even looked at the new code yet?

@rlebeau rlebeau added Type: Maintenance Issue is proposing maintenance of existing code and removed Type: Enhancement Issue is proposing a new feature/enhancement labels Apr 23, 2023
@rlebeau rlebeau added this to the Indy 11 - Maintenance Release milestone Apr 23, 2023
@rlebeau rlebeau added the Status: Review Needed Issue needs further review to decide next status label Apr 26, 2023
@MWASoftware
Copy link

I noticed the problem with IdAuthenticationNTLM when I was preparing my proposed update (https://github.com/MWASoftware/Indy.proposedUpdate). To solve these dependency issues, I would propose:

  1. Moving IdSSL into the "core" package
  2. Creating a new OpenSSL package comprising all the IdSSLOpenSSL* units, IdNTLM, idNTMLv2 and IdAuthenticationNTLM.
  3. Modifying IdAllAuthentications to remove registration of NTLM.
  4. Registration of NTML to be performed when OpenSSL package is included project.

The downside is that existing code will need to be modified to use the new package, The upside is that the dependency tree is cleaner. Note: NTMLv2 uses RC4 and which is deprecated in OpenSSL 3.

@rlebeau
Copy link
Member Author

rlebeau commented Mar 26, 2024

1. Moving IdSSL into the "core" package

Why? Nothing in the Core package uses SSL/TLS, encryption, or hashing APIs. All of that is in the Protocols package.

2. Creating a new OpenSSL package

I had thought of that before, just hadn't implemented it yet. And it might make sense to have other SSLIOHandler's (ie SChannel, etc) be implemented in their own packages, too.

... comprising all the IdSSLOpenSSL* units, IdNTLM, idNTMLv2 and IdAuthenticationNTLM.

IdNTLM/v2 and IdAuthenticationNTLM would not need to be moved to a new package if their use of encryption/hashing APIs were being properly abstracted.

3. Modifying IdAllAuthentications to remove registration of NTLM.
4. Registration of NTML to be performed when OpenSSL package is included project.

Not necessary if IdAuthenticationNTLM is not moved out of the Protocols package to begin with.

@MWASoftware
Copy link

1. Moving IdSSL into the "core" package

Why? Nothing in the Core package uses SSL/TLS, encryption, or hashing APIs. All of that is in the Protocols package.

I proposed this because IdSSL seems to be the only unit in the protocols package that is used by the OpenSSL units. Moving it to "core" would mean that an OpenSSL package would be dependent on "core" and not "protocols" - unless I have missed something of course.

2. Creating a new OpenSSL package

I had thought of that before, just hadn't implemented it yet. And it might make sense to have other SSLIOHandler's (ie SChannel, etc) be implemented in their own packages, too.

... comprising all the IdSSLOpenSSL* units, IdNTLM, idNTMLv2 and IdAuthenticationNTLM.

IdNTLM/v2 and IdAuthenticationNTLM would not need to be moved to a new package if their use of encryption/hashing APIs were being properly abstracted.

Good idea. I would use pascal "interfaces" as the abstraction. BTW, checking through IdNTLMv2: the unit loads the RC2 functions but then I can't find any use of them.

3. Modifying IdAllAuthentications to remove registration of NTLM.
4. Registration of NTML to be performed when OpenSSL package is included project.

Not necessary if IdAuthenticationNTLM is not moved out of the Protocols package to begin with.

Providing an interface to DES in IdSSL would be the best way of leaving it in Protocols IMHO and better than my original proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Status: Review Needed Issue needs further review to decide next status Type: Maintenance Issue is proposing maintenance of existing code
Projects
None yet
Development

No branches or pull requests

3 participants