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: support implicit SSL mail protocols #10077

Closed
wants to merge 3 commits into from
Closed

tests: support implicit SSL mail protocols #10077

wants to merge 3 commits into from

Conversation

monnerat
Copy link
Contributor

And as bonus, use a hash table in runtests.pl for server ports and introduce 3 new tests for cases exposed in #10053.

@jay jay added the tests label Dec 10, 2022
@jay jay mentioned this pull request Dec 10, 2022
Closed
@monnerat
Copy link
Contributor Author

From the current CI test results:

  1. I suppose new tests should be disabled for rustls ?
  2. I suspect we have a race problem when starting the protocol server just before stunnel. runtests.pl should probably make sure the server is running before starting stunnel. For now, move imaps/pop3s/smtps tests after first corresponding clear protocol tests.

@bagder
Copy link
Member

bagder commented Dec 11, 2022

I suppose new tests should be disabled for rustls ?

Why? rustls is supposed to generally work fine.

I suspect we have a race problem

Hm, yes it is supposed to first verify that the non-stunnel server works before it starts stunnel. See startservers for FTPS for example, it first runs runpingpongserver (which also verifies that it works) before it runs runftpsserver that starts stunnel.

@bagder bagder added the TLS label Dec 11, 2022
@monnerat
Copy link
Contributor Author

Why? rustls is supposed to generally work fine.

It has 12 disabled tests that are supposed to work with. That's why I asked.

Hm, yes it is supposed to first verify that the non-stunnel server works before it starts stunnel. See startservers for FTPS for example, it first runs runpingpongserver (which also verifies that it works) before it runs runftpsserver that starts stunnel.

I've reused the ftps code for the mail protocols (I even renamed runftpsserver as runsecureserver) thus I expect new protos behave the same. Investigating.

@bagder
Copy link
Member

bagder commented Dec 11, 2022

It has 12 disabled tests that are supposed to work with. That's why I asked.

Right, but we use TLS in many more tests than just those. These are just tests that for some reason fail with rustls. It is still expected to work just like the other backends unless there's some anomaly.

@bagder
Copy link
Member

bagder commented Jan 2, 2023

There are test failures now with NSS and rustls. Are they permafails or flaky?

@monnerat
Copy link
Contributor Author

monnerat commented Jan 3, 2023

There are test failures now with NSS and rustls. Are they permafails or flaky?

They seem to be non-flaky.
I've succeeded in reproducing the NSS failure locally and it shows the transfer never ends (stuck idle until timeout).
I did not try with rustls as it's way too complicated to compile/install locally.
I'm now 80% sure this does not come from the PR changes themselves, but rather are backend-specific problems wich we did not experience before as we were not able to run TLS mail tests. Help would be appreciated.
In all cases this PR is not ready for commit.

@bagder
Copy link
Member

bagder commented Jan 4, 2023

In all cases this PR is not ready for commit.

Maybe make it a "draft" PR for the time being then to make it more visible to observers?

@bagder
Copy link
Member

bagder commented Jan 4, 2023

I've succeeded in reproducing the NSS failure locally and it shows the transfer never ends (stuck idle until timeout).

Since NSS is already deprecated and scheduled for deletion in August, it feels like a boring job to try to work on a bugfix now for an issue that probably has existed for a long time already and obviously has not disturbed any users enough to report it to us. It feels attractive to just mark this single test disabled for NSS builds.

I did not try with rustls as it's way too complicated to compile/install locally.

rustls also already has a range of tests disabled so it wouldn't be too terrible to add a few more to that list. This backend is stilled marked experimental for this reason.

It generally takes someone with a little insight into rustls and its API to work on these issues.

@monnerat
Copy link
Contributor Author

monnerat commented Jan 4, 2023

Thanks for your comment.

In the meantime I've found the problem with NSS: the data_pending method is not implemented and Curl_none_data_pending is used for it, always telling there is no data ready for input.

When some TLS data is buffered by the backend, there's therefore no chance to see it and wake the handle. The problem is likely to occur with all backends not specifically implementing data_pending.

I've managed to make it work with NSS in two ways:

  1. By adding a delay in ftpserver.pl between the imap data output and postfetch output: this forces the postfetch output to be transmitted in a separate packet and therefore not buffered. This was just a confirmation test and not a problem solution.
  2. By effectively implementing nss_data_pending:.

rustls does implement data_pending but I suspect it does not work as it should.

I then consider this problem as the first one being detected by this PR and already existing before it. Do you want a separate issue report for it ? I think it should be fixed (for all backends) before we commit the current PR to avoid "parasitic" reports in the CI tests.

@bagder
Copy link
Member

bagder commented Jan 4, 2023

Do you want a separate issue report for it ?

I think at least a separate PR would make sense, yes!

@monnerat
Copy link
Contributor Author

monnerat commented Jan 4, 2023

I think at least a separate PR would make sense, yes!.

I can produce one for NSS only. However the problem should probably be handled more generally, either for each backend or by supporting (blocking?) data reads from backends that do not implement data_pending.

The rustls backend code is out of my skill.

@monnerat monnerat marked this pull request as draft January 4, 2023 10:08
monnerat added a commit to monnerat/curl that referenced this pull request Jan 4, 2023
@monnerat
nss: implement data_pending method
7acb570 
NSS currently uses the default Curl_none_data_pending() method which
always returns false, causing TLS buffered input data to be missed.

The current commit implements the nss_data_pending() method that properly
monitors the presence of available TLS data.

Ref:curl#10077
monnerat added a commit to monnerat/curl that referenced this pull request Jan 5, 2023
@monnerat
nss: implement data_pending method
b8fe900 
NSS currently uses the default Curl_none_data_pending() method which
always returns false, causing TLS buffered input data to be missed.

The current commit implements the nss_data_pending() method that properly
monitors the presence of available TLS data.

Ref:curl#10077
monnerat added a commit to monnerat/curl that referenced this pull request Jan 5, 2023
@monnerat
nss: implement data_pending method
c35b8f4 
NSS currently uses the default Curl_none_data_pending() method which
always returns false, causing TLS buffered input data to be missed.

The current commit implements the nss_data_pending() method that properly
monitors the presence of available TLS data.

Ref:curl#10077
monnerat added a commit to monnerat/curl that referenced this pull request Jan 5, 2023
@monnerat
nss: implement data_pending method
ada65c9 
NSS currently uses the default Curl_none_data_pending() method which
always returns false, causing TLS buffered input data to be missed.

The current commit implements the nss_data_pending() method that properly
monitors the presence of available TLS data.

Ref:curl#10077
monnerat added a commit to monnerat/curl that referenced this pull request Jan 5, 2023
@monnerat
nss: implement data_pending method
41d0251 
NSS currently uses the default Curl_none_data_pending() method which
always returns false, causing TLS buffered input data to be missed.

The current commit implements the nss_data_pending() method that properly
monitors the presence of available TLS data.

Ref:curl#10077
bagder pushed a commit that referenced this pull request Jan 7, 2023
@monnerat @bagder
nss: implement data_pending method
ee0f739 
NSS currently uses the default Curl_none_data_pending() method which
always returns false, causing TLS buffered input data to be missed.

The current commit implements the nss_data_pending() method that properly
monitors the presence of available TLS data.

Ref:#10077

Closes #10225
@monnerat
Copy link
Contributor Author

monnerat commented Jan 8, 2023

Should be OK now.
New tests successful with NSS after commits ee0f739 and f22cd67 applied.
Still fail with rustls: timeout in hello handshake --> rustls problem; new tests disabled for it.
Gskit should fail too because data_pending method is not implemented.
Other backends do not seem to have problems.

@monnerat monnerat marked this pull request as ready for review January 8, 2023 01:40
@bagder bagder closed this in 8bfa4d6 Feb 25, 2023
bagder pushed a commit that referenced this pull request Feb 25, 2023
@monnerat @bagder
tests: support for imaps/pop3s/smtps protocols
5855c47 
Closes #10077
bagder pushed a commit that referenced this pull request Feb 25, 2023
@monnerat @bagder
tests: test secure mail protocols with explicit SSL requests
90756d3 
New tests 987, 988 and 989, disabled for rustls (hanging).

Closes #10077
@bagder
Copy link
Member

bagder commented Feb 25, 2023

Thanks! and sorry for taking so long

@monnerat
Copy link
Contributor Author

Thanks! and sorry for taking so long

Never mind and thanks for pulling.

@monnerat monnerat deleted the testmailssl branch February 25, 2023 13:21
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
@monnerat @bch
nss: implement data_pending method
b00a35d 
NSS currently uses the default Curl_none_data_pending() method which
always returns false, causing TLS buffered input data to be missed.

The current commit implements the nss_data_pending() method that properly
monitors the presence of available TLS data.

Ref:curl#10077

Closes curl#10225
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
@monnerat @bch
runtests: use a hash table for server port numbers
f8b8d79 
Closes curl#10077
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
@monnerat @bch
tests: support for imaps/pop3s/smtps protocols
9852145 
Closes curl#10077
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
@monnerat @bch
tests: test secure mail protocols with explicit SSL requests
0b82dff 
New tests 987, 988 and 989, disabled for rustls (hanging).

Closes curl#10077
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
tests TLS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@monnerat @bagder @jay