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

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

Open
rlebeau opened this issue May 23, 2023 · 13 comments
Open

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

rlebeau opened this issue May 23, 2023 · 13 comments

Comments

@rlebeau
Copy link

rlebeau commented May 23, 2023

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
Copy link
Owner

mezen commented May 23, 2023

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 openssl/openssl#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
Copy link
Author

rlebeau commented May 24, 2023

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
Copy link
Owner

mezen commented May 24, 2023

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
Copy link

@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
Copy link
Author

rlebeau commented Jun 2, 2023

@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
Copy link

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.

1 similar comment
@grahamegrieve
Copy link

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
Copy link
Author

rlebeau commented Jun 2, 2023

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
Copy link
Owner

mezen commented Jun 2, 2023

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
Copy link
Author

rlebeau commented Jun 2, 2023

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
Copy link

Pea-ACS commented Jun 8, 2023

@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
Copy link

leus commented Aug 22, 2023

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

@rlebeau
Copy link
Author

rlebeau commented Aug 22, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants