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

Extend the ttls test to be able to test different configurations #422

Merged
merged 7 commits into from
Jul 30, 2024
Merged

Extend the ttls test to be able to test different configurations #422

merged 7 commits into from
Jul 30, 2024

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Jul 17, 2024 •
edited by simo5
Loading

Description

The TLS tests now tests just the default parameters with RSA key. This PR extends it to test also TLS 1.2, ECDSA keys and specific ciphersuites.

Checklist

  • Test suite updated with functionality tests

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible commit messages
  • Coverity Scan has run if needed (code PR) and no new defects were found

@Jakuje
Copy link
Contributor Author

Jakuje commented Jul 17, 2024 •
edited
Loading

Looks like the ECDSA fails on MacOS. @neverpanic any idea why?

@Jakuje Jakuje force-pushed the ttls-test branch 3 times, most recently from 1df3535 to 8b0529b Compare July 17, 2024 16:55
@Jakuje
Copy link
Contributor Author

Jakuje commented Jul 17, 2024

Removed the commit that made it failing all over the place and keeping just the fixups that should make the CI pass (except for mac?) and that should be generally useful.

The commit making all of that failing is available in the wip branch https://github.com/Jakuje/pkcs11-provider/commits/ttls-test-wip/ and I believe they might be useful for reproducing further TLS issues.

@Jakuje Jakuje mentioned this pull request Jul 17, 2024
Closed
@simo5
Copy link
Member

simo5 commented Jul 22, 2024

Uhm in the second macos test faile di see that softhsm setup fails, but softokn and kryoptic setups succeed, yet all tests are skipped, would you happen to know why ?

@simo5 simo5 added the covscan-ok Coverity scan passed label Jul 22, 2024
@Jakuje Jakuje force-pushed the ttls-test branch 3 times, most recently from 99b85ee to 2b33ac4 Compare July 23, 2024 13:50
@neverpanic
Copy link
Collaborator

@Jakuje Sorry for the delay on this – I checked out this branch, built it locally and ran the tests, and they all pass on my system. This either means that the issue isn't easily reproducible, or it only happens with components from Homebrew (I use MacPorts).

If you don't have any idea what's going on I can spend some more time to investigate and attempt to reproduce, but that will involve actually setting up a macOS VM, so unless we have no further avenues to follow up on, I'd like to avoid this.

Jakuje added 2 commits July 30, 2024 11:03
@Jakuje
ci: Migrate to upload-artifacts@v4
a8d444e 
The v3 will be deprecated soon

https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje
tests: Move the check for the p11-kit to proxy script
cf2531a 
The softhsm no longer uses the p11-kit proxy.

It is not relevant to kryoptic either.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje Jakuje force-pushed the ttls-test branch 3 times, most recently from 4002a46 to 44fe305 Compare July 30, 2024 09:46
@Jakuje
Copy link
Contributor Author

Jakuje commented Jul 30, 2024

Sounds like the issue was with the server that I did not kill it after each test. For some reason, it did not matter for linux, but did for macos. Now all looks green, except for the coverity, which I think is not relevant for this change.

Jakuje added 5 commits July 30, 2024 12:25
@Jakuje
tests: Set the CA cert usage for cert signing
68d9986 
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje
tests: Remove needless quirks for kryoptic
c4d287a 
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje
tests: Run different TLS versions and tests with different keys and a…
7c36c08 
…lgorithms

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje
tests: Check CA cert in TLS test
56c5fd3 
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje
ci: Fix MacOS version in the artifacts name
7bda3a2 
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@simo5
Copy link
Member

simo5 commented Jul 30, 2024

The coverity scan is "failed" because it detected a code change after the covscan-ok label was set.

@simo5
Copy link
Member

simo5 commented Jul 30, 2024

But there is no code change, so that's odd, I will have to recheck the logic in the CI automation, I will manually set the label after review

simo5
simo5 approved these changes Jul 30, 2024
View reviewed changes
Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

LGTM

@simo5 simo5 added covscan-ok Coverity scan passed and removed covscan-ok Coverity scan passed labels Jul 30, 2024
@simo5 simo5 merged commit 49f4697 into latchset:main Jul 30, 2024
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simo5 simo5 simo5 approved these changes

Assignees
No one assigned
Labels
covscan-ok Coverity scan passed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jakuje @simo5 @neverpanic