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.

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

Fix no-ec enable-ktls build #19841

Closed
wants to merge 2 commits into from
Closed

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented Dec 5, 2022

The KTLS test uses a TLSv1.2 cipher that uses ECDHE

Checklist
  • documentation is added or updated
  • tests are added or updated

The KTLS test uses a TLSv1.2 cipher that uses ECDHE
@tmshort tmshort added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels Dec 5, 2022
@tmshort tmshort requested review from paulidale and t8m December 5, 2022 20:22
@@ -40,6 +40,7 @@ jobs:
enable-trace enable-fips,
no-ts,
no-ui,
enable-ktls no-ec,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a good idea. Clearly this would have caught this problem, but there is a potential combinatorial explosion here. Why stop at the combination of "enable-ktls and no-ec"? We could keep going with all the other 2 option combinations (and why stop at 2?). This doesn't seem viable.

Copy link
Member

Choose a reason for hiding this comment

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

One idea would be to enable all non-default options in these runchecker builds and disable just one enabled by default.

Copy link
Member

Choose a reason for hiding this comment

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

This should cover most of the issues IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A more general purpose test is likely out-of scope for this one particular fix, and could discover a number of issues that cause this to blow up. Perhaps an issue to report what the found problems are, so they can be fixed? Whatever is done should probably not enable features that are dependent on other libraries (e.g. compression options).

Should I just delete this test for now?

Copy link
Member

Choose a reason for hiding this comment

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

Should I just delete this test for now?

Yes, please

@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Dec 6, 2022
@mattcaswell
Copy link
Member

@paulidale - please reconfirm

@tmshort
Copy link
Contributor Author

tmshort commented Dec 7, 2022

Is there an issue in master? Three of the builds are failing now, and they weren't yesterday. All I did was remove the test from the .github/workflows directory.

@t8m
Copy link
Member

t8m commented Dec 7, 2022

Is there an issue in master? Three of the builds are failing now, and they weren't yesterday. All I did was remove the test from the .github/workflows directory.

These look like false positive from a buggy gcc.

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 7, 2022
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
The KTLS test uses a TLSv1.2 cipher that uses ECDHE

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19841)
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
The KTLS test uses a TLSv1.2 cipher that uses ECDHE

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19841)

(cherry picked from commit 2dded44)
openssl-machine pushed a commit that referenced this pull request Dec 8, 2022
The KTLS test uses a TLSv1.2 cipher that uses ECDHE

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19841)

(cherry picked from commit 2dded44)
@t8m t8m added the tests: exempted The PR is exempt from requirements for testing label Dec 8, 2022
@t8m
Copy link
Member

t8m commented Dec 8, 2022

Applied to master, 3.1, 3.0 branches. All of them are failing the GitHub CI / enable_non-default_options build which is actually caused by this issue. It cherry-picked cleanly.

@t8m t8m closed this Dec 8, 2022
@tmshort tmshort deleted the fix-ktls-no-ec branch December 12, 2022 14:45
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
The KTLS test uses a TLSv1.2 cipher that uses ECDHE

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19841)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants