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

Support HTTPS url for -Dcsp.sentinel.dashboard.server attribute #1896

Merged
merged 5 commits into from
Dec 16, 2020
Merged

Support HTTPS url for -Dcsp.sentinel.dashboard.server attribute #1896

merged 5 commits into from
Dec 16, 2020

Conversation

polarbear567
Copy link
Contributor

Does this pull request fix one issue?

Fixes #1840

Describe how you did it

  1. The involved HTTPS request is only heartbeat transmission, so this PR takes the operation of trusting all certificates
  2. Because the dashboard does not have a certificate configured by default, this modification does not involve the dashboard. The dashboard is a spring-boot project. If you need to use HTTPS, you need to configure it yourself

Describe how to verify it

First we verify the https protocol:

  1. Add ssl config in dashboard application.properties, like this one:

server.ssl.key-store=xxx/yyy.keystore
server.ssl.key-store-password=zzz
server.ssl.keyStoreType=JKS
server.ssl.keyAlias=aaa

I used keytool to generate certificates
2. Start a project that uses the modified jar with -Dcsp.sentinel.dashboard.server=https://127.0.0.1:8080
3. Start the dashboard jar
4. Use browser access https://127.0.0.1:8080, we can see that the dashboard has been able to receive the data of demo
Second, we verify the http protocol

  1. Start a project that uses the modified jar with -Dcsp.sentinel.dashboard.server=http://127.0.0.1:8080 or -Dcsp.sentinel.dashboard.server=127.0.0.1:8080
  2. Start the dashboard jar without ssl config
  3. Use browser access http://127.0.0.1:8080, also we can see the client.

Special notes for reviews

1.Is it necessary to set a certificate for the heartbeat of https?
2.Whether it is necessary to set parameters for the dashboard so that the https protocol can be used (because users may directly use the jar package instead of compiling by themselves)

@sczyh30 sczyh30 added kind/enhancement Category issues or prs related to enhancement. to-review To review labels Dec 14, 2020
jasonjoo2010
jasonjoo2010 requested changes Dec 14, 2020
View reviewed changes
@polarbear567
Copy link
Contributor Author

Hi @jasonjoo2010 ,
Please review again, and tell me if you have any suggestions.

@polarbear567
Copy link
Contributor Author

Hi @jasonjoo2010 ,
It has been optimized according to your suggestions, supported netty http, and verified that the modified code is valid for both http and https. Please review.

@jasonjoo2010
Copy link
Collaborator

Hi @jasonjoo2010 ,
It has been optimized according to your suggestions, supported netty http, and verified that the modified code is valid for both http and https. Please review.

Before reviewing the integration tests failed. You can check and fix it parallelly.

@polarbear567
Copy link
Contributor Author

Hi @jasonjoo2010 ,
It has been optimized according to your suggestions, supported netty http, and verified that the modified code is valid for both http and https. Please review.

Before reviewing the integration tests failed. You can check and fix it parallelly.

Work fine now.

jasonjoo2010
jasonjoo2010 requested changes Dec 15, 2020
View reviewed changes
...tp/src/main/java/com/alibaba/csp/sentinel/transport/heartbeat/client/HttpClientsFactory.java
public class HttpClientsFactory {

private static class SslConnectionSocketFactoryInstance {
private static final SSLConnectionSocketFactory SSL_CONNECTION_SOCKET_FACTORY = new SSLConnectionSocketFactory(SslFactory.getSslConnectionSocketFactory(), NoopHostnameVerifier.INSTANCE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @sczyh30

Is bypass certificate verifying acceptable?
I don't insist on it.

jasonjoo2010
jasonjoo2010 approved these changes Dec 15, 2020
View reviewed changes
Copy link
Collaborator

@jasonjoo2010 jasonjoo2010 left a comment

Choose a reason for hiding this comment

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

LGTM.

@sczyh30 Do you have any concerns about the bypass certificate verification approach?

@jasonjoo2010 jasonjoo2010 merged commit 82826fa into alibaba:master Dec 16, 2020
@jasonjoo2010
Copy link
Collaborator

Thanks for contributing!

@sczyh30 sczyh30 removed the to-review To review label Dec 16, 2020
hughpearse pushed a commit to hughpearse/Sentinel that referenced this pull request Jun 2, 2021
@polarbear567
Support both HTTP and HTTPS protocol heart beating to dashboard (alib…
40e7b9d 
…aba#1896)

Apply to both simple-http and netty-http modules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonjoo2010 jasonjoo2010 jasonjoo2010 approved these changes

@cdfive cdfive Awaiting requested review from cdfive

Assignees
No one assigned
Labels
kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support HTTPS url for -Dcsp.sentinel.dashboard.server attribute
3 participants
@polarbear567 @jasonjoo2010 @sczyh30