Skip to content
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

Jump to bottom

Backport 2.28: Test TLS 1.2 builds with each encryption type #6394

Merged
merged 15 commits into from
Oct 12, 2022
Merged

Backport 2.28: Test TLS 1.2 builds with each encryption type #6394

merged 15 commits into from
Oct 12, 2022

Conversation

mprse
Copy link
Contributor

@mprse mprse commented Oct 6, 2022

Description

Resolves #6313

This is backport of #6374.
It is not 1 to 1 backport as default configuration and macros are slightly different.

Status

READY

mprse added 10 commits October 5, 2022 11:37
@mprse
Add test components for tls 1.2 builds with single encryption type
e535270 
Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
Fix guards for mbedtls_ct_size_mask() and mbedtls_ct_memcpy_if_eq()
9450dc7 
Both functions are used when MBEDTLS_SSL_SOME_SUITES_USE_MAC is defined not MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC.

Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
Fix configuration requirements(MBEDTLS_SSL_CONTEXT_SERIALIZATION, MBE…
864b43d 
…DTLS_SSL_SESSION_TICKETS)

Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
test_suite_cipher.function: always include aes.h
249fcca 
It is done to have MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH macro available (used in tests)

Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
test_suite_cmac.data: fix bug: use cipher type instead cipher id
b79d0dd 
Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
Adapt macro names: MBEDTLS_SSL_SOME_SUITES_USE_MAC->MBEDTLS_SSL_SOME_…
169554c 
…MODES_USE_MAC

Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
Enable Key Wrapping mode for 128-bit block ciphers
2b03880 
This is done to make configuration consistent with upstream and get rid of warnings.
ssl_ticket.c:254:17: warning: implicit declaration of function ‘mbedtls_cipher_auth_encrypt_ext’

Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
Add MBEDTLS_CIPHER_MODE_AEAD dependency to auth_crypt_tv() test function
802353c 
Test calls functions that require MBEDTLS_CIPHER_MODE_AEAD.

Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
Add GCM dependency to "Per-version suites: TLS 1.2"
efc894b 
Test was failing with error:
unknown ciphersuite: 'TLS-RSA-WITH-AES-128-GCM-SHA256'

Signed-off-by: Przemek Stekiel <[email protected]>
@mprse
Add changelog entry: tls 1.2 builds with single encryption type
0a48eae 
Signed-off-by: Przemek Stekiel <[email protected]>
@mprse mprse added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Oct 6, 2022
@mprse mprse mentioned this pull request Oct 6, 2022
Merged
3 tasks
@mpg
Copy link
Contributor

mpg commented Oct 6, 2022

It is not 1 to 1 backport as default configuration and macros are slightly different.

I think it would simplify review if you could give a list of the differences. I see 10 commits here vs 19 in the main PR, so apparently the differences were quite significant.

@mpg mpg requested review from mpg and AndrzejKurek October 6, 2022 10:29
@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Oct 6, 2022
@mpg
Copy link
Contributor

mpg commented Oct 6, 2022

PS: I added the PR to the unified board - this is a gentle reminder to do it in the future when you open a PR :)

@mprse
Copy link
Contributor Author

mprse commented Oct 6, 2022

PS: I added the PR to the unified board - this is a gentle reminder to do it in the future when you open a PR :)

Sorry for that. I usually add it to project.

I think it would simplify review if you could give a list of the differences. I see 10 commits here vs 19 in the main PR, so apparently the differences were quite significant.

From commit 169554c are the extra changes that had to be done (excluding changelog entry).

mpg
mpg reviewed Oct 7, 2022
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but some things around NITS KW are not clear IMO.

tests/scripts/all.sh Outdated Show resolved Hide resolved
tests/suites/test_suite_cipher.function Outdated Show resolved Hide resolved
AndrzejKurek
AndrzejKurek reviewed Oct 10, 2022
View reviewed changes
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

I did not find any new issues apart from those mentioned by @mpg already, although I would suggest some changelog rewording:

  • Fix bugs and missing dependencies when building and testing configurations with only one encryption type enabled in TLS 1.2.

@mpg
Copy link
Contributor

mpg commented Oct 10, 2022

I didn't want to nit-pick on the ChangeLog wording as I was otherwise happy with the main PR, but since it now looks like rework is needed after all, I agree with Andrzej's suggestion for improvement.

@AndrzejKurek AndrzejKurek added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 10, 2022
@mprse mprse force-pushed the enc_types_2_28_backport branch from f88cb84 to 0d72141 Compare October 10, 2022 13:41
@mprse
Copy link
Contributor Author

mprse commented Oct 10, 2022

I have synchronized backport with main PR and reverted both changes requested by @mpg.
I'm not sure why but now tests are locally passing without any more changes. Let's see what CI says.

@mprse mprse requested review from mpg and AndrzejKurek October 10, 2022 15:00
@mprse mprse added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 10, 2022
AndrzejKurek
AndrzejKurek approved these changes Oct 10, 2022
View reviewed changes
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

Funnily enough, I think that the backport now looks good, but I spotted two minor issues in the main PR :)

@AndrzejKurek AndrzejKurek removed the needs-ci Needs to pass CI tests label Oct 10, 2022
@mpg
Copy link
Contributor

mpg commented Oct 11, 2022

I'm not sure why but now tests are locally passing without any more changes. Let's see what CI says.

CI looks happy! Well, it reflects my experience working on dependency issues: there are so many things interacting with each other and small changes can cause such a mountain of failures that it's easy to loose track of what exactly was necessary to fix what. That's why we have reviews, looking at things calmly after the dust settled, and why it's important when we review to ask ourselves "why is this the right thing to do" rather than just "if it passes CI I guess it's right".

mpg
mpg approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 11, 2022
@gilles-peskine-arm gilles-peskine-arm merged commit c2e95fa into Mbed-TLS:mbedtls-2.28 Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrzejKurek AndrzejKurek AndrzejKurek approved these changes

@mpg mpg mpg approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mprse @mpg @AndrzejKurek @gilles-peskine-arm