-
Notifications
You must be signed in to change notification settings - Fork 733
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
Sim7600 SSL support v.0.12.0 #808
base: master
Are you sure you want to change the base?
Conversation
…ficates and private keys
First off, thanks for this work. This PR looks like exactly what I was looking for. I'm currently trying to run this code on the sim7600 but it seems like the Currently getting the following error trace:
https://registry.platformio.org/tools/platformio/framework-arduino-sam Thanks again |
@floBik So I replaced all I use the SIM7600E modem, and these commands seems to fail in the
But the Any help will be much appreciated! |
Hi everyone, I have just committed a fix for the problem with the .isEmpty() function, using .length() instead. This should solve the problem. @arduino12 If this doesn't solve your problem, post your code so I can have a look. |
Hi, Thanks @floBik ! I don't have the SIM7600 modem by now so I can't test your fix.. int8_t send_at_cmd(const char *cmd)
{
modem.sendAT(cmd);
return modem.waitResponse() == 1 ? 0 : 1;
}
int8_t send_https_post_request(const char *ip_address, const char *url, const char *content)
{
int ret;
char cmd[256];
send_at_cmd("+HTTPTERM");
if (send_at_cmd("+HTTPINIT"))
return -1; // Failed starting HTTPS service!
sprintf(cmd, "+HTTPPARA=\"URL\",\"https://%s%s\"", ip_address, url);
if (send_at_cmd(cmd))
return -2; // Failed setting HTTPS URL!
if (send_at_cmd("+HTTPPARA=\"CONTENT\",\"application/json\""))
return -3; // Failed setting HTTPS content-type!
sprintf(cmd, "+HTTPDATA=%d,2000", strlen(content));
modem.sendAT(cmd);
if (modem.waitResponse("DOWNLOAD") != 1)
return -4; // Failed setting HTTPS user-data!
modem.stream.print(content);
modem.stream.flush();
if (modem.waitResponse() != 1)
return -5; // Failed sending HTTPS user-data B!
if (send_at_cmd("+HTTPACTION=1"))
return -6; // Failed sending HTTPS post request!
return 0;
} I think its better to use the AT HTTP commands instead of making HTTP(S) packets using ArduinoHttpClient for this modem- Arad :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heyy, I just had a look on your code — thank you very much for your contribution!
Your implementation looks indeed pretty good and I tested it successfully. I just found some minor (mostly stylistic) issues.
return true; | ||
} | ||
|
||
bool setClientPrivateKey(const String& certificateName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we only need one function to set/reset both client cert and client pk, since setting only the cert or only the pk does not make any sense.
if (certificates[mux].length() != 0 && | ||
clientCertificates[mux].length() != 0 && | ||
clientPrivateKeys[mux].length() != 0) { | ||
authmode = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can actually be ORed into authmode
, so no need for the extra branch.
if (waitResponse(5000L) != 1) return false; | ||
|
||
sendAT(GF("+CCHOPEN="), mux, ',', GF("\""), host, GF("\","), port); | ||
// The reply is OK followed by +CIPOPEN: <link_num>,<err> where <link_num> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy/paste?
@@ -292,7 +533,21 @@ class TinyGsmSim7600 : public TinyGsmModem<TinyGsmSim7600>, | |||
/* | |||
* Secure socket layer (SSL) functions | |||
*/ | |||
// No functions of this type supported | |||
public: | |||
bool addCertificate(const char* certificateName, const char* cert, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you want to override addCertificateImpl
and deleteCertificateImpl
resp. here (and have it protected).
String certificates[TINY_GSM_MUX_COUNT]; | ||
String clientCertificates[TINY_GSM_MUX_COUNT]; | ||
String clientPrivateKeys[TINY_GSM_MUX_COUNT]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed?
at->sendAT(GF("+CIPRXGET=4,"), mux); | ||
size_t result = 0; | ||
if (at->waitResponse(GF("+CIPRXGET:")) == 1) { | ||
at->streamSkipUntil(','); // Skip mode 4 | ||
at->streamSkipUntil(','); // Skip mux | ||
result = at->streamGetIntBefore('\n'); | ||
at->waitResponse(); | ||
} | ||
// DBG("### Available:", result, "on", mux); | ||
if (!result) { | ||
at->sockets[mux]->sock_connected = at->modemGetConnected(mux); | ||
} | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like an appropriate error handling (why CIPRXGET
here?)… Maybe just check modemGetConnected()
like for non-ssl.
// NOT SUPPORTED | ||
|
||
|
||
class GsmClientSecureSim7600 : public GsmClientSim7600 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, the SIM7600 only supports two SSL connections — so there should probably be a lower limit somewhere here to enforce this.
if (!clientPrivateKeys[mux].length() != 0) { | ||
deleteCertificate(clientPrivateKeys[mux].c_str()); | ||
} | ||
GsmClientSim7600::stop(maxWaitMs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually also sends a CIPCLOSE
which we probably don't want here, so better don't call this here.
if (certificates[mux].length() != 0) { | ||
deleteCertificate(certificates[mux].c_str()); | ||
} | ||
|
||
if (!clientCertificates[mux].length() != 0) { | ||
deleteCertificate(clientCertificates[mux].c_str()); | ||
} | ||
|
||
if (!clientPrivateKeys[mux].length() != 0) { | ||
deleteCertificate(clientPrivateKeys[mux].c_str()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, the one adding certificates is also responsible for deleting them, so we don't want to delete them unconditionally here.
|
||
return true; | ||
// We assume this works, so we can do ssh disconnect too | ||
// stop the SSH client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// stop the SSH client | |
// stop the SSL client |
Co-authored-by: @Matt-Stedman
The following PR is an evolution of PR #767 and should resolve issue #786 . A few things have been done:
Feel free to test this and give some feedback if something should be changed.