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

tests: tls test without sleep #319

Merged
merged 1 commit into from
Dec 7, 2023
Merged

tests: tls test without sleep #319

merged 1 commit into from
Dec 7, 2023

Conversation

0pq76r
Copy link

@0pq76r 0pq76r commented Dec 1, 2023

I ran into some flakeyness when running the TLS tests with a HSM.

I changed the test to use expect such that the timing for sending stuff to the server and the client process depends on the program output.

@simo5
Copy link
Member

simo5 commented Dec 1, 2023

Ooh nice, I wanted to do something like this for ages, thanks!

@simo5
Copy link
Member

simo5 commented Dec 1, 2023

Ignore the softhsm failures, there is a bug in GnuTLS 3.8.2 that causes it

@simo5
Copy link
Member

simo5 commented Dec 1, 2023 •
edited
Loading

However this MacOS/softoken failure is an actual issue:

Executing ./ttls

## Test SSL_CTX creation
SSL Context works!

## Test an actual TLS connection
spawn openssl s_server -accept 23456 -naccept 1 -key pkcs11:type=private;id=%75%5a%4c%f8%b8%f6%87%32%2a%cf%9a%b6%9e%e1%9b%55%52%e3%73%e1 -cert pkcs11:type=cert;object=testCert
Using default temp DH parameters
ACCEPT
spawn openssl s_client -connect localhost:23456 -quiet
Tcl_WaitForEvent: Notifier not initialized
FAIL tls-softokn.t (exit status: 1)

@0pq76r
Copy link
Author

0pq76r commented Dec 4, 2023

I don't have access to a Mac at the moment. I'll try to fix it but it may take a few days.

@0pq76r 0pq76r force-pushed the improvement/tls-test branch 2 times, most recently from 6e76eb3 to db66a33 Compare December 5, 2023 08:40
simo5
simo5 approved these changes Dec 5, 2023
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!
Not sure why shellcheck is complaining, looks like a false positive
@Jakuje could you review and merge if it is good for you as well.

@Jakuje
Copy link
Contributor

Jakuje commented Dec 6, 2023

LGTM! Not sure why shellcheck is complaining, looks like a false positive @Jakuje could you review and merge if it is good for you as well.

There was a comment that was removed which disabled this check as I probably hit the same issue before:

# shellcheck disable=SC2317  # Shellcheck for some reason does not follow trap

Returning this back should silence the shellcheck.

Jakuje
Jakuje reviewed Dec 6, 2023
View reviewed changes
tests/ttls Outdated
Comment on lines 27 to 44
set timeout 60;
expect {
\"ACCEPT\" {};
default {exit 1;};
}
set server_ready [open \"${TMPPDIR}/s_server_ready\" w+];
puts \$server_ready \"READY\n\";
close \$server_ready;
expect {
\"END SSL SESSION PARAMETERS\" {};
default {exit 1;};
}
send \" TLS SUCCESSFUL \n\"
send \"Q\n\"
expect {
eof {exit 0;};
default {exit 1;};
}" > "${TMPPDIR}/s_server_output" &
Copy link
Contributor

Choose a reason for hiding this comment

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

only nit I would have against the extensive first indent and just two spaces indent in the expect blocks. I would just keep it flat with 4 spaces for each. The same for the expect below.

tests/ttls

# Make sure we terminate programs if test fails in the middle
# shellcheck disable=SC2317 # Shellcheck for some reason does not follow trap
Copy link
Contributor

Choose a reason for hiding this comment

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

keep this comment before the trap function to avoid the shellcheck failure.

Jakuje
Jakuje approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

There are some memory leaks in the CI (as fedora already picked up good versions of gnutls package), but they are not related to your changes so after squashing the changes I think we are good to go.

@Jakuje
Copy link
Contributor

Jakuje commented Dec 6, 2023

memory leak fixed separately in #326.

@fw0test0copilot
tests: tls test without sleep
220097e 
Synchronizing server and client command using a fifo instead of sleep.

Signed-off-by: Florian Wernli <[email protected]>
@0pq76r 0pq76r force-pushed the improvement/tls-test branch from 214ee78 to 220097e Compare December 7, 2023 07:54
@simo5
Copy link
Member

simo5 commented Dec 7, 2023

Thanks @0pq76r this looks good now, merging.

@simo5 simo5 merged commit 610c1b3 into latchset:main Dec 7, 2023
14 of 19 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

@Jakuje Jakuje Jakuje approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0pq76r @simo5 @Jakuje @fw0test0copilot