coap_openssl tidy up, fix first PSK fails if both PSK and PKI are enabled on server #179

mrdeep1 · 2018-04-27T15:57:43Z

A late change to the PKI Pull request got missed for the TLS case
in coap_dtls_context_set_pki() for the mapping of private key types to
the OpenSSL version. Have pulled common DTLS and TLS code into a separate
function

Fixed coap_(d)tls_free_session to not shut down a session that is not yet
fully started, as well as calling SSL_shutdown() twice if appropriate.
The ssl parameter is set to NULL after shutdown to prevent re-use

Made coap_dtls_context_set_psk() smarter in what whas updated, depending on
whether a client or server

Make sure that coap_dtls_psk_client_callback is defined in
coap_dtls_context_set_psk() in case coap_dtls_context_set_pki() was called
first. This fixes the first time issue after startup where a PSK client
was unable to establish a DTLS session - subsequent times were successful

Added in a set of callbacks, invoked from coap_dtls_context_set_pki(), so that
a server can differentiate between an incoming PSK or PKI connection request.
These are only invoked if no call_back was defined in the setup_data

Output error messages if PKI or PSK was not set up before a server endpoint was
set up and (D)TLS is being used.

Update internal coap_dtls_context_set_*() functions parameters to remove unused parameters and add in a server parameter

Add in TCP KeepAlive to TCP socket to pick up on failing TCP connections

mrdeep1 · 2018-04-28T19:16:22Z

Remove restriction on requiring PSK / PKI to be set up for a client and using coaps://. This allows the client to talk to a server that is PKI enabled. See Issue #174

mrdeep1 · 2018-05-18T12:12:30Z

Updated to set SSL functions at the SSL level, not the CTX level when libcoap is invoked by a client

mrdeep1 · 2018-06-01T14:32:51Z

Updated in line with the mail thread https://sourceforge.net/p/libcoap/mailman/message/36322888/

mrdeep1 · 2018-06-04T07:45:46Z

I still need to add in a callback for server side SNI handling.

mrdeep1 · 2018-06-07T20:59:38Z

rebased on develop

Application CN and SNI callback support provided.

libcoap can now the following checking within libcoap

/* Options to enable different TLS functionality in libcoap /
uint8_t verify_peer_cert; / 1 if peer cert is to be verified /
uint8_t require_peer_cert; / 1 if peer cert is required /
uint8_t allow_self_signed; / 1 if self signed certs are allowed /
uint8_t allow_expired_certs; / 1 if expired certs are allowed /
uint8_t cert_chain_validation; / 1 if to check cert_chain_verify_depth /
uint8_t cert_chain_verify_depth; / recommended depth is 3 /
uint8_t check_cert_revocation; / 1 if revocation checks wanted /
uint8_t allow_no_crl; / 1 ignore if CRL not there /
uint8_t allow_expired_crl; / 1 if expired crl is allowed */

mrdeep1 · 2018-07-06T09:40:31Z

I will do the rebase and update coap-server to be more capable of being more rigorous in the Cert checking shortly.

mrdeep1 · 2018-07-06T14:02:10Z

rebased and added in -C cafile and -R root_cafile options to both client and server. If -C is defined, then there is verification of certificates.

obgm

Here is my first round of (minor) comments. I have not yet tested the new PKI-functionality but otherwise, the code looks good (at least as good as code that deals with ASN.1 and X.509 can look ;-)

obgm · 2018-07-20T08:25:22Z

include/coap/coap_dtls.h

+ * @param asn1_public_cert  The ASN.1 encoded (DER) X.509 certificate
+ * @param asn1_length  The ASN.1 length
+ * @param session  The coap session associated with the certificate update
+ * @param depth  Depth in cert chain.  If 0, then client cert, else a CA


Could this possibly be negative? Otherwise, I would go for an unsigned type.

depth is derived from X509_STORE_CTX_get_error_depth() which returns an int, so I would prefer this not to be changed, even though the documentation currently states

X509_STORE_CTX_get_error_depth() returns a non-negative error depth

Well, the API in coap_dtls.h is agnostic of the TLS library, therefore I do not think that sticking to OpenSSL idiosyncrasies is preferable to cleaning up the API where possible. You would need to repeat the OpenSSL comment here and in the following explain to people repeatedly why this will never be a negative value...

Updated to be unsigned as depth should be 0 or greater.

obgm · 2018-07-20T10:52:21Z

include/coap/coap_dtls.h

+ * Common Name (CN) component of the subject name
+ *
+ * @param cn  The determined CN from the certificate
+ * @param asn1_public_cert  The ASN.1 encoded (DER) X.509 certificate


There are various parameter names, type names, enum values and struct fields that have "asn1" in their name. To me, it is unclear which internal representation is used for ASN.1. Are these components always serialized as DER, or....? Does this need to be reflected in their identifiers?

OpenSSL is handling the ASN.1 as DER encoding as far as I can tell.

@obgm I can make this asn1der_ prefix if you want.

obgm · 2018-07-20T11:01:46Z

include/coap/coap_dtls.h

+  const uint8_t *ca_cert;     /**< ASN1 Common CA Cert */
+  const uint8_t *public_cert; /**< ASN1 Public Cert */
+  const uint8_t *private_key; /**< ASN1 Private Key */
+  int ca_cert_len;            /**< ASN1 CA Cert length */


These three lengths should be size_t.

obgm · 2018-07-20T12:23:24Z

include/coap/coap_dtls.h

+  uint8_t check_cert_revocation;   /**< 1 if revocation checks wanted */
+  uint8_t allow_no_crl;            /**< 1 ignore if CRL not there */
+  uint8_t allow_expired_crl;       /**< 1 if expired crl is allowed */
+  uint8_t reserved[6];             /**< Reserved - set to 0 - causes


Is there any reason why we do not rely on the compiler to do the alignment?

By having the reserved defined, they can then be used later for additional options without breaking the coap_dtls_pki_t overall size/order breaking old code using a newer library, so I would prefer it to be left 'as is'. Comment on reserved updated.

obgm · 2018-07-20T12:26:46Z

include/coap/coap_dtls.h

+   * If not NULL, then application is handling the characteristics of the TLS
+   * connection setup in the defined call-back handler.  If set, then none of
+   * the options or call-backs above are acted on. */
+  coap_dtls_security_setup_t full_call_back;


Can we change the field name here? This is not very self-explanatory:

coap_dtls_pki_t pki; pki.full_call_back = func;

Renamed to app_override_tls_setup_call_back.

obgm · 2018-07-20T14:12:58Z

src/coap_openssl.c

+      /* Add CA to the trusted root CA store */
+      st = SSL_CTX_get_cert_store(ctx);
+      /* Flush out existing errors */
+      while ((e = ERR_get_error()) != 0) { }


For better readability, please move the closing parenthesis to the next line.

obgm · 2018-07-20T14:15:18Z

src/coap_openssl.c

+
+      /* We do not add CA for client to send to client as we are client! */
+
+      /* Add CA to the trusted root CA store */


There seems to be a lot of duplicate code here. I suggest factoring it out into a separate function.

Fixed by making use of a common function add_ca_to_cert_store()

obgm · 2018-07-20T14:20:24Z

src/coap_openssl.c

+    }
+    /* Otherwise look for the CN= field */
+    X509_NAME_oneline(X509_get_subject_name(x509), buffer, sizeof(buffer));
+    cn = strcasestr(buffer, "CN=");


strcasestr() is a nonstandard extension. The idea is to keep these out of the core library.

Fixed by using local equivalent

obgm · 2018-07-20T14:34:06Z

src/coap_openssl.c

  return context->psk_pki_enabled;
 }


 void coap_dtls_free_context(void *handle) {
+  int i;


Should be of type size_t or so in alignment with `context->sni_count.

obgm · 2018-07-20T14:36:26Z

src/coap_session.c

@@ -631,33 +632,43 @@ coap_session_t *coap_new_client_session_psk(
  if (!session)
    return NULL;

-  if (identity) {
+  if (identity && strlen(identity) > 0) {


Please enclose the comparison in parentheses as always.

Fixed. Brain is not always fully in gear when different projects have different coding styles!

mrdeep1 · 2018-07-20T16:22:34Z

Just did a rebase on develop push - none of the comments have yet been addressed.

To enable early detection of a TCP connection failure. Tidy up linelength of some surrounding functions.

coap_dtls_pki_t struct has been enhanced to have more configuration options. These configuruation options then tell libcoap what needs to get checked by the underlying OpenSSL library rather than the user application has to do it. There still is support for the user application to do all the Certificate checking etc. A late change to the PKI Pull request got missed for the TLS case in coap_dtls_context_set_pki() for the mapping of private key types to the OpenSSL version. Have now pulled common DTLS and TLS code into a seperate function Fixed coap_(d)tls_free_session to not shut down a session that is not yet fully started, as well as calling SSL_shutdown() twice if appropriate The ssl parameter is set to NULL after closedown Made coap_dtls_context_set_psk() smarter in what was updated, depending on whether called by a client or server Make sure that coap_dtls_psk_client_callback is defined in coap_dtls_context_set_psk() in case coap_dtls_context_set_pki() was called first. This fixes the first time issue after startup where a PSK client was unable to establish a DTLS session - subsequent times were successful Added in a set of callbacks, invoked from coap_dtls_context_set_pki(), so that a server can differentiate between an incoming PSK or PKI connection request. These are only invoked if no full_call_back was defined in the setup_data Add in a set of user definable callbacs for checking CN and SNI provided information Add in support for the generation and detection of ALPN externsion Output error message if PKI or PSK was not set up before a server endpoint was set up and (D)TLS is being used. Allow a client to talk to a server that is PKI enabled without the need for a PreShared Key or PKI Certificate Set the SSL information at the SSL, not the CTX level when it is a client

…dtls_pki_t coap_context_set_pki_root_cas(): New coap_dtls_context_set_psk(): remove unused parameters, add in server parameter coap_dtls_context_set_pki(): add in server parameter coap_dtls_pki_t: Additional configuration parameters coap_dtls_security_setup_t: Updated coap_dtls_cn_callback_t: New coap_dtls_sni_callback_t: New coap_pki_key_t: New coap_pki_key_pem_t: New coap_pki_key_asn1_t: New coap_dtls_sni_t: New Comments tidy up. Better Doxygen comments for coap_dtls.h and net.h Doxygen fix for debug.h (introduced by obgm#219 merge) Doxygen update coap_endpoint_new_dtls_session() for in coap_session.h

…*() functions coap_dtls_context_set_pki_root_cas(): New function coap_dtls_context_set_psk(): Remove unused parameters, add in server parameter coap_dtls_context_set_pki(): Add in server parameter coap_dtls_context_check_keys_enabled(): Add in UNUSED src/coap_tinydtls.c: coap_dtls_free_session() set coap_session->tls = NULL

…ions Update calls to coap_dtls_context_set_*() functions "\n" terminate coap_log() calls src/coap_session.c: coap_session_disconnected() Report the session disconnected reason Warn if coap_new_client_session_psk() called without both key and identity defined (OpenSSL cannot handle this) src/net.c: coap_context_set_pki_root_cas(): New function

mrdeep1 · 2018-07-23T10:24:15Z

Code pushed, rebased off develop. Only change not made is whether the asn1_ prefix should really be asn1der_.
Doxygen comments also updated in the include files.

obgm · 2018-07-23T12:24:49Z

examples/coap-server.c

+    dtls_pki.pki_key.key.pem.ca_file = ca_file;
+    /* If general root CAs are defined */
+    if (root_ca_file) {
+      coap_context_set_pki_root_cas(ctx, root_ca_file, NULL);


During testing, I found it very useful to specify a directory such as /etc/ssl/certs like this:

diff --git a/examples/coap-server.c b/examples/coap-server.c index 5f9d2c7..5f75009 100644 --- a/examples/coap-server.c +++ b/examples/coap-server.c @@ -389,7 +389,12 @@ fill_keystore(coap_context_t *ctx) { dtls_pki.pki_key.key.pem.ca_file = ca_file; /* If general root CAs are defined */ if (root_ca_file) { - coap_context_set_pki_root_cas(ctx, root_ca_file, NULL); + struct stat stbuf; + if ((stat(root_ca_file, &stbuf) == 0) && S_ISDIR(stbuf.st_mode)) { + coap_context_set_pki_root_cas(ctx, NULL, root_ca_file); + } else { + coap_context_set_pki_root_cas(ctx, root_ca_file, NULL); + } } coap_context_set_pki(ctx, &dtls_pki); if (key_defined)

Yes, I like that change.

mrdeep1 · 2018-07-23T12:50:59Z

Code pushed - done change in client as well as man pages / usage().

obgm · 2018-07-24T12:23:06Z

man/coap_encryption.txt.in

+coap_encryption(3)
+===================
+:doctype: manpage
+:man source:   coap_tls_library


This should be :man source: coap_encryption, I suppose.

The joys copying files and using them as templates! Fixed.

obgm · 2018-07-24T12:23:54Z

man/coap_encryption.txt.in

+to provide a PSK.  The Server must have a Hint defined and the Client must
+have an Identity defined.
+
+For PKI setup, if the libcoap PKI configuration options do not handle a specific


I wonder if we could also give an example for RPK usage?

I'm not entirely sure after investigation that OpenSSL 1.1.0 properly supports RPK, but am willing to be proven wrong. Does tinyDTLS support this as an alternative?

Yes, TinyDTLS supports RPK (and only RPK, no full-fledged PKI).

The reason you might want asymmetric cryptography but not check certificate chains (consider 150 or so root CAs stored in a class 1 IoT device...)

Fully understand why RPK makes sense, need it for the DOTS protocol. However https://en.wikipedia.org/wiki/Comparison_of_TLS_implementations confirms my suspicion of OpenSSL as unknown. My guess is that RPK would be done using the asn1 variant.

I was specifically concerned about the API. I would expect to use RPKs in libcoap similar to full X.509 certificates but without transitive trust. The root CA files then pretty much would be the known RPKs, and the certfiles an RPK instead of a full certificate. (All being DER-encoded ASN.1 structures.)

The right way forward here is to add in coap_context_set_rpk() and coap_new_client_session_rpk() for RPK support. This has 2 benefits

The server can support all 3 of PSK. PKI and RPK concurrently, so the client can choose any of the 3 methods

RPK can then use the TinyDTLS support for RPK.
The functions would be modeled on the *_pki() equivalent, but the setup_data parameter would be different. I would expect setup_data to contain a parameter pointing to a directory containing the known RPKs.

This would be done under a new PR.

Okay, sounds reasonable to me. I suppose, this would be somehow related to PR #128.

Yes, I was planning to use what made sense out of #128 with these new functions for TinyDTLS.

Split out old coap_context(3) into coap_context(3), coap_encryption(3) and coap_session(3) with coap_encryption(3) primarily updated to reflect the updated PKI configuration options. coap-client.txt.in coap-server.txt.in Update with new options making use of updated PKI interface

Also, remove coap_endpoint_new_dtls_session() as it is an internal function

Also, exclude coap_endpoint_new_dtls_session() as it is an internal function

mrdeep1 · 2018-07-24T13:50:32Z

Push with man/coap_encryption.txt.in typo fixed.

obgm · 2018-07-25T08:11:52Z

Commit e1fbd15 has a typo in its commit message (s/mutial/mutual/). If you want to fix that, I will wait for it, otherwise, I see this PR ready for merge. Awesome work!

Updated to support the new coap_dtls_pki_t. Add in new -C and -R options to support / test PKI mutual authentication Make sure time resource is triggered no more frequently than once a sec.

mrdeep1 · 2018-07-25T08:17:04Z

Commit typo corrected.

mrdeep1 mentioned this pull request Apr 28, 2018

libcoap and DTLS with public keys #174

Closed

mrdeep1 force-pushed the tcp-setup branch from 96a8ed7 to 9b0e108 Compare April 28, 2018 19:11

mrdeep1 force-pushed the tcp-setup branch from 9b0e108 to b19cf24 Compare May 18, 2018 12:11

mrdeep1 force-pushed the tcp-setup branch 2 times, most recently from 7d87ace to 64e13bc Compare June 1, 2018 14:29

mrdeep1 mentioned this pull request Jun 1, 2018

Add in support for PKI for coap-client and coap-server #176

Merged

mrdeep1 force-pushed the tcp-setup branch from 64e13bc to 46542f9 Compare June 7, 2018 20:54

mrdeep1 force-pushed the tcp-setup branch from 46542f9 to 58821d9 Compare June 7, 2018 21:05

mrdeep1 force-pushed the tcp-setup branch from 58821d9 to ee65577 Compare July 6, 2018 14:00

mrdeep1 force-pushed the tcp-setup branch from ee65577 to dcf91cd Compare July 6, 2018 15:43

obgm requested changes Jul 20, 2018

View reviewed changes

mrdeep1 force-pushed the tcp-setup branch from dcf91cd to df2c01e Compare July 20, 2018 16:17

mrdeep1 added 5 commits July 23, 2018 10:26

src/coap_io.c: Add in TCP Keepalive to socket

c9763db

To enable early detection of a TCP connection failure. Tidy up linelength of some surrounding functions.

mrdeep1 force-pushed the tcp-setup branch from df2c01e to bf8f54c Compare July 23, 2018 10:21

obgm reviewed Jul 23, 2018

View reviewed changes

mrdeep1 force-pushed the tcp-setup branch from bf8f54c to 8f53f77 Compare July 23, 2018 12:49

obgm reviewed Jul 24, 2018

View reviewed changes

mrdeep1 added 3 commits July 24, 2018 14:49

libcoap-2.{map|sym}: Add in coap_context_set_pki_root_cas()

e2c3571

Also, remove coap_endpoint_new_dtls_session() as it is an internal function

Makefile.am: Exclude coap_dtls_context_set_pki_root_cas

95fb280

Also, exclude coap_endpoint_new_dtls_session() as it is an internal function

mrdeep1 force-pushed the tcp-setup branch from 8f53f77 to e1fbd15 Compare July 24, 2018 13:49

obgm approved these changes Jul 25, 2018

View reviewed changes

examples/coap-server.c, examples/client.c Support new coap_dtls_pki_t

bb0b195

Updated to support the new coap_dtls_pki_t. Add in new -C and -R options to support / test PKI mutual authentication Make sure time resource is triggered no more frequently than once a sec.

mrdeep1 force-pushed the tcp-setup branch from e1fbd15 to bb0b195 Compare July 25, 2018 08:16

obgm merged commit 6bb40d5 into obgm:develop Jul 25, 2018

mrdeep1 deleted the tcp-setup branch July 25, 2018 08:20

obgm mentioned this pull request Jul 25, 2018

Monitor status of client sessions and send ping messages #183

Closed


		/* We do not add CA for client to send to client as we are client! */

		/* Add CA to the trusted root CA store */

coap_openssl tidy up, fix first PSK fails if both PSK and PKI are enabled on server #179

coap_openssl tidy up, fix first PSK fails if both PSK and PKI are enabled on server #179

Conversation

mrdeep1 commented Apr 27, 2018 • edited Loading

mrdeep1 commented Apr 28, 2018

mrdeep1 commented May 18, 2018

mrdeep1 commented Jun 1, 2018

mrdeep1 commented Jun 4, 2018

mrdeep1 commented Jun 7, 2018

mrdeep1 commented Jul 6, 2018

mrdeep1 commented Jul 6, 2018

obgm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 commented Jul 20, 2018

mrdeep1 commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 commented Jul 24, 2018

obgm commented Jul 25, 2018

mrdeep1 commented Jul 25, 2018

mrdeep1 commented Apr 27, 2018 •

edited

Loading

mrdeep1 Jul 20, 2018 •

edited

Loading