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

Perhaps SSLIOHandler descendants should define their own SSL version definitions #569

Open
JPeterMugaas opened this issue Dec 14, 2024 · 7 comments
Labels
Element: I/O Handlers Issues related to TIdIOHandler and descendants Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Status: Reported Issue has been reported for review Type: Enhancement Issue is proposing a new feature/enhancement

Comments

@JPeterMugaas
Copy link
Contributor

JPeterMugaas commented Dec 14, 2024

I noticed that the Indy-10.7 branch has this in IdSSL:

type
TIdSSLVersion = (sslvSSLv2, sslvSSLv23, sslvSSLv3, sslvTLSv1, sslvTLSv1_1, sslvTLSv1_2, sslvTLSv1_3);
TIdSSLVersions = set of TIdSSLVersion;

Perhaps it would be better if the TIdSSLIOHandlerSocketBase and TIdServerIOHandlerSSLBase class descendants define these in their own way. Taurus already defines the SSLVersions very differently than the OpenSSL IOHandlers:

TaurusTLS defines things like this:

type
TTaurusTLSSSLVersion = (Unknown, SSLv2, SSLv23, SSLv3, TLSv1, TLSv1_1,
TLSv1_2, TLSv1_3);
{ May need to update constants below if adding to this set }
TTaurusTLSSSLVersions = set of TTaurusTLSSSLVersion;
TTaurusTLSSSLMode = (sslmUnassigned, sslmClient, sslmServer, sslmBoth);
TTaurusTLSVerifyMode = (sslvrfPeer, sslvrfFailIfNoPeerCert, sslvrfClientOnce);
TTaurusTLSVerifyModeSet = set of TTaurusTLSVerifyMode;
TTaurusTLSCtxMode = (sslCtxClient, sslCtxServer);

const
DEF_SSLVERSION = TLSv1_2;
DEF_SSLVERSIONS = [TLSv1_2, TLSv1_3];
MAX_SSLVERSION = TLSv1_3;
P12_FILETYPE = 3;

This is incompatible with the changes you propose.

@JPeterMugaas JPeterMugaas added Type: Bug Issue is a bug in existing code Status: Reported Issue has been reported for review labels Dec 14, 2024
@JPeterMugaas
Copy link
Contributor Author

I should explain things further. TaurusTLS supports both this version of Indy and the shipping version in RAD Studio. The Values in TaurusTLS do not completely correspond with what you have. I have a value called "Unknown" which is a catchall in case something does not work.

@rlebeau
Copy link
Member

rlebeau commented Dec 14, 2024

Moving the TIdSSLVersion(s) types into IdSSL.pas is related to #281.

But, in any case, I don't see a need for there to be an Unknown value defined. What would happen if a user tried to assign an Unknown version to an SSLIOHandler? These are supposed to be values for the user to assign, not for the component to report errors with. And, why would there ever be an Unknown version in an established TLS session?

If you can remove Unknown, then your TTaurusTLSSSLVersion(s) types can just be aliases of the IdSSL types. IdSSLOpenSSL.pas will be doing exactly that once everything is finished.

Since you are also copying Indy's SSLMode, VerifyMode(Set), and CtxMode types, maybe they should be moved to IdSSL as well? Well, maybe not the VerifyMode at least, since that is very OpenSSL-oriented, but the others are more generic.

@rlebeau rlebeau changed the title Perhpas descendant classes should define their own SSL Version definitions. Perhpas SSLIOHandler descendants should define their own SSL version definitions Dec 14, 2024
@rlebeau rlebeau added Type: Enhancement Issue is proposing a new feature/enhancement Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Element: I/O Handlers Issues related to TIdIOHandler and descendants and removed Type: Bug Issue is a bug in existing code labels Dec 14, 2024
@rlebeau rlebeau changed the title Perhpas SSLIOHandler descendants should define their own SSL version definitions Perhaps SSLIOHandler descendants should define their own SSL version definitions Dec 14, 2024
@JPeterMugaas
Copy link
Contributor Author

JPeterMugaas commented Dec 14, 2024

I have removed the "unknown" value and an exception is now being raised for an invalid value. I too think VerifyMode should not be moved down but the other values probably could be moved.

The definitions in TaurusTLS are now:

{$I TaurusTLSIndyVers.inc}

{$IFDEF GETURIHOST_SUPPORTED}
const
SSLv2 = sslvSSLv2;
SSLv23 = sslvSSLv23;
SSLv3 = sslvSSLv3;
TLSv1 = sslvTLSv1;
TLSv1_1 = sslvTLSv1_1;
TLSv1_2 = sslvTLSv1_2;
TLSv1_3 = sslvTLSv1_3;
type
TTaurusTLSSSLVersion = IdSSL.TIdSSLVersion;
TTaurusTLSSSLVersions = IdSSL.TIdSSLVersions;
{$ELSE}
type
TTaurusTLSSSLVersion = (SSLv2, SSLv23, SSLv3, TLSv1, TLSv1_1,
TLSv1_2, TLSv1_3);
{ May need to update constants below if adding to this set }
TTaurusTLSSSLVersions = set of TTaurusTLSSSLVersion;
{$ENDIF}
TTaurusTLSSSLMode = (sslmUnassigned, sslmClient, sslmServer, sslmBoth);
TTaurusTLSVerifyMode = (sslvrfPeer, sslvrfFailIfNoPeerCert, sslvrfClientOnce);
TTaurusTLSVerifyModeSet = set of TTaurusTLSVerifyMode;
TTaurusTLSCtxMode = (sslCtxClient, sslCtxServer);

@rlebeau
Copy link
Member

rlebeau commented Dec 14, 2024

I too think VerifyMode should not be moved down but the other values probably could be moved.

They have now been moved to IdSSL.pas.

@JPeterMugaas
Copy link
Contributor Author

For some reason, they got moved back into IdSSLOpenSSL.pas and this is breaking my code.

@JPeterMugaas
Copy link
Contributor Author

PR #572 should fix the problem.

@rlebeau
Copy link
Member

rlebeau commented Dec 15, 2024

A bunch of other files got reverted as well. I must have missed a merge when I was bringing some other branches together. I have corrected the problem.

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 Status: Reported Issue has been reported for review Type: Enhancement Issue is proposing a new feature/enhancement
Projects
None yet
Development

No branches or pull requests

2 participants