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.

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

add tls config for http client in proxy middleware #1393

Merged
merged 8 commits into from Jun 24, 2021
Merged

add tls config for http client in proxy middleware #1393

merged 8 commits into from Jun 24, 2021

Conversation

tuhao1020
Copy link
Contributor

In some cases, we often need to disable certificate validation, so I add WithTlsConfig function for proxy.
This function can be used before Forward, for example:

proxy.WithTlsConfig(tls.Config{
    InsecureSkipVerify: true,
})

@welcome
Copy link

welcome bot commented Jun 19, 2021

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

Thx, could you add a description/example here https://github.com/gofiber/fiber/blob/master/middleware/proxy/README.md and a test

@tuhao1020
Copy link
Contributor Author

@ReneWerner87 example updated. but I don't know how to write a test that mock a https server and use a self-signed certificate 😿

@ReneWerner87
Copy link
Member

please check
https://github.com/gofiber/fiber/blob/master/app_test.go#L971
https://github.com/gofiber/fiber/blob/master/app_test.go#L988
https://github.com/gofiber/fiber/blob/master/app_test.go#L1022
https://github.com/gofiber/fiber/blob/master/client_test.go#L909

we have already deposited a test certificate which you can use, please have a look at the tests, think you can come up with an idea, you can also do it like with the client agent test

@tuhao1020
Copy link
Contributor Author

@ReneWerner87 test added, please have a check.

@ReneWerner87 ReneWerner87 self-requested a review June 20, 2021 15:03
@ReneWerner87
Copy link
Member

Could you check the unittests again, there is an error and maybe its related to the test code changes.

@tuhao1020
Copy link
Contributor Author

=== CONT  Test_Proxy_Timeout_Slow_Server
--- PASS: Test_Proxy_Forward_WithTlsConfig (10.64s)
==================
WARNING: DATA RACE
Read at 0x0000011c3b08 by goroutine 13:
  github.com/gofiber/fiber/v2/middleware/proxy.Balancer()
      D:/a/fiber/fiber/middleware/proxy/proxy.go:50 +0x5d6
  github.com/gofiber/fiber/v2/middleware/proxy.Test_Proxy_Modify_Request()
      D:/a/fiber/fiber/middleware/proxy/proxy_test.go:179 +0x1a2
  testing.tRunner()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1050 +0x1f2

Previous write at 0x0000011c3b08 by goroutine 11:
  [failed to restore the stack]

Goroutine 13 (running) created at:
  testing.(*T).Run()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1095 +0x53e
  testing.runTests.func1()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1339 +0xad
  testing.tRunner()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1050 +0x1f2
  testing.runTests()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1337 +0x59b
  testing.(*M).Run()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1252 +0x306
  main.main()
      _testmain.go:62 +0x22a

Goroutine 11 (finished) created at:
  testing.(*T).Run()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1095 +0x53e
  testing.runTests.func1()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1339 +0xad
  testing.tRunner()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1050 +0x1f2
  testing.runTests()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1337 +0x59b
  testing.(*M).Run()
      C:/hostedtoolcache/windows/go/1.14.15/x64/src/testing/testing.go:1252 +0x306
  main.main()
      _testmain.go:62 +0x22a
==================
=== CONT  Test_Proxy_Modify_Request
Error:     testing.go:965: race detected during execution of test
--- FAIL: Test_Proxy_Modify_Request (2.00s)
=== CONT  Test_Proxy_Timeout_Slow_Server
Error:     testing.go:965: race detected during execution of test
--- FAIL: Test_Proxy_Timeout_Slow_Server (4.00s)

@ReneWerner87 Does it mean clientTlsConfig should be protected by a mutex? But they're all ok in my local test environment.

@ReneWerner87
Copy link
Member

will also analyze it again, give me a little time, don't know if i will make it today

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

the code looks good, i just noticed that you set this tls config global and it is only present once

for the client that does the DO and FORWARD without further settings this is completely ok

but with the other variant of the BALANCER or with the NEW method this is not ok, because this middleware could be used multiple times for different routes, i would also expect that it can behave differently in these, i.e. the tls configuration would have to be injected via the config

sorry for the late comment, can you extend the config and possibly the test, so that it tests both cases, the DO/FORWARD and the Balancer

@tuhao1020
Copy link
Contributor Author

@ReneWerner87 Thanks!
tls config has been moved from the global configuration to proxy.Config and proxy.WithTlsConfig is reserved for proxy.Forward. I have run go test in my laptop and passed all test.

@ReneWerner87 ReneWerner87 merged commit 93dc33a into gofiber:master Jun 24, 2021
@welcome
Copy link

welcome bot commented Jun 24, 2021

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants