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

ArduinoWiFiServer with send-to-all-clients functionality #7612

Merged
merged 7 commits into from
May 15, 2021
Merged

ArduinoWiFiServer with send-to-all-clients functionality #7612

merged 7 commits into from
May 15, 2021

Conversation

JAndrassy
Copy link
Contributor

and with available() working according to Arduino documentation. plus PagerServer example.

should the ESP8266WiFi library have this?

@earlephilhower
Copy link
Collaborator

I think this is pretty cool, @JAndrassy! I would be careful about how available() on SSL connections works with this. IIRC, when I did the SSL server/client available and connected were the 2 big stumbling blocks.

@JAndrassy
Copy link
Contributor Author

I think this is pretty cool, @JAndrassy! I would be careful about how available() on SSL connections works with this. IIRC, when I did the SSL server/client available and connected were the 2 big stumbling blocks.

Thank you @earlephilhower . I am not sure if it is useful, but at least it shows the difference between Servers in Arduino libraries and in ESP libraries.

I attempted to test the SSL server, but telnet-ssl could not connected. I will test it again if this PR should be merged.

@JAndrassy
Copy link
Contributor Author

@earlephilhower, multiple copies of BearSSL::WiFiClientSecure for one connection are not in sync. so ArduinoWiFiServer doesn't work because the copy it holds still returns available() based on _recvapp_len, even if the copy returned before already read the data from the connection. if the data is read again esp8266 crashes.

axTLS works (the copies share the context object)

@JAndrassy JAndrassy marked this pull request as ready for review September 28, 2020 05:46
libraries/ESP8266WiFi/src/ArduinoWiFiServer.h Outdated Show resolved Hide resolved
libraries/ESP8266WiFi/src/ArduinoWiFiServer.h Outdated Show resolved Hide resolved
libraries/ESP8266WiFi/src/ArduinoWiFiServer.h Outdated Show resolved Hide resolved
@d-a-v
Copy link
Collaborator

d-a-v commented Sep 29, 2020

multiple copies of BearSSL::WiFiClientSecure for one connection are not in sync.

From what I understand, there's only one copy for each client, given by ::accept(). Am I wrong ?

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Sep 29, 2020

multiple copies of BearSSL::WiFiClientSecure for one connection are not in sync.

From what I understand, there's only one copy for each client, given by ::accept(). Am I wrong ?

the concept of Arduino Client is that multiple copies of WiFiClient/EthernetClient (created as return values or parameters passed by copy) can interact with the same TCP connection. WiFiClient should be only a pointer to internal representation. In ESP8266WiFi library the WiFiClient does it with ClientContext. the axTLS does it very very nice with the std shared pointer to SSLContext.

(I think this strange concept of Arduino API classes has origin in simulating Java objects behaviour for some sketch source compatibility with Processing framework. so no -> or & only .)

In the ArduinoWiFiServer template the array of 'monitored' WiFiClient objects has instance pointing to the connection and server.available() returns a copy always when data are available for it. So the WiFiClient object in the array
must have the same information about count of data available as the copy which reads the data.

@d-a-v
Copy link
Collaborator

d-a-v commented Sep 29, 2020

the concept of Arduino Client is that multiple copies of WiFiClient/EthernetClient (created as return values or parameters passed by copy) can interact with the same TCP connection.

Right, I think we agree, I was meaning: "From what I understand, there's only one TCP instance for each client, given by ::accept().".
Do we really agree ? If we do I don't see the reason why several SSL clients should not work.

However maybe you should call on every occasion and even if it's useless ::connected() and ::available() on every client occurence to make the TLS state machine move forward. All of them, but of course only the index one is returned to the user.

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Sep 29, 2020

the concept of Arduino Client is that multiple copies of WiFiClient/EthernetClient (created as return values or parameters passed by copy) can interact with the same TCP connection.

Right, I think we agree, I was meaning: "From what I understand, there's only one TCP instance for each client, given by ::accept().".
Do we really agree ? If we do I don't see the reason why several SSL clients should not work.

However maybe you should call on every occasion and even if it's useless ::connected() and ::available() on every client occurence to make the TLS state machine move forward. All of them, but of course only the index one is returned to the user.

for BearSSL the problem are the two copies of WiFiClientSecure for the same connection. one in the array of monitored clients and second in sketch filled at return from server.available(). the sketch reads data over its copy. the buffer counter _recvapp_len gets to 0. but the instance in the array still has the original value of _recvapp_len and returns the client again as if it has data available.

available() calls bool() for every monitored client and bool() calls connected.

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 5, 2020

for BearSSL the problem are the two copies of WiFiClientSecure for the same connection

That is true. I wasn't aware. WiFiClientSecure's context must move away to an external context for your class to work with it in the arduino way.
Like with WiFiClient which is stateless and pointing to a ClientContext which holds the TCP connection.

@JAndrassy JAndrassy closed this Oct 6, 2020
@JAndrassy JAndrassy reopened this Oct 6, 2020
@d-a-v
Copy link
Collaborator

d-a-v commented Oct 28, 2020

Can you try with #7680 ?

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Nov 3, 2020

@d-a-v

connectedClients[i].status() == ESTABLISHED is never true, because status() is not redirected to Ctx.

and second client gets

juraj@nuc ~ $ telnet-ssl -z ssl 192.168.1.117 2323
Trying 192.168.1.117...
SSL: reason(0)
SSL_Connect: Input/output error

for the second client "BSSL:_connectSSLServerRSA: OOM error" is logged
WiFiServerSecureAxTLS could handle 4 clients

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 6, 2020

Thanks to @earlephilhower 's suggestion in gitter server.setBufferSizes() (512 was not enough, but 1024 is OK),
and his ssl server example from which I copied keys and server.setECCert(),
and also thanks to a fix on ssl::status() not properly routed to ssl context in #7680 that I'm about to I just push,
I could run your PR with 3 SSL clients and it's working well (showing two below):

$ telnet-ssl -z ssl 10.0.1.144 2323
Trying 10.0.1.144...
SSL: Server has a self-signed certificate
SSL: unknown Issuer: /C=AU/ST=Some-State/O=Internet Widgits Pty Ltd
Connected to 10.0.1.144.
Escape character is '^]'.
toto
echo: toto
titi
echo: titi
$ telnet-ssl -z ssl 10.0.1.144 2323
Trying 10.0.1.144...
SSL: Server has a self-signed certificate
SSL: unknown Issuer: /C=AU/ST=Some-State/O=Internet Widgits Pty Ltd
Connected to 10.0.1.144.
Escape character is '^]'.
toto
titi

patched example:

  // Attach the server private cert/key combo
  BearSSL::X509List *serverCertList = new BearSSL::X509List(server_cert);
  BearSSL::PrivateKey *serverPrivKey = new BearSSL::PrivateKey(server_private_key);
  server.setECCert(serverCertList, BR_KEYTYPE_KEYX|BR_KEYTYPE_SIGN, serverPrivKey);
  server.setBufferSizes(1024,1024);
  server.begin();

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but to merge after #7680 (needed for this PR when SSL is used)
(or after #7680 is removed from this PR)

and with available() working according to Arduino documentation
plus PagerServer example
@JAndrassy
Copy link
Contributor Author

Approving but to merge after #7680 (needed for this PR when SSL is used)
(or after #7680 is removed from this PR)

#7680 is removed

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

Successfully merging this pull request may close these issues.

None yet

3 participants