-
Notifications
You must be signed in to change notification settings - Fork 423
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
PKI: Make (D)TLS operation consistent across all TLS libraries #561
Conversation
Code updated and pushed to update help/man documentation - triggered by #563 conversation. |
rebased and updated code pushed |
rebased and updated code pushed |
commit 9b39bd9
I built the examples with openssl to test it. On my build, PSK is broken for coap-client, and working for coap-server.
|
examples/client.c
Outdated
"\t \t\twere signed by a common CA (used for mutual\n" | ||
"\t \t\tauthentication)\n" | ||
"\t-C cafile\tPEM file or PKCS11 URI for the CA certificate that was\n" | ||
"\t \t\tused to sign the client certfile. The contents of cafile\n" |
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.
On the client side, isn't it the "server certificate", which is intended to be signed by the clients CA?
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.
Comment corrected
The PSK client issue seems to be introduced before this PR. client.c
must be
otherwise the pointer will be set to zero. I open a new issue. |
Code pushed, including a My regression tests (which include valgrind) had not found this as I usually do |
I use
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:
|
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? |
Found the DN sending issue in OpenSSL i/f and code fix pushed for testing. |
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. |
Fixed common CA issue in OpenSSL and updated -R documentation. Code pushed. |
"\t \t\tThis is a file that contains one or more lines of Identity\n" | ||
"\t \t\tHints to match for new user and new pre-shared key\n" | ||
"\t \t\t(PSK) (comma separated) to be used. E.g., per line\n" | ||
"\t \t\tThis is a file that contains one or more lines of\n" |
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.
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?
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 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.
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.
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?
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 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.
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 the -h option is not used, then the -k and -u options are used for the key and identity.
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'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?
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.
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.
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.
Documentation updated to not use new and additional clarification on the server side. Code pushed for review.
A build using tinydtls also shows the documentation for x509 support. Just as idea/extension: is it possible, to load trusted public keys for RPK, e.g. by |
It is not (easily) possible to do this in the man pages - hence
Not at present. |
I ment the text, displayed with argument -h. And -R is also accepted without any warning. |
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. |
Maybe a build-variant hint in a footline with "tinyDTLS only supports RPK, not x.509", or "openssl only supports x.509, not RPK"? |
Issue should now be fixed and subsequent alert non longer sent. Code pushed. |
Also on my side ;-). Great! |
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 Code changes pushed If is_rpk_not_cert is set, then all certificate validation is ignored. |
@boaks Apologizes, but I do not have merge capabilities. I have just rebased and done a push to clear some examples conflicts. |
I am doing a review within the next couple of days. Sorry for the delay. |
That sound great! |
@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? |
About CLOSE_NOTIFY: About mbedtls: |
Ping ... Ping. Just to mention: even if something may require additional work, that could applied also later in a second step. |
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(). |
I use the Californium - Interoperability Test. |
Thanks |
examples/client.c
Outdated
"\t \t\tcase\n" | ||
"\t-k key \t\tPre-shared key for the specified user\n" | ||
"\t-u user\t\tUser identity for pre-shared key mode\n" | ||
"\t \t\tcase in case there is no match\n" |
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.
"case in case" → "in case"
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.
Corrected
Updated code pushed (updated Copyright date as well in src/coap_mbedtls.c) |
OK - we get a build error |
83cadb8
to
699e83c
Compare
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.
Fixed by using |
Thanks! Here we go... |
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