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

feat(start): allow insecure https connections to remote api hosts #534

Closed
wants to merge 1 commit into from

Conversation

danwatford
Copy link
Contributor

Allow insecure connections to remote api hosts to handle cases where remote TLS host presents a wildcard certificate.

Change the host header in proxied requests from localhost to match the remote api host.

Relates to #523

Allow insecure connections to remote api hosts to handle cases where remote TLS host presents a wildcard certificate.

Change the host header in proxied requests from localhost to match the remote api host.

Relates to Azure#523
@github-actions github-actions bot added the scope: core Issues happened a the ./src/core level label Jul 6, 2022
@sinedied
Copy link
Member

Hey @danwatford , thanks for your contribution!

I was thinking that maybe this could be something that could be always activated as it's a proxy to a dev server anyways? @manekinekko what do you think?

@manekinekko
Copy link
Member

Hey @danwatford , thanks for your contribution!

I was thinking that maybe this could be something that could be always activated as it's a proxy to a dev server anyways? @manekinekko what do you think?

We are going to be adding support for /api proxying to non-Azure Function APIs as well (AS, ACA, AKS...). I am not sure how this option will affect these remote connections, so I guess we can still provide this flag to users, but have it be false by default.

@danwatford have you tried using the --ssl option to enable SSL for local dev? Or this is not related?

@danwatford
Copy link
Contributor Author

@danwatford have you tried using the --ssl option to enable SSL for local dev? Or this is not related?

Hi @manekinekko
The --ssl option only affects serving of the SWA front-end. To enable the CLI to proxy to an API hosted in a GitHub codespaces debug session or in Azure Functions, we need to change the behaviour of the http-proxy node module.

Issue #534 mentions how http-proxy doesn't handle wildcard certificates from remote API hosts. This PR is a workaround in that http-proxy is (optionally) instructed to ignore any remote host certificate authentication.

Assuming the new support for /api proxying to other APIs hosted remotely will continue to use http-proxy, then I think you will run in to the same problem with wildcard certificate handling and may still want to consider applying this PR.

However if there are plans to replace http-proxy with something that can handle wildcard certificates then this PR should not be necessary.

@danwatford
Copy link
Contributor Author

Hey @danwatford , thanks for your contribution!

I was thinking that maybe this could be something that could be always activated as it's a proxy to a dev server anyways?

Hi @sinedied ,

It might be acceptable to leave the insecure connection option enabled by default, but if we did that then we should probably log a warning to the user that we have not authenticated the connection to the remote API server.

I don't know if we can assume users will always be proxying to a development server. My preference would be to ensure a secure connection by default, but log some guidance about using the insecure connection option if they have any connectivity issues.

@manekinekko
Copy link
Member

Assuming the new support for /api proxying to other APIs hosted remotely will continue to use http-proxy, then I think you will run in to the same problem with wildcard certificate handling and may still want to consider applying this PR.

This is the plan.

However if there are plans to replace http-proxy with something that can handle wildcard certificates then this PR should not be necessary.

There are no plan to replace http-proxy.

It might be acceptable to leave the insecure connection option enabled by default, but if we did that then we should probably log a warning to the user that we have not authenticated the connection to the remote API server.

In order to avoid any breaking changes or unexpected side effects, I'd recommend adding this flag but turning it off by default and allowing users to enable it on demand. cc @sinedied @sgollapudi77 @sulabh-msft

@manekinekko manekinekko added the status: need doc updates The documentation website need to be updated after this issue has been revolved label Sep 28, 2022
@manekinekko
Copy link
Member

@danwatford can you please update the corresponding documentation to add this flag, and also an usage example?

@danwatford
Copy link
Contributor Author

Some good(ish) news. While updating the documentation I wanted to capture the error text that users might see for problems connecting to remote API hosts, but couldn't recreate the problem that was intended to be solved by the --api-devserver-insecure command line option.

Testing against APIs accessed GitHub Codespaces public ports or via locally developed APIs accessed via Ngrok were no longer failing.

I tracked down the cause of the unintended successful connection to the changeOrigin: true option passed to http-proxy.

I think I had misinterpreted errors such as:

Hostname/IP does not match certificate's altnames: Host: localhost. is not in the cert's altnames: DNS:online.visualstudio.com, DNS:*.online.visualstudio.com, DNS:*.app.online.visualstudio.com, DNS:*.tunnels.vsengsaas.visualstudio.com

as http-proxy having issues with wildcard certificates. I had failed to notice the Host: localhost portion of the error message.

Conclusion: This PR can be closed and I will raise a new PR to apply the changeOrigin: true property for http-proxy and another PR to update the documentation on how to connect to a remote API server.

@danwatford danwatford closed this Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: core Issues happened a the ./src/core level status: need doc updates The documentation website need to be updated after this issue has been revolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants