Permissions 2b: TLS 1.3 sigalg selection

[mprse]

Description

TLS 1.3: Take into account key policy while picking a signature algorithm. Resolves https://github.com/Mbed-TLS/mbedtls/issues/5844

@mpg @ronald-cron-arm Do we need extra tests for this change?

Status

IN DEVELOPMENT

Requires Backporting

NO

Migrations

NO

Todos

• [ ] Tests ???

[mprse]

Regarding tests in ssl-opt.sh. I started with the following two tests to check if usage sign flag is taken into account:

requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3
requires_config_enabled MBEDTLS_USE_PSA_CRYPTO
requires_config_enabled MBEDTLS_RSA_C
run_test    "TLS1.3 opaque key: no suitable algorithm found" \
            "$P_SRV debug_level=4 force_version=tls13 key_opaque=1 key_opaque_algs=rsa-decrypt,none" \
            "$P_CLI debug_level=4 force_version=tls13 key_opaque=1 key_opaque_algs=rsa-decrypt,rsa-sign-pss" \
            1 \
            -c "The SSL configuration is tls13 only" \
            -s "The SSL configuration is tls13 only" \
            -c "key type: Opaque" \
            -s "key types: Opaque, Opaque" \
            -c "error" \
            -s "select_sig_alg_for_certificate_verify:no suitable signature algorithm found" \

requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3
requires_config_enabled MBEDTLS_USE_PSA_CRYPTO
requires_config_enabled MBEDTLS_RSA_C
run_test    "TLS1.3 opaque key: suitable algorithm found" \
            "$P_SRV debug_level=4 force_version=tls13 key_opaque=1 key_opaque_algs=rsa-decrypt,rsa-sign-pss" \
            "$P_CLI debug_level=4 force_version=tls13 key_opaque=1 key_opaque_algs=rsa-decrypt,rsa-sign-pss" \
            0 \
            -c "The SSL configuration is tls13 only" \
            -s "The SSL configuration is tls13 only" \
            -c "key type: Opaque" \
            -s "key types: Opaque, Opaque" \
            -C "error" \
            -S "error" \

Both tests are passing.

Then I tried to use two keys config on server and it seems that the results are unexpected:

requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3
requires_config_enabled MBEDTLS_USE_PSA_CRYPTO
requires_config_enabled MBEDTLS_RSA_C
run_test    "TLS1.3 opaque key: 2 keys on server, suitable algorithm found" \
            "$P_SRV debug_level=4 force_version=tls13 key_opaque=1 key_opaque_algs=ecdsa-sign,none key_opaque_algs2=rsa-decrypt,rsa-sign-pss" \
            "$P_CLI debug_level=4 force_version=tls13 key_opaque=1 key_opaque_algs=rsa-decrypt,rsa-sign-pss" \
            0 \
            -c "The SSL configuration is tls13 only" \
            -s "The SSL configuration is tls13 only" \
            -c "key type: Opaque" \
            -s "key types: Opaque, Opaque" \
            -C "error" \
            -S "error" \

First key is ecdsa-sign and second is rsa-decrypt,rsa-sign-pss. The second matches to client side, but the test fails with select_sig_alg_for_certificate_verify:no suitable signature algorithm found and client gets alert message. If I switch keys on server so that key_opaque_algs2=ecdsa-sign,none key_opaque_algs=rsa-decrypt,rsa-sign-pss, then test passes.

[mprse]

Pushed last changes, so it is easier to review the tests. Now we have conflict, but will wait with rebasing until review is done.

[mprse]

PR is suspended while waiting for the alternative solution from @ronald-cron-arm. Alternative solution will probably lead to closing this PR. Details: https://github.com/Mbed-TLS/mbedtls/pull/6051#discussion_r915716705

[ronald-cron-arm]

I've rebased the PR and added seven commits to implement and test what was discussed in https://github.com/Mbed-TLS/mbedtls/pull/6051#discussion_r915716705. Let's see what the CI says first.

[ronald-cron-arm]

The CI ran successfully, @mpg @mprse please have a look.

[ronald-cron-arm]

@mpg @mprse I believe I have addressed your comments. Please have another look.

[mpg]

Point of procedure: since this PR is co-authored by @mprse and @ronald-cron-arm can each of you review the other's commits and record the result as a review? This we we know each commit has two reviewers other than its author. (Either that, or someone who's not one of you reviews everything, but it seems simpler for you two to review.)

[ronald-cron-arm]

Point of procedure: since this PR is co-authored by @mprse and @ronald-cron-arm can each of you review the other's commits and record the result as a review? This we we know each commit has two reviewers other than its author. (Either that, or someone who's not one of you reviews everything, but it seems simpler for you two to review.)

Yes will do

[mpg]

I believe this needs a ChangeLog entry, as this is a user-visible change: some cases where the handshake would have failed before will now succeed, I think. (And some case that would have failed will also fail but different, which I don't think is worth mentioning.) Something along those lines, in the "Features" section: "When using opaque keys with TLS 1.3, the code will now properly select the signature algorithm according to the key's policy. As a result, some handshake that would have failed due to selecting the wrong algorithm will now succeed." Hmm, seen that way, sounds a bit like a bug fix actually... Wdyt?

[mprse]

I cannot approve this PR as I'm an an author(person who opened this one). I reviewed changes provided by @ronald-cron-arm and I'm giving approval via this comment (if this is ok).

[ronald-cron-arm]

I believe this needs a ChangeLog entry, as this is a user-visible change: some cases where the handshake would have failed before will now succeed, I think. (And some case that would have failed will also fail but different, which I don't think is worth mentioning.) Something along those lines, in the "Features" section: "When using opaque keys with TLS 1.3, the code will now properly select the signature algorithm according to the key's policy. As a result, some handshake that would have failed due to selecting the wrong algorithm will now succeed." Hmm, seen that way, sounds a bit like a bug fix actually... Wdyt?

#5844 is an enhancement task thus I would rather be on the side of "Features". I've tried something in the sense that opaque keys were not supported before as private keys associated to certificates for authentication in TLS 1.3.

[ronald-cron-arm]

I've added a change log. @mprse and @mpg please have a look. When you are happy with it, I will approve the PR given my approval for the six first commits from @mprse and the approval from @mprse of the subsequent ones.

[mprse]

Change log looks good. Approved.