PKI: Make (D)TLS operation consistent across all TLS libraries

[mrdeep1]

The use of the verify_peer_cert and require_peer_cert variables in the
coap_dtls_pki_t structure was giving inconsistent results across all the
TLS libraries. This primarily was down to the large numbers of options
available to control the TLS handshakes in OpenSSL compared to the limited
control available to mbedTLS port which followed later. require_peer_cert is
not easy to control in mbedTLS as it is an implicit configuration based on how
other, not always related, items were configured.

require_peer_cert was used by the server to control whether the client could
use anonymous certificates or not. This is now controlled by verify_peer_cert.

require_peer_cert variable has been replaced with check_common_ca, so that
the OpenSSL functionality can continue, but enable GnuTLS / mbedTLS to
produce the same results. This allows peers to mutually authenticate (because
the peer certs are signed by the same common CA) or not which was in effect
controlled by verify_peer_cert previously.

If check_common_ca is set, then allow_self_signed is ignored.

If is_rpk_not_cert is set, then all certificate validation is ignored.

In the examples, use of the -R option unsets check_common_ca, so disables
mutual authentication support by having a common CA. This was needed as
mbedTLS and GnuTLS only have a single trust store for CAs.

configure.ac:

Increase the number of mbed libraries to use when checking for mbedTLS.

examples/client.c:
examples/coap-rd.c:
examples/coap-server.c:

Add in -n (unset verify_peer_cert) option. In the case of coap-server and
coap-rd, make -n refer to verify_peer_cert.
Add in TLS library capabilites in usage().

Update usage() documentation as appropriate, with some changes to fit
everything into a 80 column output.

include/coap2/coap_dtls.h:
include/coap2/net.h:

Update with variable changes, and make the coap_dtls_pki_t parameter const for
the *_context_set_pki() functions.

man/coap-client.txt.in:
man/coap-rd.txt.in:
man/coap-server.txt.in:

Update documentation to reflect the examples option usage.

man/coap_context.txt.in:
man/coap_encryption.txt.in:
man/coap_session.txt.in:

Update with the new variable name and document as appropriate.

src/coap_gnutls.c
src/coap_mbedtls.c
src/coap_notls.c
src/coap_openssl.c
coap_tinydtls.c

Update to make variable usage consistent. Update logging from LOG_WARNING to
LOG_INFO where there is an override of a PKI check failure by one of the
coap_dtls_pki_t variables.

Timing window closed for TLS where the peer does not like a certificate, sends
fatal alert and closes connection. local then fails on writing the next
handshake - but now also reads in alert and reports on it.

src/coap_io.c:

Update logging from LOG_WARNING to LOG_INFO for EPIPE or ECONNRESET errors in
coap_socket_write().

src/net.c:

Handle the const coap_dtls_pki_t parameter in coap_context_set_pki() function.

This replaces #557

[mrdeep1]

Code updated and pushed to update help/man documentation - triggered by #563 conversation.

[mrdeep1]

rebased and updated code pushed

[mrdeep1]

rebased and updated code pushed

[boaks]

commit 9b39bd9

make clean
./configure --enable-dtls --with-openssl --disable-doxygen --disable-manpages

libcoap configuration summary:
      libcoap package version : "4.3.0beta"
      libcoap library version : "2.1.0"
      libcoap API version     : "2"
      libcoap DTLS lib extn   : "-openssl"
      host system             : "x86_64-pc-linux-gnu"
      build with TCP support  : "yes"
      build DTLS support      : "yes"
         -->  OpenSSL around  : "yes" (found OpenSSL 1.1.1)
              OPENSSL_CFLAGS  : ""
              OPENSSL_LIBS    : "-lssl -lcrypto"
      build using epoll       : "yes"
      enable small stack size : "no"
      build doxygen pages     : "no"
      build man pages         : "no"
      build unit test binary  : "no"
      build examples          : "yes"
      build with gcov support : "no"
      build shared library    : "yes"
      build static library    : "yes"

make all
sudo make install
sudo ldconfig

I built the examples with openssl to test it.

On my build, PSK is broken for coap-client, and working for coap-server.

coap-client coaps://127.0.0.1:5684 -u me -k so 
WARN Identity (PSK) not defined
EMRG cannot create client session

[boaks]

On the client side, isn't it the "server certificate", which is intended to be signed by the clients CA?

[mrdeep1]

Comment corrected

[boaks]

The PSK client issue seems to be introduced before this PR.

client.c

static ssize_t
cmdline_read_user(char *arg, unsigned char **buf, size_t maxlen) {
  size_t len = strnlen(arg, maxlen);
  if (len) {
    *buf = (unsigned char *)arg;
    /* len is the size or less, so 0 terminate to maxlen */
    buf[len] = '\000';
  }
  /* 0 length Identity is valid */
  return len;
}

must be

    (*buf)[len] = '\000';

otherwise the pointer will be set to zero.

I open a new issue.

[mrdeep1]

Code pushed, including a (*buf)[len] = '\000'; fix in both the coap-server and coap-client.

My regression tests (which include valgrind) had not found this as I usually do coap-client -u me -k so coaps://127.0.0.1. Good that we have caught this.

[boaks]

I use

coap-server -p 5683 -v 6 -c server.pem -R trustStore.pem -C rootTrustStore.pem

I expect, that the CERTIFICATE_REQUEST contains the DNs of -C ("C=CA,L=Ottawa,O=Eclipse IoT,OU=Californium,CN=cf-root"), but it's empty.

Wireshark:

dtls.handshake.dnames_len: 0

[mrdeep1]

I agree that this does not look right. Is it possible to get a copy of these .pem files as well as the .pem files you are using for the client to debug what is going on here?

[mrdeep1]

Found the DN sending issue in OpenSSL i/f and code fix pushed for testing.

[boaks]

Great! The CERTIFICATE_REQUEST contains the DNs again.

I use Eclipse Californium / Interoperability Tests. The pem could be found in that directory.

To test this PR, I created a Eclipse Californium / PR1448 with the changes for the new options.

[mrdeep1]

Fixed common CA issue in OpenSSL and updated -R documentation. Code pushed.

[boaks]

According RFC4279, ClientKeyExchange the client didn't send the hint, it just receives that from the server. So, how is that hint intended to be used on the client side? Should the client select the identity based on that?

[mrdeep1]

The server provided Identity Hint could be indicating different, say, services (which have different PSKs) and so this gives the client the ability to chose a new key and new user (identity) to continue the (D)TLS set up.

However, TLS1.3 does not support this as the client includes the Identity in the Client Hello before the server can send an Identity Hint.

If the parameter validate_ih_call_back is set in coap_dtls_cpsk_t, then libcoap downgrades to (D)TLS1.2 (with a warning) so that Identity Hints are supported.

[boaks]

Sure, for the server.

But this is the help for the client. so, what is the client intended to do with a hint, provided at its command line?

[mrdeep1]

The server issues a hint, the client receives the hint and the client (D)TLS layer can optionally call a callback with the hint and the callback then returns a key and identity to use for the ongoing (D)TLS session setup. See SSL_set_psk_client_callback().

So, if the client invokes with the -h option, the user of the client can define alternative key/identity depending on the hint received from the server.

[mrdeep1]

If the -h option is not used, then the -k and -u options are used for the key and identity.

[boaks]

I'm not sure, if that works. Using only the hint as key requires that multiple server's are using a system of hints or shared crendtials. Do you have feedback from users about that?

RFC 42795.2. Identity Hint, with that, I would mention, that the hint on the client side should only be used, if the server chose the hint accordingly.

If it's used for that, why does the documentation mention "new user"?
Even, if the user is known, when the server sends it's hint, then the credentials are selected by the hint, or?

[mrdeep1]

https://tools.ietf.org/html/rfc4279#section-2 also states

   Both clients and servers may have pre-shared keys with several
   different parties.  The client indicates which key to use by
   including a "PSK identity" in the ClientKeyExchange message (note
   that unlike in [SHAREDKEYS], the session_id field in ClientHello
   message keeps its usual meaning).  To help the client in selecting
   which identity to use, the server can provide a "PSK identity hint"
   in the ServerKeyExchange message.  If no hint is provided, the
   ServerKeyExchange message is omitted.  See Section 5 for a more
   detailed description of these fields.

So a client can choose an identity and possibly an associated pre-shared key based on receiving the hint. I agree that new is confusing and perhaps alternative should be used. The server will still choose the pre-shared key to use based on the received identity(user).

No specific feedback from the users -but it is functionality in TLS1.2 or earlier.

[mrdeep1]

Documentation updated to not use new and additional clarification on the server side. Code pushed for review.

[boaks]

A build using tinydtls also shows the documentation for x509 support.
Maybe, the not supported arguments can be removed.

Just as idea/extension: is it possible, to load trusted public keys for RPK, e.g. by -R?

[mrdeep1]

A build using tinydtls also shows the documentation for x509 support.
Maybe, the not supported arguments can be removed.

It is not (easily) possible to do this in the man pages - hence PKI Options (if supported by underlying (D)TLS library)

Just as idea/extension: is it possible, to load trusted public keys for RPK, e.g. by -R?

Not at present.

[boaks]

It is not (easily) possible to do this in the man pages

I ment the text, displayed with argument -h. And -R is also accepted without any warning.

[mrdeep1]

Fixed -R option for tinydtls not reporting anything (code pushed). I am holding fire on the -h option as I do not like inconsistency in the man and help documentation - especially as the options are described in https://libcoap.net/doc/reference/4.2.1/man_coap-client.html etc.

[boaks]

Maybe a build-variant hint in a footline with "tinyDTLS only supports RPK, not x.509", or "openssl only supports x.509, not RPK"?

[mrdeep1]

Issue should now be fixed and subsequent alert non longer sent. Code pushed.

[boaks]

Issue should now be fixed and subsequent alert non longer sent. Code pushed.

Also on my side ;-). Great!

[mrdeep1]

Timing window closed for TLS where the peer does not like a certificate, sends fatal alert and closes connection. local then fails on writing the next handshake - but now also reads in alert and reports on it.

If check_common_ca is set, then allow_self_signed is ignored.

Code changes pushed

If is_rpk_not_cert is set, then all certificate validation is ignored.

[boaks]

@obgm
@mrdeep1

Hi together,

FMPOV, this PR is really a great improvement. Is there any reason, why it is still not merged?

[mrdeep1]

@boaks Apologizes, but I do not have merge capabilities. I have just rebased and done a push to clear some examples conflicts.

[boaks]

@mrdeep1

Thanks! I will start to test your latest changes it in the evening.

@obgm

And your impression? Any changes to do? Or just not enough time to review it by yourself?

[obgm]

I am doing a review within the next couple of days. Sorry for the delay.

[boaks]

That sound great!
I really like that consistent way. It makes it much easier to build interoperability tests on it!

[boaks]

@mrdeep1

I updated to your latest commit. The interoperabilitiy tests are still green :-).
Extending the interoperability tests to check the alerts, I had minor findings in tinydtls and mbedtls, but this is not caused by this PR.

I'm looking forward to have this PR merged.

[mrdeep1]

@boaks Thanks for the confirming feedback. I have seen your comment re Fatal Close_Verify alert and TinyDTLS - but have not so far found a definitive statement on that. Were you seeing the same in Mbed TLS?

[boaks]

About CLOSE_NOTIFY:
wiki see TLS Alert Protocol there.
RFC 5246 mentions the CLOSE_NOTIFY in relation with resumption, that is not possible, if the level is FATAL.
But, yes, RFC 5246 is not explicit about the level. Just to mention, the other libraries are using WARNING.

About mbedtls:
This is using a "MUST NOT" FATAL ALERT/NO_CERTIFICATE_RESERVED (here RFC 5246 is explicit).

[boaks]

@obgm

Ping ... Ping.

Just to mention: even if something may require additional work, that could applied also later in a second step.

[mrdeep1]

About mbedtls:
This is using a "MUST NOT" FATAL ALERT/NO_CERTIFICATE_RESERVED (here RFC 5246 is explicit).

Not sure how you are managing to get this to happen with the libcoap wrapper for Mbed TLS. It is there (if compiled in) in the Mbed TLS library for SSL 3.0. However, the libcoap code is setting a minimum level of TLS 1.2 when selecting the cipher suites as well as when calling mbedtls_ssl_conf_min_version().

[boaks]

Not sure how ...

I use the Californium - Interoperability Test.
That prepares the Californium client to send no certificate, and mbedtls responds with that alert.
That's no bug related to libcoap, it's a minor bug in mbedtls

[mrdeep1]

Thanks

[obgm]

"case in case" → "in case"

[mrdeep1]

Corrected

[mrdeep1]

Updated code pushed (updated Copyright date as well in src/coap_mbedtls.c)

[mrdeep1]

OK - we get a build error -Werror=implicit-fallthrough at line 2108 src/coap_gnutls.c where I actually wanted the code to fall-through. Working on getting it fixed. Could be some others.

[mrdeep1]

Fixed by using /* Fall through */. Also fixed a compile error due to build GnuTLS being less than 3.6.6 (no RPK).

[obgm]

Thanks! Here we go...