net: tls: Implement TLS socket options

[rlubos]

This PR is a follow up of #7118

It adds a set of TLS socket options that can be used for TLS connection configuration:

• TLS_SEC_TAG_LIST for setting TLS credentials
• TLS_HOSTNAME for setting hostname for peer verification
• TLS_CIPHERSUITE_LIST for setting ciphersuites allowed on a socket
• TLS_CIPHERSUITE_USED for reading ciphersuite selected during handshake
• TLS_PEER_VERIFY for setting peer verification level

For TLS credential configuration a simple network lib was added.

Socket options were tested with http_get, big_http_download (they now verify certificates) and socket echo_client/echo_server (these two can be tested with net-tools utils, as they use the same credentials as their net_app counterparts, or simply client vs server).

With this PR, #7118 is pretty much covered (some test would be needed for new code though).

[codecov-io]

Codecov Report

Merging #9007 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #9007      +/-   ##
==========================================
- Coverage   52.45%   52.43%   -0.02%     
==========================================
  Files         200      200              
  Lines       25121    25127       +6     
  Branches     5244     5246       +2     
==========================================
  Hits        13176    13176              
- Misses       9823     9829       +6     
  Partials     2122     2122

Impacted Files	Coverage Δ
include/net/socket.h	100% <ø> (ø)	⬆️
subsys/net/lib/sockets/sockets.c	42.06% <0%> (-1.03%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ba0ddf...b49691d. Read the comment docs.

[pfalcon]

It adds a set of TLS socket options that can be used for TLS connection configuration:

For TLS credential configuration a simple network lib was added.

I yet need to look thru the individual commit messages. But I would like to suggest to clearly emphasize that TLS configuration of this subsys is 2-stage process:

1. "System-wide" credential pool is set, using a special API. (This also can be done "declaratively" using Kconfig options for common case(s)).
2. Individual credentials and other TLS params are set on a socket using setsockopt().

(Yes, my suggestion is to state that explicitly (as 1. 2.) in a commit message, and in the docs, which I'm sure will follow.)

[pfalcon]

TLS_CIPHERSUITE_LIST for setting ciphersuites allowed on a socket

I appreciate implementing other setsockopt() params beyond just credential setting, should give a clear picture how all that supposed to work and set the right rails for future extension.

[pfalcon]

Socket options were tested with http_get, big_http_download (they now verify certificates) and socket echo_client/echo_server

As one of the next steps, you may want to look into "native" samples/net/sockets/echo (should be trivial), and samples/net/sockets/echo_async, which should be a real interesting case, showing whether your API is async ready. Mind that Zstream worked with it without any special "magic". (Because it kept different layers separate; the only rule of thumb to follow there was: poll'ing happens on the level of the underlying sockets; after that, calling a (TLS, or whatever) wrapper may still return EAGAIN).

After that, we'll be more ready to revisit @d3zd3z's concurrency concerns, hopefully backed by some code from him (which he currently works on).

[pfalcon]

@rlubos , so this would be the commit whose message can be updated with the description of 2-stage credential management as suggested in #9007 (comment)

[pfalcon]

I'm sure it should be "a reference to ..." (instead of "for").

[pfalcon]

s/have/be/.

[pfalcon]

I said it in #7118, and think it's worth repeating here: I'd explicitly designated an error code for "access denied", to give users an expectation that credentials can be expected to be read back easily. (And yes, as it's security stuff, I think it's worth to do it right away.)

[pfalcon]

Btw, sooner or later someone will think "Is that subsys is for TLS credentials, or security credentials in general?". But I guess, it's too early for that.

[pfalcon]

Repeating these checks in each individual opt handler is a bit boring. Why not do that in tls setsockopt dispatcher?

[pfalcon]

Maybe then there's no need for getsockopt() of it at all?

[jukkar]

Looks quite sane thing to provide getter also for hostname.

[rlubos]

Acutally I don't have a use case for it (didn't use a getter even once, beside testing). I think we're fine skipping it to save some flash - if there's a need for it, implementation is pretty straigtforward.

[pfalcon]

"its"

[pfalcon]

This looks like confusion/mixing up different concerns. The purpose of mbedtls_ssl_set_hostname() is to enable use of SNI (TLS VHOST). If mbedtls_ssl_set_hostname() is not called, everything should work normally, except that no SNI extension should be sent. Forcible setting it to empty string to trigger some "hostname verification" seems wrong.

[pfalcon]

Is this right return code?

[rlubos]

Well I'm not sure about it, because on one hand we have (from getsockopt man page):

If the size of the option value is greater than option_len, the value stored in the object pointed to by the option_value argument shall be silently truncated.

I guess this is kind of situation we have here, and by silently what I understand is to not raise an error.

On the other hand, passing 0 as a buffer length does not sound correct, so EINVAL would be another candidate here.

[pfl]

I'd say -EINVAL whenever the option length is not what is expected by a socket function.

[jukkar]

I agree with @pfl, returning 0 would be quite confusing here.

[pfalcon]

This strlen() + 1 looks a bit weird, I'm sure will be source of common mistake ;-)

[pfalcon]

Does it make sense to say "Let’s Encrypt Authority X3 for https://www.7-zip.org"?

[pfalcon]

Ditto, "... for https://google.com"

[pfalcon]

Maybe TLS_CREDENTIAL_SERVER_CERTIFICATE would be a better name for this after all?

[pfalcon]

Well, now we can come back to my comment "why bother to port that legacy code instead of writing normal socket app from scratch?". It related exactly to chunks like this - they don't appear in a normal socket app, and you had to comment them out adding TLS stream support.

[rlubos]

To be honest I'm not happy with this part of code either and would be very happy to get rid of this - it's more like a workaround code than an actual value to the sample.

The problem is that current socket shim layer, even if we provide buffer large enough to hold entire message to the recv function, will pass it chunk by chunk to the application (or rather net_buf by net_buf). I know it's a stream and this behaviour is correct, still, it's unnecessary overhead. And it doesn't matter whether an app is a port of some legacy code, or a "pure" socket application - the simple echo sample behaves the same way.

Maybe I'm a little bit oversensitive here, but I still have Thread somewhere in a back of my head, and hopefully one day will have some time to verify socket apps with OpenThread. And there, such a fragmentation be a huge impact to the throughput.

[pfalcon]

This looks mostly good. There're minor comments here and there, mostly about code comments and better documenting stuff.

A bigger question is defining and documenting semantics of TLS_HOSTNAME. As mentioned, in my list, its purpose is for allowing TLS VHOST working. If there's also some "host verification" happening, then it has both purposes, and this fact should be explicitly documented. And that effectively makes it mandatory option, which should be always set for a TLS socket, this fact should be made explicit too. But I'm not sure it should be artificially force empty hostname.

[pfalcon]

Another note for getsockopt() counterparts for each and every setsockopt() call: I doubt there're good usecases for each of them. And due to code structure, unused options will still be included in the binary code. I might suggest to implement just one option as an example, and wait for actual usecases to implement more. (TLS_CIPHERSUITE_USED on the other hand good. But it's display name of ciphersuite as far as I understood, that creates inconsistency with TLS_CIPHERSUITE_LIST).

[rlubos]

@pfalcon Thank you for the review.

Let's start with that I've followed you suggestion regarding adding TLS to echo and echo_async samples. The first one went smooth as expected. With the latter I've identified two issues:

• I had realized that TLS poll does not work entirely as expected - it behaves in a very way you've described it - data availability on a socket does not mean that data will be available on mbedTLS, therefore consecutive call to read may return EAGAIN. That's something that has to be adressed in a separate PR, I'll take care of it later.
• echo_async crashes after several (well, two actually) connection attempts, but it's not related to the TLS - it's the same on the vanilla sample. I'll create an issue for that.

Otherwise echo_async seemed to work well. No magic in the code was needed either (well, except the first issue described): rlubos@233db59.

As for the review and TLS_HOSTNAME, I've had an impression from feedback in #7118 that it should be verified always, yet it might be my misunderstanding. Anyway, there's no push from my side to keep this change, I'll revert behaviour to mbedTLS default.

Regarding option getters, indeed not all may be needed. I'd keep though getters for TLS_CIPHERSUITE_LIST and TLS_SEC_TAG_LIST. The first one is simply an easy way to get all ciphersuites that are available for a socket, which may be convenient, the latter because I know something like this is already used by my collegaues. TLS_CIPHERSUITE_USED will obviously stay as it's read only, but I'll allign it with TLS_CIPHERSUITE_LIST to return an integer instead of string.

Otherwise I'm fine with the changes. Any other remarks, I'll address inline in the review comments. Will update the PR soon hopefully.

[pfl]

Let's go forward with the above. The functions were marked as experimetal, so we can still adjust as needed? Thanks for the hard work so far!

[rlubos]

@pfl I guess I will be able to push fixes by tommorow, there's not that much work with them

[pfalcon]

I had realized that TLS poll does not work entirely as expected - it behaves in a very way you've described it - data availability on a socket does not mean that data will be available on mbedTLS, therefore consecutive call to read may return EAGAIN. That's something that has to be adressed in a separate PR, I'll take care of it later.

Yeah, as I mentioned in my final review of #8814 (review) , I wasn't sure about poll() implementation there - whether it covered all possible cases or whether it had to cover all those special cases at all vs just possibly relaxing API contract (letting poll() return "readable"/"writable", but the corresponding call return EAGAIN).

For "TLS is a separate layer", the latter approach makes sense. I'll leave it up to you to decide what makes sense for the "under socket layer" approach. One thing I found useful in reasoning about this matters, though, is not to treat TLS as the only/special case. Rather, there can be seen a pattern, where there's a "black box" which performs additional processing - and "delaying" (in the sense that some input data from socket may be buffered and not immediately lead to output from "black box") - of data received from an async socket. Examples of this are TLS, websocket, etc. A good solution would work with any kind of such a "black box" (and hopefully without taking much care about "black box" internal on the poll side).

[pfalcon]

As for the review and TLS_HOSTNAME, I've had an impression from feedback in #7118 that it should be verified always, yet it might be my misunderstanding. Anyway, there's no push from my side to keep this change, I'll revert behaviour to mbedTLS default.

Well I guess, this point, and this PR overall, really could use review/ack from @d3zd3z. My treatment of TLS lies more in the vein of "communication protocol" rather than "security protocol", so I may be missing something too. (I'm going to take a deeper look what mbedTLS does with mbedtls_ssl_set_hostname()).

[pfalcon]

I'd keep though getters for TLS_CIPHERSUITE_LIST and TLS_SEC_TAG_LIST.

Sounds good. Thanks for considering this matter.

[jukkar]

There is min() macro already defined in misc/util.h, please use that instead.

[jukkar]

This case branch does not need {}

[jukkar]

Don't we need to unlock the credentials before returning?

[rlubos]

Good point, I've completely messed up here, will fix!

[jukkar]

Looks quite sane thing to provide getter also for hostname.

[jukkar]

Should we also prevent buffer length 1 here as it would make not much sense because we place \0 at the end?

[jukkar]

If *optlen is 1, then we return also 1 but the string will be empty, which is quite confusing.

[rlubos]

I think it makes sense, we return empty string which binary size is 1 byte. I'm removing this code anyway, so no use arguing here.

[jukkar]

Please put empty line before comment, it looks too crowded now.

[jukkar]

Same thing here as with hostname, having *optlen as 1 would return empty string which is quite useless imho.

[rlubos]

This option will return integer, to align with TLS_CIPHERSUITE_LIST.

[jukkar]

Could we have the pem file as a file and just emded it here by including it. So similar way as what is done for example in rpl_border_router sample.

[rlubos]

Sounds reasonable, I can give a try. I guess you ment der file though? We've agreed in review of #7118 to use DER cert format to save some code size.

[jukkar]

I guess you ment der file though? We've agreed in review of #7118 to use DER cert format to save some code size.

Yes, I meant the DER file.

[jukkar]

Same comment as I had with echo-client/server sample, please place the cert into a file and just include it here. There is the generate_inc_file_for_target() CMake macro for this.

[rlubos]

@jukkar Thank you for the review.

I'll apply fixes and push them in a single batch with those already applied.

[rlubos]

Pushed updated branch with fixes, I hope I didn't miss anything.

[pfalcon]

s/and/a/.

[rlubos]

Fixed.

[pfalcon]

Well I guess, this point, and this PR overall, really could use review/ack from @d3zd3z. My treatment of TLS lies more in the vein of "communication protocol" rather than "security protocol", so I may be missing something too. (I'm going to take a deeper look what mbedTLS does with mbedtls_ssl_set_hostname()).

And it's actually right on the surface: https://github.com/ARMmbed/mbedtls/blob/7d728bd70e6a8d129ee07813da1538de1bf95cb2/include/mbedtls/ssl.h#L2158

Set or reset the hostname to check against the received
server certificate. It sets the ServerName TLS extension,
too, if that extension is enabled. (client-side only)

So, sorry for the hassle caused. As I mentioned, I mostly have been paying attention to communicative and integrational aspects of TLS, and didn't give good thinking to security aspects, and well, now I should. And I guess what you had in the original code is actually the best way to deal with it. Actually, I don't remember exactly what was there (and I see you reworked the code already), so we have a chance to go over over it more explicitly together: I think following would be the good way (and again, likely what you had originally):

1. On the TLS socket creation, call mbedtls_ssl_set_hostname(mbedtls_ssl_context *ssl, ""). This will ensure that if user "forgets" to call setsockopt(TLS_HOSTNAME, real_value), then certificate validation will fail.
2. However, user still should be able to call setsockopt(TLS_HOSTNAME, NULL) to explicitly disable peer (usually server) name checking. Actually, it's worth thinking how this validation would work on client vs server (listening) sockets. As client certificates aren't common in the "default" TLS deployment (specifically, real-world HTTPS usage), them maybe mbedtls_ssl_set_hostname(mbedtls_ssl_context *ssl, "") should be called only for client sockets?

The rationale for conservatively forcing (server) hostname validation is that without validating it, we can be easily connecting and talking to wrong server. E.g., if we want to talk to foo.com, but don't check its name in the cert, we can end up talking to bar.com, whose cert is issued by the same CA. We especially should "force" this check, because in deeply embedded environments, there're limited means to validate certs at all. For example, real time (as in: wall clock) may be not available, so we might not be able to check cert validity dates. Then we really should cling to checks that we can do, and enable them by default (so, again, if user forgets to provide data needed for such validations, then it'll fail rather than will be skipped). I hope @d3zd3z would approve of such rationale.

The PR was updated.

[pfalcon]

@rlubos, Thanks for addressing review comments. I'd like to have another look thru the entire PR after the weekend, but I guess it's fine by me. I however think it might make sense to let few extra days to give a chance to @d3zd3z to look thru it too. Due to this, I don't give +1 right away, but as I'm going on vacation on Wed, dismissed by old -1. Thanks for your work.

[rlubos]

@pfalcon I'll restore this feature with proposed improvements then (I agree that it makes sense to enforce it for clients by default and give an user an option to disable hostname verification). Thanks for looking into this and for detailed description.

I however think it might make sense to let few extra days to give a chance to @d3zd3z to look thru it too.

I'm ok with that, guess that a few more days won't hurt us.

[rlubos]

Pushed hostname option changes.

[jukkar]

recheck

[jukkar]

Minor nitpicks

[jukkar]

Even thou it makes no difference here, I would recommend that there is a break; statement also in the default branch. It is a good habit to do and avoids errors if new cases are added later.

[jukkar]

same here

[jukkar]

Why -1 here but not in psk->len?

[rlubos]

psk_identity_len is expected to be a length of a string, while psk_len is size of the key (binary). Credential management subsys treats both as a binary, therefore PSK_id len field counts the NULL character.
At least that's behaviour expected from what I saw in other samples (they use sizeof(psk_id) - 1 or strlen(psk_id)), mbedTLS API docs are actually pretty vauge here.

[jukkar]

Please remove extra empty line

[jukkar]

we usually do "if (!ciph)" in zephyr

[nashif]

actually we should avoid (!ciph) and keep it as the original. We will have to go through the code and change how it is done so far (MISRA-C: STMT.COND.NOT_BOOLEAN)

[rlubos]

@jukkar Pushed fixes for the nits. I didn't touch ciph == NULL though, as suggested by nashif

[jukkar]

LGTM