erlyaws / yaws

Yaws webserver
https://erlyaws.github.io
BSD 3-Clause "New" or "Revised" License
1.28k stars 267 forks source link

Changes for OTP 26 #471

Closed avtobiff closed 8 months ago

avtobiff commented 1 year ago
vinoski commented 1 year ago

I've had basically the same file:pid2name/1 changes sitting on a local branch in my clone ever since OTP 26 release candidates were published. But I've never had time to figure out what's going wrong with the test failures under OTP 26, which your changes are also hitting.

avtobiff commented 1 year ago

I see. I tried bisecting otp between maint-25..OTP-26.0 and digging manually.

The failing test cases:

Fails on OTP-26.0-rc1.

Have not looked into why.

Error: {{assertMatch,
            [{module,ssl_sni_SUITE},
             {line,228},
             {expression,
                 "testsuite : http_get ( Url , [ HostHdr , ConnHdr ] , [ ] , SSLOpts )"},
             {pattern,"{ ok , { { _ , 200 , _ } , _ , Res } }"},
             {value,
                 {error,
                     {tls_alert,
                         {handshake_failure,
                             "TLS client: In state hello received SERVER ALERT: Fatal - Handshake Failure\n"}}}}]},

Fails on OTP-26.0-rc1. Passes on erlang/otp@9e7c0300548f5381cc44e53dbd84ee4c6ccede76 .

Shows that something is going wrong in the handshake, presumably already in ssl:handle_options/5 but I haven't found anything conclusive about where or how SNI handling has changed.

Fixed in PR.

OTP changed default verify option in erlang/otp@bb3603db8459e13e9e5f27c4fb46ca59ee8e4a39

Adding {verify, verify_none} to websockets_SUITE:sslopen/2 makes the test pass, which could be ok short term. It is consistent with the previous behavior.

The better long term option is to generate proper TLS data for testing.

avtobiff commented 8 months ago

Regarding {verify, verify_none}, this needs to be changed because {verify, verify_peer} now doesn't allow {cacerts, undefined} but requires a trusted cert.

https://www.erlang.org/patches/otp-26.0#ssl-11.0 https://github.com/erlang/otp/issues/5899

vinoski commented 8 months ago

Regarding {verify, verify_none}, this needs to be changed because {verify, verify_peer} now doesn't allow {cacerts, undefined} but requires a trusted cert.

Yeah, I've been working on this and found the same. I should probably push my current changes up on a branch, even though they still don't pass all the tests.

avtobiff commented 8 months ago
Changes for OTP 27

* file:pid2name/1 is removed, update yaws_config:fload functions to pass
  config file name.

* Add OTP 26.0, 26.1, and 26.2 to the test matrix.

* Make websockets_SUITE:secure_socket pass

  OTP changed default ssl verify option in

      commit bb3603db8459e13e9e5f27c4fb46ca59ee8e4a39

      ssl: Change client default verify

  Adding {verify, verify_none} to websockets_SUITE:sslopen/2 makes the
  test pass, which could be ok short term. The better long term option
  is to generate proper TLS data for testing.

* Remove unused sni_not_available test

  The test ssl_sni_SUITE:sni_not_available is failing on OTP-26 and it
  has not been relevant since OTP R7.

* Update ssl/mkcert_altname

  * Update openssl mkcert_altname README and config, use SHA-256 and
    2048 bit keys by defualt.

  * alice.sni.example.com-*.pem and yaws.sni.example.com-*.pem

    Used SHA-1 signature algorithm, which OTP ssl doesn't allow anymore.
    This fixes the failing ssl_sni_SUITE tests.

Fixes #467
avtobiff commented 8 months ago

This uncovered a bug in OTP where dhfile was not read from the ssl options at all. See https://github.com/erlang/otp/pull/7984

When the above OTP PR is merged, Yaws works on OTP-26+. It is tested on both maint and master (both with the dhfile patch).

avtobiff commented 8 months ago

Perhaps it should be renamed to Changes for OTP-26?

vinoski commented 8 months ago

Perhaps it should be renamed to Changes for OTP-26?

Yes, done.

vinoski commented 8 months ago

This uncovered a bug in OTP where dhfile was not read from the ssl options at all. See erlang/otp#7984

Interesting!

When the above OTP PR is merged, Yaws works on OTP-26+. It is tested on both maint and master (both with the dhfile patch).

I guess this means we either have to wait to merge this until the OTP team issues a patch release containing the fix that we can then add to our Github Actions for testing, or we have to adjust the code to not run certain tests on OTP 26 unless we can determine we're running the patched release or newer.

avtobiff commented 8 months ago

Yes I thought about when to merge and possibly handle tests for OTP-26 versions as well. IMHO we can wait and monitor how the OTP PR progresses and decide later.

avtobiff commented 8 months ago

Pushed a suggestion on how and when to skip the dhfile tests on broken OTP-26 releases.

The OTP ssl dhfile PR is merged, but I don't know what release it will land in. Guessing that it will be fixed in the next OTP-26 point release 26.2.2.

Disabling the tests for other OTP-26 releases. Perhaps patch releases will get the fix as well, if so they can be added to the condition to skip the dhfile tests.

etnt commented 8 months ago

Looks OK to me.