Implement client side identity-hint callback

[peckto]

Description

This pull request implements the client side of the identity-hint (TLS server identity) callback.
With the identity-hint, the TLS server can hint the client to choose the right PSK.
The current code base is already prepared for the identity-hint, so only small changes are necessary.
The implementation and usage is analog to the handling of the client hint on the server side.

Note: This is only the client side implementation of the identity-hint, the server side of mbedTLS is still not capable of sending the hint.

Status

READY

Requires Backporting

No backporting required.

Migrations

mbedtls_ssl_conf_psk_cb can now be used on the client side to receive the server identity-hint.

Additional comments

The server identity is a TLS 1.2 only feature and is changed in TLS 1.3.

Todos

• Tests
• Documentation
• Changelog updated

Steps to test or reproduce

1. Install callback function to receive the identity-hint on the client with mbedtls_ssl_conf_psk_cb.
2. Use a TLS server with server identity-hint support to send the hint to the client.

[gabor-mezei-arm]

The CI is failing with:

Trailing whitespace:
library/ssl_cli.c: 846, 3639

Tabs present:
library/ssl_cli.c: 3640

Please fix these formatting issues and please commit with the --signoff option.

[gilles-peskine-arm]

Thank you for doing the implementation. We are unlikely to accept it without tests, however. Can you please add the requisite support in programs/ssl/ssl_client2.c (similar to what is in ssl_server2) and test cases to tests/ssl-opt.sh, which will either require implementing the server side or interoperability testing with OpenSSL or GnuTLS?

[peckto]

Support to programs/ssl/ssl_client2 was added.
Test case with OpenSSL was added to tests/ssl-opt.sh.

[mpg]

Thanks for your contribution! This is looking pretty good so far, but I think there are a few things left to address:

1. In terms of testing, currently the patch doesn't actually exercise the new feature in ssl_client2.c. I think what we want is a new command-line option for the program that causes it to set the PSK dynamically via the callback instead of setting it statically as usual. Perhaps something like psk_callback=%%d (default: 0). Also, ideally the callback should match if the server identity hint matches the locally configured identity, and return an error if it doesn't - that way we can test the behavior of the library when the callback fails, which we should do in ssl-opt.sh.

2. The documentation of mbedtls_ssl_conf_psk_cb() in include/mbedtls/ssl.h needs to be updated, currently it still starts with "Set the PSK callback (server-side only)." this line and the following paragraph need to be adjusted now that you've lifted this restriction.

3. The new feature should be briefly described in a ChangeLog entry - see ChangeLog.d/00README.md for how to add one.

[mpg]

Thanks for implemented the test as requested. I think this is progressing very well. I'll comment below on your questions about setting the client identity.

[mpg]

Both functions can be used inside the callback, but mbedtls_ssl_set_hs_psk would possibly be better choice.

Actually mbedtls_ssl_set_hs_psk() is the only choice in the general case: we're not allowed to modify the ssl conf once it's associated with a context, as documented here. The reason is, in multi-threaded environments, you want to be able to share your SSL conf between threads, so to avoid race conditions we should treat it as immutable after it's been set initially.

The problem in this case is, that there is currently no possibility to configure the client identity independently of the psk, both is done by mbedtls_ssl_conf_psk.

Ah, that's a good point. (By the way, that's one of the reasons I wanted to test actually setting the key in the callback: before you did, we didn't notice this.) Considering my previous remark about not modifying the SSL conf inside a callback, I think the only clean option here is to add a new function mbedtls_ssl_set_hs_psk_identity(), backed my new members psk_identity and psk_identity_len in the ssl_handshake structure, then making sure ssl_write_client_key_exchange() uses them instead of the ones in ssl->conf when they're set, probably using a helper function similar to mbedtls_ssl_get_psk().

I hope that makes sense, please don't hesitate if you have more questions or want to clarify the strategy before implementing it.

[mpg]

Hi @peckto ! I just wanted to check how things are going with this PR. If you need any support from us in order to progress it, feel free to ask.

[peckto]

If I understood you right, you want to have two possibilities to store the client identity:

1. static in the ssl_context
2. dynamic in the ssl_handshake

In this case, the callback could store the identity inside the ssl_handshake struct.

I'm not sure if we have the requirement to set the client identity dynamically inside the callback at all.
I would expect the client identity to be dependent on the client (thus static for every ssl_context) and not dependent on the server.
Only the psk depends on the server. In this case I see the requirement for a static and dynamic method.
What do you think?

[mpg]

Hi @peckto and sorry for not replying sooner, especially after I asked how this was going; we have a release coming soon and I've been busy making sure fixes that need to be in were ready, and each time I came to this PR, I found that I needed to think about it more...

Just to recap the context: In general, there's 3 ways a particular SSL setting can be configured (in order of precedence):

1. per handshake, using a mbedtls_ssl_set_hs_xxx() function inside a callback during the handshake
2. per context, using a mbedtls_ssl_set_xxx() function before starting the handshake
3. shared config, using a mbedtls_ssl_conf_xxx() function before any handshake

Regarding PSK (disregarding opaque PSKs for a moment), we currently have the following functions:

• mbedtls_ssl_conf_psk(), category 3 above, allows setting the key and identity, and currently forces the caller to provide both.
• mbedtls_ssl_set_hs_psk(), category 1 above, allows setting the key only.

I think I misunderstood a previous comment of yours as implying that you'd like to overwrite the identify in the callback, but your last comment made it cleat that it's actually the opposite: you'd like to set the identify globally, and set the key only in the callback. I think the existing functions allow that, but currently in a suboptimal way:

• set the identify globally using mbedtls_ssl_conf_psk() with a dummy key just to pacify it;
• set the real key (without touching the identity) with mbedtls_ssl_set_hs_psk() in the callback.

If you agree, I suggest ding that in ssl_client2.c. We could try changing mbedtls_ssl_conf_psk() so that it accepts an identity without a key, but I think that would take us to far, as we'd have to change all places in ssl_cli.c where the code checks wether PSK is available, and think about interactions with other plans we have in #4191 - both of which would only delay the PR. So unless you disagree, I think we should settle for that in the example for now, finalize this PR, and possibly improve the rest in future PRs.

[mpg]

Here I think we also want to call mbedtls_ssl_conf_psk() with a dummy buffer for the key, and the correct identity. That way we'll have the identity set, and won't need to set it in the callback (while the key set from the callback will overwrite the one set here).

[mpg]

Hmm, sorry, you should probably wait before implementing my suggestion. I keep changing my mind about how to go with this. The other option is to add mbedtls_ssl_set_hs_psk_identity() and document that when using a PSK callback client-side, the callback needs to call both set_hs_psk() and set_hs_psk_identity().

I understand that in cases where the client identity is set regardless of the server hint, this is less convenient, but otoh I see the following advantages:

• conceptually it may be easier to consider that each key is tied to an identity, so that they form a pair. For example when implementing the extension suggested in Relax specification of mbedtls_ssl_conf_psk() #4191 this might be an important thing, we don't want to mix and match keys and and identities from various sources
• I think there are cases where people will indeed want to select the identity based on the server's hint, so we might as well support that; then from a testing perspective it becomes easier to have only one case to test (key and identity set from the callback) rather than having to test multiple cases (the same simplicity argument applies to documentation as well).

Since I've been thinking about this (on-and-off) for 10 days now and sill haven't made up my mind, I think it's time for a second opinion. @hanno-arm it looks like you've been thinking about PSK recently, would you mind having a look and telling us what you think?

[peckto]

I see your point. In my use-case I have a static client identity and server dependent PSK. But I see the use-case, where the identity is also dependent on the server.
If I understood you correctly, there are three possible solutions:

1. The most flexible (and costly) solution would be to add the client identity to the handshake config and set it in the callback
2. Call mbedtls_ssl_conf_psk with a dummy key before the handshake (no implementation cost, only documentation)
3. Add method to configure just the client identity in the ssl config (medium cost)

[mpg]

@peckto Yes, I think we're on the same page regarding the solution space. Now as I said, I'm undecided about which of these solutions would be best to support. @hanno-arm if you had any opinion to share on the topic, that would be appreciated.

Also, just to set expectations, I'm going to be on leave until end of March, so I won't be able to discuss or review this further before April.

[hanno-becker]

@mpg @peckto Apologies, this somehow slipped my radar - I will take a look in the next days and give feedback.

[hanno-becker]

In the case of the new client-side callback, this is actually the PSK identity hint, or am I misunderstanding something?

[peckto]

I'm not sure, if I got your point. Which identity hint are you referring to? In my understanding, the client side callback receives the sever identity hint. For me the question is, how the client can configure the client identity.

[hanno-becker]

@peckto Yes that's right. My point is that now we're using a single callback which (a) for the client, receives a PSK identity hint, (b) for the server, receives a PSK identity. Semantically these are different things that are overloaded into a single parameter here, which we may or may not find acceptable. In any case, it would need to be documented.

[peckto]

Thanks, now I see your point. You want to refer with PSK identity to what the client is sending and with PSK identity hint to what the server is sending. I will clarify this in the doc.
Update: Initially I had it just the other way around

[peckto]

I updated the doc accordingly

[gilles-peskine-arm]

Please edit the commit message on the last commit to add a signoff line.

Also, there are multiple failures on the CI. Please get Travis to pass. Please ask for help if you don't understand the failures on Travis or if there are remaining failures on Jenkins once Travis passes.

[peckto]

Please edit the commit message on the last commit to add a signoff line.

Also, there are multiple failures on the CI. Please get Travis to pass. Please ask for help if you don't understand the failures on Travis or if there are remaining failures on Jenkins once Travis passes.

Thanks for the hint. I fixed the commit message.
Fixing the CI job depends on the unsolved discussion:
#3831 (comment)