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

Windows: Fix ssl_socket_test #13264

Merged
merged 8 commits into from
Oct 28, 2020
Merged

Windows: Fix ssl_socket_test #13264

merged 8 commits into from
Oct 28, 2020
+28 −10

Conversation

sunjayBhatia
Copy link
Member

@sunjayBhatia sunjayBhatia commented Sep 25, 2020 •
edited
Loading

Commit Message:
Windows: Fix ssl_socket_test

  • SmallReadsIntoSameSlice test: use write method on connection instead
    of transport socket so SSL_write failures will be properly retried,
    also otherwise empty writes from connection will trigger failed
    assertions in buffer::linearize
  • When SSL shutdown is not appropriate, perform basic socket shutdown;
    relevant when client certificate is invalid and connection should be
    dropped by server, however we still need to ensure TLS alerts are
    able to be read by client, shutdown on socket ensures this can happen
  • Replace \r with "" when comparing test fixtures to results to ensure line
    endings are ignored

Risk Level: Low
Testing: Locally on Windows
Docs Changes: N/A
Release Notes: N/A
Fixes #13191

@sunjayBhatia
Windows: Fix ssl_socket_test
138d3d1 
- SmallReadsIntoSameSlice test: use write method on connection instead
  of transport socket so SSL_write failures will be properly retried,
  also otherwise empty writes from connection will trigger failed
  assertions in buffer::linearize
- When SSL shutdown is not appropriate, perform basic socket shutdown;
  relevant when client certificate is invalid and connection should be
  dropped by server, however we still need to ensure TLS alerts are
  able to be read by client, shutdown on socket ensures this can happen
- Open files for test in text mode to take care of line ending issues

Signed-off-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia
Copy link
Member Author

cc @envoyproxy/windows-dev

@sunjayBhatia sunjayBhatia mentioned this pull request Sep 25, 2020
Closed
@dio
Copy link
Member

dio commented Sep 25, 2020

/assign @PiotrSikora

@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Sep 25, 2020 •
edited
Loading

Need to make binary mode open optional/refactor some tests

Also some of the ssl_socket_tests seem to fail in RBE and not locally, another thing to dig into

/wait

@davinci26
Copy link
Member

Looks good besides a small fix. Thanks a lot for getting this one to work!

sunjayBhatia and others added 2 commits September 25, 2020 17:50
@sunjayBhatia
Use cleaner \r replacement
144c7ae 
Signed-off-by: Sunjay Bhatia <[email protected]>
@wrowe
Fix format
ef8db9c 
Signed-off-by: William A Rowe Jr <[email protected]>
@sunjayBhatia
Copy link
Member Author

I believe the openssl fork errors are either caused by the Windows update service, or MSYS2 issues, hopefully addressed by one of: envoyproxy/envoy-build-tools#101 or envoyproxy/envoy-build-tools#100

@sunjayBhatia
Copy link
Member Author

See #13276

Looks like envoyproxy/envoy-build-tools#100 is required to solve this issue in the build container/RBE

@sunjayBhatia sunjayBhatia mentioned this pull request Sep 29, 2020
Merged
@wrowe
Copy link
Contributor

wrowe commented Sep 30, 2020

/wait

on envoyproxy/envoy-build-tools#100

@sunjayBhatia sunjayBhatia mentioned this pull request Oct 1, 2020
Merged
@sunjayBhatia
Copy link
Member Author

/wait

waiting on #13367

@sunjayBhatia sunjayBhatia mentioned this pull request Oct 6, 2020
Merged
@sunjayBhatia
Copy link
Member Author

Failing due to same reason this test is being marked as flaky for the time being: #13407

@sunjayBhatia
Copy link
Member Author

/wait

still waiting on resolving the openssl msys2 fork issue, may continue with just checking in certs etc.

@sunjayBhatia sunjayBhatia mentioned this pull request Oct 22, 2020
Merged
@sunjayBhatia
Copy link
Member Author

Blocked on whichever #13702 or envoyproxy/envoy-build-tools#107 is chosen as the solution

@sunjayBhatia
Copy link
Member Author

unblocked by #13702

@sunjayBhatia
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines.

🐱

Caused by: a #13264 (comment) was created by @sunjayBhatia.

see: more, trace.

@sunjayBhatia
Copy link
Member Author

@PiotrSikora this should be ready to go again, macos CI failure looks like one of the systematic ones we've been having generally

@sunjayBhatia
Copy link
Member Author

merged master again to hopefully get the non-windows flakes out, @PiotrSikora have you had a chance to take a look at this yet?

@mattklein123 mattklein123 merged commit 28909c4 into envoyproxy:master Oct 28, 2020
@wrowe wrowe deleted the windows-fix-ssl-socket-test branch October 28, 2020 20:02
mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 30, 2020
@mpuncel
Merge branch 'master' into mpuncel/http2-hc-goaway
0d57547 
* master: (83 commits)
  tls: Typesafe tls slots (envoyproxy#13789)
  docs(example): Correct URL for caching example page (envoyproxy#13810)
  [fuzz] Made health check fuzz more efficient (envoyproxy#13747)
  rtds: properly scope rtds stats (envoyproxy#13764)
  http: fixing a bug with IPv6 hosts (envoyproxy#13798)
  connection: Remember transport socket read resumption requests and replay them when re-enabling read. (envoyproxy#13772)
  network: adding some accessors for ALPN work. (envoyproxy#13785)
  docs: added a step about how to handle platform specific extensions (envoyproxy#13759)
  Fix identation in ip transparency code snippet (envoyproxy#13743)
  wasm: enable WAVM's stack unwinding feature (envoyproxy#13792)
  log: set route name for direct response (envoyproxy#13683)
  Use nghttp2 as external dependsncy in protocol_constraints_lib (envoyproxy#13763)
  [Windows] Update windows dev docs (envoyproxy#13741)
  cel: patch thread safety issue (envoyproxy#13739)
  Windows: Fix ssl_socket_test (envoyproxy#13264)
  apple dns: add fake api test suite (envoyproxy#13780)
  overload: scale selected timers in response to load (envoyproxy#13475)
  examples: Add dynamic configuration (control plane) sandbox (envoyproxy#13746)
  Removed exception in getResponseStatus() (envoyproxy#13314)
  network: add timeout for transport connect (envoyproxy#13610)
  ...

Signed-off-by: Michael Puncel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davinci26 davinci26 davinci26 left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@PiotrSikora PiotrSikora PiotrSikora approved these changes

@lizan lizan Awaiting requested review from lizan

@asraa asraa Awaiting requested review from asraa

Assignees

@PiotrSikora PiotrSikora

Labels
area/tls area/windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Envoy Server drops the connection on Windows when the client certificate is invalid
6 participants
@sunjayBhatia @dio @davinci26 @wrowe @PiotrSikora @mattklein123