Skip to content

Zephyr TLS socket options #7118

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.

Sign up for GitHub

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

Closed
wants to merge 24 commits into from
Closed

Zephyr TLS socket options #7118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
de8465c
net: socket: Define getsockopt() and setsockopt()
pfl Apr 18, 2018
5303cb5
net: socket: Add TLS getsockopt and setsockopt functions
pfl Apr 18, 2018
95207e7
tests: socket: Create getsockopt() and setsockopt() tests
pfl Apr 18, 2018
9e9f822
net: tls: Add build files and initial header file for TLS
pfl Apr 20, 2018
b7dbf47
net: context: Store TLS enabled/disabled value in context
pfl Apr 18, 2018
f0510d3
net: tls: Initialize TLS data structures
pfl Apr 20, 2018
f38bc88
net: pkt: Add flag to net_pkt_clone()
pfl May 14, 2018
2ed2522
net: tls: Connect TLS on listen() and connect()
pfl Apr 20, 2018
9ea8193
net: context: Split packet sending into two functions
pfl Apr 25, 2018
ef6c11c
net: tls: Add mbedTLS packet sending
pfl Apr 25, 2018
8a3131a
net: context: Add TLS receive functionality
pfl May 16, 2018
029eee1
net: tls: Add Kconfig options for default configuration
rlubos May 10, 2018
2c12867
net: socket: Add socket option to select tls credentials
rlubos May 11, 2018
ae911ca
net: tls: Configure tls credentials in mbedtls
rlubos May 15, 2018
d714929
net: tls: Add simple logging
rlubos May 15, 2018
071920a
net: tls: Provide a configurable mbedtls config file
rlubos May 16, 2018
353d667
net: samples: Add TLS support to http_get sample
rlubos May 23, 2018
0af76af
samples: net: Decrease memory consumption for test case
pfl Jun 1, 2018
d9828bf
net: tls: Provide RNG to mbedTLS that actually works
rlubos May 18, 2018
87bef22
net: tls: Handle TCP window size in TLS module
rlubos May 28, 2018
bf8d3f9
net: tls: Separate TLS context from net_context
rlubos May 29, 2018
4474b2e
net: tls: Move default certificate to the example
rlubos May 28, 2018
cf322ec
net: samples: Add TLS support to big_http_download
rlubos May 23, 2018
18e80da
net: ip: Mark socket TLS support experimental
pfl Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/net/loopback.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ static int loopback_send(struct net_if *iface, struct net_pkt *pkt)
* must be dropped. This is very much needed for TCP packets where
* the packet is reference counted in various stages of sending.
*/
cloned = net_pkt_clone(pkt, K_MSEC(100));
cloned = net_pkt_clone(pkt, K_MSEC(100), NET_PKT_CLONE_USER_DATA);
Copy link
Contributor

@pfalcon pfalcon Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have this change in this PR, instead of a separate small, clean PR, easy to review and merge?

Copy link
Member

@jukkar jukkar Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the pattern we are forced to use with github. In order to test the feature, all relevant patches need to be inside one PR (even if they are not fully related). This makes reviewing a bit painful but slightly faster to merge. I am personally trying to get gPTP patches reviewed but it is very slow because I am trying to send patches in smaller chunks which serializes the whole process.

Copy link
Contributor

@pfalcon pfalcon Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the pattern we are forced to use with github.

No, why suddenly? We used to work with Gerrit, which required a review per commit, and everything was "fine".

In order to test the feature, all relevant patches need to be inside one PR (even if they are not fully related).

I'm not sure what you mean by "test". If you effectively mean "try/review", then splitting to another PR only makes it easier. And if you mean "test" literally, then we have unittest to check for regressions (and ideally, new tests should be added for new functionality, which we're behind of doing, but still easier to do in a small focused PR).

This makes reviewing a bit painful but slightly faster to merge.

It's hardly possible to review such PRs thoroughly. How it worked before is:

  • Not making thorough reviews
  • Ignoring comments and concerns raised

So no, it doesn't make merging faster, but definitely makes it lower quality.

I am personally trying to get gPTP patches reviewed but it is very slow because I am trying to send patches in smaller chunks which serializes the whole process.

I don't think it's related. It's because of overall complexity of the matter, and that not everyone is familiar with the requirements for this protocol implementation in Zephyr, or with protocol itself. For example, I feel a bit shy to give +1 for it. (But then I'm sure there's no -1 at least in the initial patch I looked it, thanks to it being contained and easy to review).

if (!cloned) {
res = -ENOMEM;
goto out;
Expand Down
1 change: 1 addition & 0 deletions ext/lib/crypto/mbedtls/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ config MBEDTLS_CFG_FILE
string "mbed TLS configuration file"
depends on MBEDTLS_BUILTIN
default "config-mini-tls1_2.h"
default "config-tls-generic.h" if NET_TLS || NET_DTLS
Copy link
Contributor

@pfalcon pfalcon Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good pattern to follow. If it's truly "config-tls-generic", then likely it should be made the default. Configs of existing mbedTLS-related samples can be updated to refer to "config-mini-tls1_2.h", if they can't be proven to work with new "config-tls-generic.h" quickly/easily.

In other words, please try to improve the way mbedTLS is used/configured in Zephyr as a whole, not just add another adhoc way to configure it specifically for "NET_TLS || NET_DTLS". We have a ticket for that too: #6132 .

help
Use a specific mbed TLS configuration file. The default is suitable to
communicate with majority of HTTPS servers on the Internet, but has
Expand Down
270 changes: 270 additions & 0 deletions ext/lib/crypto/mbedtls/configs/config-tls-generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,270 @@
/*
* Copyright (C) 2006-2015, ARM Limited, All Rights Reserved
* Copyright (c) 2017 Intel Corporation.
* Copyright (c) 2018 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*
* Generic configuration for TLS, manageable by Kconfig.
*/

#ifndef MBEDTLS_CONFIG_H
#define MBEDTLS_CONFIG_H

/* System support */
#define MBEDTLS_PLATFORM_C
#define MBEDTLS_PLATFORM_MEMORY
#define MBEDTLS_MEMORY_BUFFER_ALLOC_C
#define MBEDTLS_PLATFORM_NO_STD_FUNCTIONS
#define MBEDTLS_PLATFORM_EXIT_ALT
#define MBEDTLS_NO_PLATFORM_ENTROPY
#define MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES
#define MBEDTLS_PLATFORM_PRINTF_ALT

#if !defined(CONFIG_ARM)
#define MBEDTLS_HAVE_ASM
#endif

#if defined(CONFIG_MBEDTLS_TEST)
#define MBEDTLS_SELF_TEST
#define MBEDTLS_DEBUG_C
#endif

/* mbedTLS feature support */

/* Supported TLS versions */
#if defined(CONFIG_NET_TLS)
#if defined(CONFIG_TLS_VERSION_1_0)
#define MBEDTLS_SSL_PROTO_TLS1
#endif

#if defined(CONFIG_TLS_VERSION_1_1)
#define MBEDTLS_SSL_PROTO_TLS1_1
#endif

#if defined(CONFIG_TLS_VERSION_1_2)
#define MBEDTLS_SSL_PROTO_TLS1_2
#endif

/* Module required for TLS */
#define MBEDTLS_SSL_TLS_C
#define MBEDTLS_SSL_SRV_C
#define MBEDTLS_SSL_CLI_C
#define MBEDTLS_CIPHER_C
#define MBEDTLS_MD_C

/* TODO Perhaps make this configurable? */
#define MBEDTLS_SSL_MAX_FRAGMENT_LENGTH
Copy link
Contributor

@pfalcon pfalcon Jun 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review comment thread is result of confusion on my side, please ignore

/* TODO Perhaps make this configurable? */

Of course make this configurable, like it's already done in Zephyr mainline: 7558ce8

Copy link
Contributor

@rlubos rlubos Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you refer to MBEDTLS_SSL_MAX_CONTENT_LEN here? MBEDTLS_SSL_MAX_FRAGMENT_LENGTH is just hardcoded in some of the config files.

Copy link
Contributor

@pfalcon pfalcon Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I refer to.

MBEDTLS_SSL_MAX_FRAGMENT_LENGTH is just hardcoded in some of the config files.

That's because there're too many config files, and a bit hard to update them consistently (without risking regressions). However, as the linked PR shows, there's a Kconfig option in Zephyr to set TLS fragment size, and all newer configs should use it.

Copy link
Contributor

@rlubos rlubos Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are two different configs, though they look pretty similar:
MBEDTLS_SSL_MAX_FRAGMENT_LENGTH
MBEDTLS_SSL_MAX_CONTENT_LEN

TODO in the code refers to MBEDTLS_SSL_MAX_FRAGMENT_LENGTH, which enables max_fragment_length TLS extension, I saw it was present in some of the tls-related config files, yet I've left a remainder to investigate the use-case of it in future.

MBEDTLS_SSL_MAX_CONTENT_LEN is configured with the option from Kconfig, just as in the PR posted (at the very bottom of config-tls-generic.h file).

Copy link
Contributor

@pfalcon pfalcon Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh, sorry for this noise, indeed, I mixed up these two options :-(

Regarding fragment length extension, I'd say it makes sense to enable it by default (like it's done), yet there're too few real-world sites with which it works :-(.

Copy link
Contributor

@rlubos rlubos Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yet there're too few real-world sites with which it works :-(.

Yep, I've faced that already :/

#endif /* CONFIG_NET_TLS */

#if defined(CONFIG_NET_DTLS)
#define MBEDTLS_SSL_PROTO_DTLS
#define MBEDTLS_SSL_DTLS_ANTI_REPLAY
#define MBEDTLS_SSL_DTLS_HELLO_VERIFY
#define MBEDTLS_SSL_COOKIE_C
#endif

/* Supported key exchange methods */
#if defined(CONFIG_TLS_KEY_EXCHANGE_PSK_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_PSK_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_DHE_PSK_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_RSA_PSK_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_RSA_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_RSA_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_DHE_RSA_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_ECDH_RSA_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED
#endif

#if defined(CONFIG_TLS_KEY_EXCHANGE_ECJPAKE_ENABLED)
#define MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED
#endif

/* Supported cipher modes */
#if defined(CONFIG_TLS_CIPHER_AES_ENABLED)
#define MBEDTLS_AES_C
#endif

#if defined(CONFIG_TLS_CIPHER_CAMELLIA_ENABLED)
#define MBEDTLS_CAMELLIA_C
#endif

#if defined(CONFIG_TLS_CIPHER_DES_ENABLED)
#define MBEDTLS_DES_C
#endif

#if defined(CONFIG_TLS_CIPHER_CCM_ENABLED)
#define MBEDTLS_CCM_C
#endif

#if defined(CONFIG_TLS_CIPHER_CBC_ENABLED)
#define MBEDTLS_CIPHER_MODE_CBC
#endif

/* Supported message authentication methods */

#if defined(CONFIG_TLS_MAC_MD5_ENABLED)
#define MBEDTLS_MD5_C
#endif

#if defined(CONFIG_TLS_MAC_SHA1_ENABLED)
#define MBEDTLS_SHA1_C
#endif

#if defined(CONFIG_TLS_MAC_SHA256_ENABLED)
#define MBEDTLS_SHA256_C
#endif

#if defined(CONFIG_TLS_MAC_SHA512_ENABLED)
#define MBEDTLS_SHA512_C
#endif

/* Automatic dependencies */

#if defined (MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED)
#define MBEDTLS_DHM_C
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED)
#define MBEDTLS_ECDH_C
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED)
#define MBEDTLS_RSA_C
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED)
#define MBEDTLS_PKCS1_V15
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED) || \
defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED)
#define MBEDTLS_X509_CRT_PARSE_C
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED)
#define MBEDTLS_ECDSA_C
#endif

#if defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED)
#define MBEDTLS_ECJPAKE_C
#endif

#if defined(MBEDTLS_ECDH_C) || \
defined(MBEDTLS_ECDSA_C) || \
defined(MBEDTLS_ECJPAKE_C)
#define MBEDTLS_ECP_C
/* For now use single specific curve, can be configurable in the future */
#define MBEDTLS_ECP_DP_SECP256R1_ENABLED
#endif

#if defined(MBEDTLS_X509_CRT_PARSE_C)
#define MBEDTLS_X509_USE_C
#endif

#if defined(MBEDTLS_X509_USE_C) || \
defined(MBEDTLS_ECDSA_C)
#define MBEDTLS_ASN1_PARSE_C
#endif

#if defined(MBEDTLS_ECDSA_C)
#define MBEDTLS_ASN1_WRITE_C
#endif

#if defined(MBEDTLS_DHM_C) || \
defined(MBEDTLS_ECP_C) || \
defined(MBEDTLS_RSA_C) || \
defined(MBEDTLS_X509_USE_C)
#define MBEDTLS_BIGNUM_C
#endif

#if defined(MBEDTLS_RSA_C) || \
defined(MBEDTLS_X509_USE_C)
#define MBEDTLS_OID_C
#endif

#if defined(MBEDTLS_X509_USE_C)
#define MBEDTLS_PK_PARSE_C
#endif

#if defined(MBEDTLS_PK_PARSE_C)
#define MBEDTLS_PK_C
#endif

/* mbedTLS modules */
#if defined (CONFIG_NET_TLS_PEM_CERTIFICATE_FORMAT) && \
defined(MBEDTLS_X509_CRT_PARSE_C)
#define MBEDTLS_PEM_PARSE_C
#define MBEDTLS_BASE64_C
#endif

#if defined(MBEDTLS_AES_C)
#define MBEDTLS_CTR_DRBG_C
#endif

#if defined(MBEDTLS_SHA256_C) || defined(MBEDTLS_SHA512_C)
#define MBEDTLS_ENTROPY_C
#endif

/* For test certificates */
#if defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECDSA_C)
#define MBEDTLS_CERTS_C
#endif

#if defined(CONFIG_MBEDTLS_DEBUG)
#define MBEDTLS_ERROR_C
#define MBEDTLS_DEBUG_C
#define MBEDTLS_SSL_DEBUG_ALL
#define MBEDTLS_SSL_ALL_ALERT_MESSAGES
#endif

#define MBEDTLS_SSL_MAX_CONTENT_LEN CONFIG_MBEDTLS_SSL_MAX_CONTENT_LEN

#include "mbedtls/check_config.h"

#endif /* MBEDTLS_CONFIG_H */
11 changes: 10 additions & 1 deletion include/net/net_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ struct net_tcp;

struct net_conn_handle;

struct net_tls;

/**
* Note that we do not store the actual source IP address in the context
* because the address is already be set in the network interface struct.
Expand Down Expand Up @@ -276,6 +278,11 @@ struct net_context {
struct k_fifo accept_q;
};
#endif /* CONFIG_NET_SOCKETS */

#if defined(CONFIG_NET_TLS) || defined(CONFIG_NET_DTLS)
/** TLS context information */
struct net_tls *tls;
#endif /* CONFIG_NET_TLS || CONFIG_NET_DTLS */
};

static inline bool net_context_is_used(struct net_context *context)
Expand Down Expand Up @@ -778,7 +785,9 @@ int net_context_update_recv_wnd(struct net_context *context,
s32_t delta);

enum net_context_option {
NET_OPT_PRIORITY = 1,
NET_OPT_PRIORITY = 1,
NET_OPT_TLS_ENABLE = 2,
NET_OPT_TLS_SEC_TAG_LIST = 3,
};

/**
Expand Down
3 changes: 3 additions & 0 deletions include/net/net_ip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ enum net_ip_protocol {
IPPROTO_TCP = 6,
IPPROTO_UDP = 17,
IPPROTO_ICMPV6 = 58,
IPPROTO_TLS_1_0 = 256,
Copy link
Member

@jukkar jukkar Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we support TLS 1.0 at all, I though it was kind of obsolete already?

Copy link
Contributor Author

@pfl pfl Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps not anymore, as there is a suggested migration from TLS 1.0 to TLS 1.1 or higher before June 30, 2018, see https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_1.0

Copy link
Contributor

@rlubos rlubos Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I've included it just for sake of completness, as it is supported by mbedTLS.

Anyway, at this point different values of TLS versions here are more like placeholder, as mbedTLS configuration of which TLS version is supported is compile-time, and it's done in Kconfig. Yet I can imagine that different TLS implementations might enable to select it dynamically.

IPPROTO_TLS_1_1 = 257,
IPPROTO_TLS_1_2 = 258,
};

/** Socket type */
Expand Down
13 changes: 12 additions & 1 deletion include/net/net_pkt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1543,15 +1543,26 @@ struct net_buf *net_frag_get_pos(struct net_pkt *pkt,
u16_t offset,
u16_t *pos);

/** Flags for pkt clone operation
*/
/** Clone pkt metadata only */
#define NET_PKT_CLONE_META 0x0
/** Clone pkt headers */
#define NET_PKT_CLONE_HDR 0x1
/** Clone headers and application data */
#define NET_PKT_CLONE_USER_DATA 0x2

/**
* @brief Clone pkt and its fragment chain.
*
* @param pkt Original pkt to be cloned
* @param timeout Timeout to wait for free net_buf
* @param flags Flags to indicate which parts of the pkt to clone
*
* @return NULL if error, clone fragment chain otherwise.
*/
struct net_pkt *net_pkt_clone(struct net_pkt *pkt, s32_t timeout);
struct net_pkt *net_pkt_clone(struct net_pkt *pkt, s32_t timeout,
int flags);

/**
* @brief Get information about predefined RX, TX and DATA pools.
Expand Down
Loading