mezen / Indy

Indy - Internet Direct
7 stars 6 forks source link

Does this zip need to be reviewed/merged in? #12

Open rlebeau opened 1 year ago

rlebeau commented 1 year ago

A few years ago, a zip file with updates was attached to a comment on the open PR in Indy's repo:

https://github.com/IndySockets/Indy/files/5029069/OpenSSL.zip

That code doesn't appear to have been merged into this repo, and so doesn't appear in the code for the open PR.

For example, IdOpenSSLConsts.pas has much more code in it than what is in the repo right now. A user posted at https://en.delphipraxis.net/topic/9118-delphi-113-indy-openssl-31/ making some changes to the zip code, not this repo code, so I just want to make sure this repo is up-to-date.

mezen commented 1 year ago

The repo is currently not up-to-date. And I will archive the repo soon and close the corresponding PR without merge.

There are several changes that have not yet merged back into the repo, for example that a bidirectional shutdown is performed when the connection is closed, meaning waiting for the TLS connection of the remote peer to be closed. The RFC allows skipping the wait only in the case that you know exactly on the protocol level that no further data will come. However, waiting for the remote peer is always conformal. And as a general IO handler I cannot make any assumptions about the protocol level. Especially "send-only" connections, where no reads are ever done on the TCP socket, are vulnerable to TCP RST as a result, which lead to data loss. I had received feedback regarding TLS 1.3 and FTP and the Data Channel experiencing data loss. The background was explained very well and exciting in https://github.com/openssl/openssl/issues/7948, FTP Data Channel (as an example for "Send-Only" connections) came in there later. Currently my change is in the in-house test and I want to backport it.

Otherwise I'm currently working on a complete (automated) translation of the C headers of OpenSSL 3.1. These are not finished yet, so nothing published yet.

Another customization is to support older Indy versions, so that the Indy version provided by emba is sufficient for the IO handler. These are, as far as I remember, also the changes from the zip file: They wanted to spare a complete Indy update. However, this will not be the recommended way, but should be sufficient for quick and easy testing so that others can evaluate my code.

rlebeau commented 1 year ago

If you close the PR and archive the repo, will the latest code be available somewhere else? I've been directing people to the PR for a long time. I know I'm lagging on review and merge (partly because its a big review, and partly because I don't have a working IDE, but I'm working on resolving that).

Would it make sense to make a new branch in Indy's repo (or revive the old OpenSSL-1.1.x branch) to hold the latest code, and make you a contributor for it? Or, how should we proceed with the latest code?

mezen commented 1 year ago

If you close the PR and archive the repo, will the latest code be available somewhere else?

No, but the archived repo is still available. Archiving a repo does not mean deleting the repo. Its more like a read-only flag, so everybody knows: there will be no more changes. Here is an example of a archived repo: https://github.com/google/google-authenticator

And a closed PR does not mean the PR (or the changes) will be deleted.

But if you are still uncertain you could hold a fork of my changes on your own. Thats the best part about open source :)

grahamegrieve commented 1 year ago

@rlebeau is there anything we can do to help you close this out? Do you need $$? This is turning into a major operational issue for the community

rlebeau commented 1 year ago

@grahamegrieve money won't really help me. What I lack is time and environment.

Time to review the code (of which there is a lot in this PR!) to make sure i's are dotted, t's are crossed, packages are updated, and everything flows nicely with the rest of Indy's code.

Environments to test everything with. I haven't had a working IDE installed for awhile now (but I'm working on correcting that). And I don't have access to any non-Windows systems (and, I wouldn't even know what to do with them, anyway).

If the community wants to chip in, they can focus on those efforts. At this point, if the community says the code is good, I'm inclined to just commit it and deal with any consequences later.

grahamegrieve commented 1 year ago

well, I and others have long had it in production, and I'm ok from that pov. I had it working with Delphi 10/11 and now have it working and tested on win64, linux and OSX using FPC 3.2+

So it seems to me that what's left is code consistency and quality. What I can't find is any remaining documentation of our coding standards. We used to have some back in the Indy9 days, but things have moved along, and I don't see anything current.

grahamegrieve commented 1 year ago

well, I and others have long had it in production, and I'm ok from that pov. I had it working with Delphi 10/11 and now have it working and tested on win64, linux and OSX using FPC 3.2+

So it seems to me that what's left is code consistency and quality. What I can't find is any remaining documentation of our coding standards. We used to have some back in the Indy9 days, but things have moved along, and I don't see anything current.

rlebeau commented 1 year ago

I do recall there being a coding standards doc at some point, but that was a LONG time ago. I don't think it exists anymore, certainly not for Indy 10 anyway.

If you think the new code is ready for prime-time, I'm good with that. At the very least, just have to make sure all of the packages are up-to-date for all of the various IDE versions, since Indy 10 still supports back to Delphi/C++Builder v5. And, are there palette icons available for the new components?

mezen commented 1 year ago

If you are talking about my new components: No, they are actually not included in the packages, and no special work is done for design time. My primary goal was working components, that works good in Indy. Adding to package/design time is easy possible to do after.

rlebeau commented 1 year ago

So, right now, they are just add-on units that people would have to reference directly, right? Going forward, if this is to be merged into master, I would think they would at least need to be added to the contains of the IndyProtocols runtime package, yes? Or maybe even a new runtime package, given the sheer number of units involved. Trying to think of the best way for people to incorporate this into their projects, especially once this is in master and Embarcadero starts shipping it in future Delphi releases.

We can tell people that design-time support is coming later. Maybe in the cleanup for Indy 11, since I'll be doing some reorganizing of the packages anyway in that release. At some point, maybe Indy 12, the Protocols package should probably get broken up into smaller packages (Web, Email, etc), so there could be a separate OpenSSL package.

Pea-ACS commented 1 year ago

@rlebeau

If I may ask; what is the monetary requirement to get your environment up and running to full spec so that you have everything that you need?

leus commented 1 year ago

any help we can provide? this is becoming a big issue for me.

rlebeau commented 1 year ago

If I may ask; what is the monetary requirement to get your environment up and running to full spec so that you have everything that you need?

@Pea-ACS The problem is time rather than money. I did recently get a new license for the latest IDE version, I just need to get it installed, and get Indy building with it. But between my day job, my kids, my wife's small business, etc, I just don't have the free time that I used to have. That's why its taken so long to get PR #299 reviewed and merged, as its a massive PR.