You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The reason will be displayed to describe this comment to others. Learn more.
@mrdeep1 Can you please tag this commit? will it be ok, if I take this commit only, and not refer latest commit? I see earlier commits are also related to some locking mechanism.
The reason will be displayed to describe this comment to others. Learn more.
I couldn't check the full flow earlier but with this commit id, handshake fails which was passing with commit id 9f9129c (PR name OpenSSL: Add in support for configuring an alternative TLS ENGINE)
The reason will be displayed to describe this comment to others. Learn more.
Although close to tagging with 4.3.5rc3, the following PR #1478 fixes a potential, but unlikely, locking issue. There is another (minor) locking issue to do with random number generation that I am looking into which needs to be in the 4.3.5 release.
The reason will be displayed to describe this comment to others. Learn more.
I couldn't check the full flow earlier but with this commit id, handshake fails which was passing with commit id 9f9129c .
It is possible that OPENSSL_cleanup() is needed for cleaning up a previous use of an alternative TLS ENGINE. Are you trying to use an alternative TLS Engine?
[OpenSSL_cleanup() unloads the OpenSSL libraries, which don't get reloaded when you start using OpenSSL again - hence your second coap_new_context() issue.]
Also, I don't think you really need coap_cleanup, coap_startup in the coap-free_context, coap_cleanup, coap_startup, coap_new_context sequence. coap-free_context, coap_new_context should be sufficient.
The reason will be displayed to describe this comment to others. Learn more.
I am using the same user defined tls engine. Also the handshake is failing for first transaction which is without restarting the server.
I can try removing coap_cleanup and coap_startup from the sequence, but in this case coap cleanup was not called at all.
The reason will be displayed to describe this comment to others. Learn more.
If it is failing after the first usage of coap_new_context(), then you are looking into a different issue.
I do not know what your user defined TLS engine is doing, and hence why there is a handshake user. There is nothing obvious as a change to src/coap_openssl.c after 9f9129c that could explain this. I suggest you do a binary chop of the commits between where it is working and where it is failing to narrow down the commit that is giving the issue.
The reason will be displayed to describe this comment to others. Learn more.
removing coap_startup(), and coap_cleanup() from the sequence resolves the restart issue and program runs with older commit 9f9129c supporting multiple start/stop flows and also communication is happening successfully.
The reason will be displayed to describe this comment to others. Learn more.
Good there is movement here.
It would appear that check_pkcs11_engine() is not being fully executed after coap_cleanup() called, and so the ENGINE is not restarted. Why this worked for you with 9f9129c is unclear at present.
The reason will be displayed to describe this comment to others. Learn more.
ok, I did incremental testing and found out that after, 9f9129c, till cec964e it was working fine, but it got broken with d5a0d9c.
I will put additional logs and try to find out what exact change started causing the failure.
The reason will be displayed to describe this comment to others. Learn more.
Good there is movement here.
It would appear that check_pkcs11_engine() is not being fully executed after coap_cleanup() called, and so the ENGINE is not restarted. Why this worked for you with 9f9129c is unclear at present.
What I am doing is, putting coap_startup() and coap_cleanup() in ctor and dtor respectively, and calling coap_new_conext(), coap_free_context() in start and stop APIs respectively. Hope it explains.
The reason will be displayed to describe this comment to others. Learn more.
It would be good to see how you have defined the information in coap_dtls_pki_t, with a particular interest in which coap_pki_key_t you are using in coap_dtls_key_t - is it PEM or PKCS11?
The reason will be displayed to describe this comment to others. Learn more.
The only potential change between those 2 commits is the use of SSL_CTX_use_certificate_chain_file() instead of SSL_CTX_use_certificate_file() for the public key PEM definition.
The reason will be displayed to describe this comment to others. Learn more.
It would be good to see how you have defined the information in coap_dtls_pki_t, with a particular interest in which coap_pki_key_t you are using in coap_dtls_key_t - is it PEM or PKCS11?
our coap_dtls_key_t setup looks like this:
`
coap_dtls_key_t dtls_key;
dtls_key.key_type = COAP_PKI_KEY_DEFINE;
dtls_key.key.define.public_cert.s_byte = cert_name.c_str(); /*PEM certificate file in filesystem*/
dtls_key.key.define.private_key.s_byte = key_name.c_str(); /*subject key - which will be used by openssl engine to load the private key*/
dtls_key.key.define.ca.s_byte = ca_name.c_str(); /*ASCII text file in file system*/
dtls_key.key.define.public_cert_def = COAP_PKI_KEY_DEF_PEM;
dtls_key.key.define.private_key_def = COAP_PKI_KEY_DEF_ENGINE;
dtls_key.key.define.ca_def = COAP_PKI_KEY_DEF_PEM;
The reason will be displayed to describe this comment to others. Learn more.
So, this looks like it is the culprit
key.key.define.public_cert.u_byte[0]) {
switch (key.key.define.public_cert_def) {
case COAP_PKI_KEY_DEF_PEM: /* define public cert */
- if (!(SSL_use_certificate_file(ssl,
- key.key.define.public_cert.s_byte,
- SSL_FILETYPE_PEM))) {
+ if (!(SSL_use_certificate_chain_file(ssl,
+ key.key.define.public_cert.s_byte))) {
return coap_dtls_define_issue(COAP_DEFINE_KEY_PUBLIC,
COAP_DEFINE_FAIL_BAD,
- &key, role);
+ &key, role, 0);
}
break;
case COAP_PKI_KEY_DEF_PEM_BUF: /* define public cert */
So, this is failing after you have only done the first coap_new_context() - correct?
If you reverted out the SSL_use_certificate_file()/SSL_use_certificate_chain_file() change (not the coap_dtls_define_issue() change) does your issue go away?
The reason will be displayed to describe this comment to others. Learn more.
It looks like you have not recompiled the source code where you did dtls_key.key.define.private_key_def = COAP_PKI_KEY_DEF_ENGINE; and just updated the libcoap library. The value of COAP_PKI_KEY_DEF_ENGINE has changed between cec964e and d5a0d9c.
Please recompile any source files of your project that call libcoap code and try again.
The reason will be displayed to describe this comment to others. Learn more.
Sorry! my bad. Copy to root include, and lib got failed earlier due to privilege issue.
I replaced them and recompiled the project with d5a0d9c, yes handshake is happening now.
d87af11
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.
@mrdeep1 Can you please tag this commit? will it be ok, if I take this commit only, and not refer latest commit? I see earlier commits are also related to some locking mechanism.
d87af11
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.
I couldn't check the full flow earlier but with this commit id, handshake fails which was passing with commit id 9f9129c (PR name OpenSSL: Add in support for configuring an alternative TLS ENGINE)
d87af11
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.
Although close to tagging with 4.3.5rc3, the following PR #1478 fixes a potential, but unlikely, locking issue. There is another (minor) locking issue to do with random number generation that I am looking into which needs to be in the 4.3.5 release.
In summary, no plans to tag this commit.
d87af11
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.
It is possible that
OPENSSL_cleanup()
is needed for cleaning up a previous use of an alternative TLS ENGINE. Are you trying to use an alternative TLS Engine?[
OpenSSL_cleanup()
unloads the OpenSSL libraries, which don't get reloaded when you start using OpenSSL again - hence your secondcoap_new_context()
issue.]Also, I don't think you really need
coap_cleanup
,coap_startup
in thecoap-free_context
,coap_cleanup
,coap_startup
,coap_new_context
sequence.coap-free_context
,coap_new_context
should be sufficient.d87af11
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.
I am using the same user defined tls engine. Also the handshake is failing for first transaction which is without restarting the server.
I can try removing coap_cleanup and coap_startup from the sequence, but in this case coap cleanup was not called at all.
d87af11
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.
If it is failing after the first usage of
coap_new_context()
, then you are looking into a different issue.I do not know what your user defined TLS engine is doing, and hence why there is a handshake user. There is nothing obvious as a change to
src/coap_openssl.c
after 9f9129c that could explain this. I suggest you do a binary chop of the commits between where it is working and where it is failing to narrow down the commit that is giving the issue.d87af11
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.
removing coap_startup(), and coap_cleanup() from the sequence resolves the restart issue and program runs with older commit 9f9129c supporting multiple start/stop flows and also communication is happening successfully.
d87af11
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.
Good there is movement here.
It would appear that
check_pkcs11_engine()
is not being fully executed aftercoap_cleanup()
called, and so the ENGINE is not restarted. Why this worked for you with 9f9129c is unclear at present.d87af11
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.
ok, I did incremental testing and found out that after, 9f9129c, till cec964e it was working fine, but it got broken with d5a0d9c.
I will put additional logs and try to find out what exact change started causing the failure.
d87af11
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.
What I am doing is, putting coap_startup() and coap_cleanup() in ctor and dtor respectively, and calling coap_new_conext(), coap_free_context() in start and stop APIs respectively. Hope it explains.
d87af11
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.
It would be good to see how you have defined the information in
coap_dtls_pki_t
, with a particular interest in whichcoap_pki_key_t
you are using incoap_dtls_key_t
- is it PEM or PKCS11?d87af11
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.
The only potential change between those 2 commits is the use of
SSL_CTX_use_certificate_chain_file()
instead ofSSL_CTX_use_certificate_file()
for the public key PEM definition.d87af11
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.
our coap_dtls_key_t setup looks like this:
`
coap_dtls_key_t dtls_key;
`
d87af11
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.
So, this looks like it is the culprit
So, this is failing after you have only done the first coap_new_context() - correct?
If you reverted out the
SSL_use_certificate_file()
/SSL_use_certificate_chain_file()
change (not thecoap_dtls_define_issue(
) change) does your issue go away?d87af11
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.
Still fails after reverting.
Attaching the logs.
handshake_fail.txt
d87af11
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.
It looks like you have not recompiled the source code where you did
dtls_key.key.define.private_key_def = COAP_PKI_KEY_DEF_ENGINE;
and just updated the libcoap library. The value ofCOAP_PKI_KEY_DEF_ENGINE
has changed between cec964e and d5a0d9c.Please recompile any source files of your project that call libcoap code and try again.
d87af11
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.
Sorry! my bad. Copy to root include, and lib got failed earlier due to privilege issue.
I replaced them and recompiled the project with d5a0d9c, yes handshake is happening now.
d87af11
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.
Just checked coap_openssl.c has the same implementation as you suggested, and it seems to be already working in latest develop.
thanks!
d87af11
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.
Thanks for confirming that the latest develop branch is working for you.