Angus Robertson

The latest version of the ICS FTP server includes full SSL/TLS support and will even automatically order and install free Let's Encrypt certificates, or commercial certificates if you really want to pay. 

The most important point about certificates is they are mostly for public domain names that can be accessed on the public internet, and the certificates are published in public logs for any to find and check.  Indeed hackers monitor those logs, and within a few seconds of a new certificate being issued expect hacking attempts to port 80.  BTW, port 80 needs to be open to order a domain validated certificate. and the ICS FTP server also has an HTTP server to satisfy the request, only runs for a few seconds until the order process is finished.

If you don't care about domain validation and only encryption, you can use self signed certificates which ICS will also generate.  The X509Certs component will even issue certificates signed by a private CA, I use that on my LAN for testing lots of internal servers.

Angus

So reference counting was the culprit, sorry missed that in the blizzard of overnight messages.  I don't use TBytes very often but will carefully check ICS for all such use.  As I said yesterday, the web server sample was erratic under Win64, but my development PC had not been rebooted since the last patch Tuesday and does become more unstable over time.  Should be able to update SVN later today.

Angus

Making it const Buffer: TBytes; is a safe change, var would be dangerous since it's a cast pointer. 

Still confused why it would be necessary, I always thought pointers did not need to be declared const, and the different behaviour when compiled with Win32 and Win64 suggests a compiler bug to me?  

I'll need to check a few other functions using TBytes.

Angus

Since I can not reproduce the problem with Win64, I'll do nothing more for now.  I still await the IcsLogger log lines I asked for.  

Angus

His solution may have fixed the exception, but will not return any sensible ALPN data, and will probably introduce other problems instead. 

I did wonder whether I should use PAnsiChar instead of TBytes, which would be safer in case someone in the future tried to resize the parameter.  But TBytes seems to be the preferred method for working with binary and is mobile compatible, where Ansi is not.

Angus

I quite understand that pointers differ in Win64, but they that code is not copying pointers, it is copying 8-bit bytes. 

Will you please build the OverbyteIcsSslWebServ sample with ICS 8.64 Win64 and your changes, make an request to https://localhost/demo.html with Display SSL Info and Logger Dest Event ticked, the log should contain lines with AlpnCB similar to:

[16:22:53 127.0.0.1] SNI "localhost" received
16:22:53:024 AlpnCB> inlen: 12 - 02683208687474702F312E31
16:22:53:031 AlpnCB> Protocols: h2,http/1.1
[16:22:53] SSL Application Layer Protocols allowed from client: h2,http/1.1
16:22:53:044 0000000002710020 ICB> SSL_accept: SSLv3/TLS read client hello
 

where those two lines are the input and output from IcsWireFmtToStrList.  I'm not going to look at this further with evidence of a real problem in ICS.  Or you could add IcsLogger to your own server to get the same logging information. 

Angus

15 hours ago, Marcelo Jaloto said:

The IcsWireFmtToStrList method of the OverbyteIcsUtils unit had a problem moving characters with Move without regard to the number of bytes.

Sorry, but you misunderstand the declaration for TBytes, it is a pointer to a dynamic array of bytes, not a pointer to a array of pointers.  A byte remains the same size however compiled, so your proposed suggestion of copying the content based on the pointer size will cause many problems. 

I can only assume your correction was theoretical and you did not test it with an ICS SSL server application that uses the code. 

I've just built the OverbyteIcsSslWebServ sample with Win64, and ALPN is working as expected, as I showed earlier in this thread,   I am getting some other strange exceptions running Win64 samples today, so there may be other Win64 issues elsewhere, or with OpenSSL Win64, but not with ALPN.   Don't have time to investigate further at the moment.

Angus

The ALPN changes are in V8.64 which is not released yet, but the changes can be downloaded from the SVN overnight zip on the page you mention. 

However I did not fix any bugs in the server version, just improved the code as previously mentioned. 

Angus

If you are using TSslWSocketThrdServer then you should be using TSslWSocketThrdClient.  However TSslWSocketThrdServer is itself a descendent of TSslWSocketServer which does not use threads, so it all depends on how you are using the component.  OnSessionClosed is only gone to triggered once since the client is destroyed immediately afterwards. 

Presumably you started with the OverbyteIcsThrdSrvV3.dpr sample which is the documentation and sample for TSslWSocketThrdServer, but does not support SSL. 

Angus

Further to my last comment, I'm not going to change the code at the moment.  If you specifically want to ignore the Location: header for an API request, set property FollowRelocation to false. 

This is what the TSslX509Certs component does making REST requests which return Location: headers for responses 201 and 409. 

Angus

Thanks, 201 Created is used a lot with REST, the Location code already has lots of special cases and I guess we need another.

Angus

I was wrong about TLS/1,3, it has been experimentally added to Windows 10 1909 but only for use in Microsoft Edge (old version) not Internet Explorer, enabled in Internet Options,  Advanced, Security.  Can not test it because Edge stopped working months ago and just puts errors in the Windows Logs instead.

Angus

On 12/27/2019 at 7:54 PM, stijnsanders said:

Older Windows versions don't support TLS 1.3,

No versions of Windows support TLS/1,3 yet using SChannel, not even Windows Server 2019.   Only applications using OpenSSL 1.1.1 or browsers with their own SSL libraries like Firefox and Chrome support TLS/1,3.

Even TLS/1,2 was not supported by Windows Vista and 7 until a patch a couple of years ago when Microsoft realised people did not want to use Windows 10.  And it's now increasingly common for protocols earlier than TLS/1,2 to be disabled on web servers for security reasons.  

Angus

Start by building and running the OverbyteIcsHttpRestTst.dpr demo in SslDemos, that directory includes the latest OpenSSL DLLs, assuming you are using the latest ICS download, which is V8.63 or V8.64.

You get a version exception using newer DLLs on older ICS versions, or vice versa. 

Angus

That sounds like an IDE or compiler error rather than an ICS error.  I've not really used D7 for 10 years or more, but ICS should still build on it, others have reported errors in D7 which I've fixed this year.

Angus

Should have said earlier the main event that tells you a client is connected is receiving data in the onDataAvailable in TSslWSocketClient, this is where any data from the remote client arrives.   You assign this even in the OnClientConnect event.  Very unusual for a server to initiate sending data to a client without receiving something first, even a blank line.

Angus

The OnClientConnect event I've mentioned twice is how you know the client is connected.  TSslWSocketClient is not created until after connection is successful, although SSL negotiation may still fail so the socket gets disconnected again. 

Servers only ever listen and respond to commands from remote clients, most clients will cleanly disconnect once completed, but you generally don't know if the client is still there which is why most servers have a timeout for an idle client. 

Angus

Again why does it matter?  The client has already connected to the server by the time the client socket is created and the OnClientConnect event has been triggered.  SocketState is only used by the component to check if the socket needs to closed, there is no connecting state concept for a server listening socket. 

Angus

Why does any of this matter, this is a listening server and there is no thread until after a client connects successfully, which you see from the OnClientConnect event.  The server state should always be wsListening. 

BTW, TSslWSocketThrdServer has now been updated or tested for many years, although it descends from TWSocketServer which is heavily used and updated. 

Angus

Nothing to do with any file name, more likely a new bug in Hyper-V on Windows 10, at least never seen it before.  After accessing a couple of virtual machines (with old compilers), discovered Hyper-V has installed 57 Virtual Ethernet Adaptors, for no obvious reason, not seen that before. Uninstalled 50 adaptors, one at a time, and Delphi runs again.

Angus

SSL clients don't need certificates.  Are the DLLs in the same directory as the application, if not you may be opened rubbish from elsewhere on the PC. 

And try other samples, to see if just one failing, specifically try  OverbyteIcsXferTst.dp which is a far more advanced FTP client with better error handling.

Angus

Very unusual to see access violations with OpenSSL, most likely mismatched DLLs or a very old version of ICS and a newer OpenSSL.  Certainly nothing wrong with the components, use SSL all the time.  What about other ICS SSL samples, so they all fail as well? 

Angus

I know the function works because it unpacks and logs real ALPN data, but for completeness I've added logging of the ALPN wire packet:

AlpnCB> inlen: 12 - 02683208687474702F312E31
AlpnCB> Protocols: h2,http/1.1

I also know the ALPN data may be incorrectly formed, OpenSSL simply passes whatever is received in the TLS initial packet, because until I fixed the bug yesterday, Delphi unicode compilers where sending 086008700470047 for http/1.1 if ALPN was specified in SslContext (not the default). This was correctly ignored by the ICS web server, but I noticed it because one of my web pages reports the client ALPN and was truncated by the first null.  I would eventually have found this when testing the Let's Encrypt TLS challenge which does not work yet due to another OpenSSL callback bug.

Angus

Don't believe there a problem in IcsWireFmtToStrList, more likely to be setting the output pointer for the AlpnSelectCallBack function, which means OpenSSL reads a Delphi variable.  Originally it was a local variable but it had gone out of scope when OpenSSL tried to read it. so V8.62 changed it to a TWSocket variable FAlpnProtoAnsi and that worked for Win32, and for Win64 according to my testing here.  But perhaps there is something different about your server implementation to the ICS samples, 

As I said before, simply not using the onSslAlpnSelect should have fixed the issue.

Angus

I've updated the ICS OverbyteIcsSslMultiWebServ sample with the code you show (which comes from the older web server sample), built and tested it for Win64, and it's working fine, no exceptions with ALPN which is being reported correctly, using OpenSSL 1.1.1d Win64. 

The code you show for the onSslAlpnSelect is correct usage, but not really needed since applications should default to HTTP/1.1 anyway unless told to use HTTP/2.  I did fix a memory bug in V8.62 relating to this which I guess could have come back or not been fixed properly.  I would just comment out the loop so the event returns without changing ErrCode and see if your problem goes away. 

I have updated the wsocket ALPN code to suppress any exceptions processing ALPN since it's not really fatal and made IcsWireFmtToStrList check for bad packet formatting. 

More importantly, during testing I found a unicode bug in IcsStrListToWireFmt which is used in HTTPS clients sending the ALPN list which sent a corrupted packet, now fixed, but you are not using that since we do not send the h2 protocol. 

I'll put the source changes in SVN once my own public web server has been updated and been used for at least 24 hours.

Angus