Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

HTTPS Sender #602

Closed
wants to merge 26 commits into from
Closed

HTTPS Sender #602

wants to merge 26 commits into from

Conversation

iori-yja
Copy link

@iori-yja iori-yja commented Mar 7, 2019

Which problem is this PR solving?

Short description of the changes

Added those below:

  • Optional Certificate Pinning
  • Optional Certificate verification disabling (used only with pinning)
  • Integration test over HTTPS

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
…with Pinning

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
@iori-yja iori-yja mentioned this pull request Mar 7, 2019
@jpkrohling
Copy link
Collaborator

jpkrohling commented Mar 7, 2019

Looks like it failed the checkstyle:

[ant:checkstyle] [ERROR] /home/travis/build/jaegertracing/jaeger-client-java/jaeger-crossdock/src/main/java/io/jaegertracing/crossdock/JerseyServer.java:147: Line is longer than 120 characters (found 140). [LineLength]

https://travis-ci.org/jaegertracing/jaeger-client-java/jobs/503139012#L2534

Signed-off-by: Iori YONEJI <fivo.11235813@gmail.com>
Signed-off-by: Iori YONEJI <fivo.11235813@gmail.com>
@iori-yja
Copy link
Author

iori-yja commented Mar 7, 2019

Oh, that was too stupid. I believe it's fixed now.
Also, crossdock check is still broken. It turns out that the cause is Bad Gateway on call to jaeger-collector from its https-proxy, but it seems weird...

@yurishkuro
Copy link
Member

Also, crossdock check is still broken. It turns out that the cause is Bad Gateway on call to jaeger-collector from its https-proxy, but it seems weird...

if you found this in the build logs, could you copy them here? Many PRs have been seeing crossdock failures recently, I suspect a common cause.

@iori-yja
Copy link
Author

I've added an nginx proxy in the docker-compose.yml. This service's log shows no route to host(pasted below).
For debug purpose, I added host-port mapping (14443:14443) as in the current diff and saw Bad Gateway response from localhost:14443 with my browser (chromium, curl) on the host machine.

% docker-compose -f jaeger-crossdock/docker-compose.yml -f jaeger-crossdock/jaeger-docker-compose.yml logs jaeger-collector-https-proxy
Attaching to jaeger-crossdock_jaeger-collector-https-proxy_1
jaeger-collector-https-proxy_1  | 2019/03/10 11:55:40 [error] 6#6: *1 connect() failed (113: No route to host) while connecting to upstream, client: 192.168.64.8, server: jaeger-collector-https-proxy, request: "POST /api/traces?format=jaeger.thrift HTTP/1.1", upstream: "http://192.168.64.9:14268/api/traces?format=jaeger.thrift", host: "jaeger-collector-https-proxy:14443"
jaeger-collector-https-proxy_1  | 192.168.64.8 - - [10/Mar/2019:11:55:40 +0000] "POST /api/traces?format=jaeger.thrift HTTP/1.1" 502 157 "-" "okhttp/3.9.0" "-"

I don't think the network problem is caused by reporter misconfiguration (as I changed it) because it succeeded to establish a session to the proxy, and also I don't think collector's issue because plain http version (java-http) is able to report to it.

This failure happens very likely but merely it succeeds, if I remember correctly (I couldn't reproduce now).

@iori-yja
Copy link
Author

I tried once again which was successful this time (no change).

% make crossdock-clean && make
(snip)
2019/03/10 15:57:22 HTTP: HEAD http://test_driver:8080/
HTTP: Service test_driver:8080 is up

All services are up after 15.10839437s!

Executing Matrix...

...................................................................................................................................

131/131 passed (0/131 skipped)

Tests passed!

% git diff
% docker-compose -f jaeger-crossdock/docker-compose.yml -f jaeger-crossdock/jaeger-docker-compose.yml logs jaeger-collector-https-proxy
Attaching to jaeger-crossdock_jaeger-collector-https-proxy_1
jaeger-collector-https-proxy_1  | 192.168.80.8 - - [10/Mar/2019:15:57:25 +0000] "POST /api/traces?format=jaeger.thrift HTTP/1.1" 202 0 "-" "okhttp/3.9.0" "-"

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

Merging #602 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #602      +/-   ##
============================================
+ Coverage     89.53%   89.66%   +0.12%     
- Complexity      542      563      +21     
============================================
  Files            68       69       +1     
  Lines          1949     2070     +121     
  Branches        251      263      +12     
============================================
+ Hits           1745     1856     +111     
- Misses          129      133       +4     
- Partials         75       81       +6     
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/io/jaegertracing/Configuration.java 93.33% <100.00%> (-2.18%) 44.00 <0.00> (ø)
...egertracing/internal/reporters/RemoteReporter.java 85.05% <100.00%> (+2.49%) 7.00 <0.00> (ø)
...rtracing/internal/reporters/CompositeReporter.java 71.42% <0.00%> (-28.58%) 6.00% <0.00%> (-1.00%)
...gertracing/internal/reporters/LoggingReporter.java 81.81% <0.00%> (-9.10%) 4.00% <0.00%> (-1.00%)
...a/io/jaegertracing/internal/utils/RateLimiter.java 94.73% <0.00%> (-5.27%) 5.00% <0.00%> (ø%)
...ernal/baggage/RemoteBaggageRestrictionManager.java 94.91% <0.00%> (-5.09%) 11.00% <0.00%> (+1.00%) ⬇️
...n/java/io/jaegertracing/internal/JaegerTracer.java 88.96% <0.00%> (-0.05%) 26.00% <0.00%> (ø%)
...aegertracing/internal/propagation/BinaryCodec.java 100.00% <0.00%> (ø) 17.00% <0.00%> (?%)
...ain/java/io/jaegertracing/internal/JaegerSpan.java 93.51% <0.00%> (+0.06%) 48.00% <0.00%> (+1.00%)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb0fa21...4e0a704. Read the comment docs.

yurishkuro referenced this pull request in jaegertracing/jaeger Mar 11, 2019
* Make grpc reporter default and add retry

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Polish

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Fix port

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Polish

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Use higher retry

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Increase retry to 100

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
* TLS certificates (in comma-separated BASE64 SHA256 Hash) for certificates pinning,
* used in case of HTTPS communication to the endpoint.
*/
public static final String JAEGER_TLS_CERTIFICATE_PINNING = JAEGER_PREFIX + "TLS_CERTIFICATE_PINNING";
Copy link
Member

Choose a reason for hiding this comment

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

Not thrilled with the name. Is it typical to pass certs via env vars? We've recently added many TLS options to Jaeger backend, and the certs are always passed via files.

Copy link
Author

Choose a reason for hiding this comment

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

I had three reasons to pass this via environment variable over file.

  1. It should be good to be consistent with other reporter configurations most of them can be done via fromEnv() otherwise manipulated with Internal*.
  2. This must be SHA256 Hash of Certificate or Public Key (https://square.github.io/okhttp/3.x/okhttp/okhttp3/CertificatePinner.html) so this is kept short.
  3. In Lambda, one of the motivations for this, managing environment variables is easier than doing so for file. Environment variables are shown and settable plainly in the console, while files are to be uploaded directly or indirectly (S3) to the console as zipped format.

…TTPSender

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
… any trust anchor

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori YONEJI <fivo.11235813@gmail.com>
@iori-yja iori-yja force-pushed the tls-http-thrift branch 5 times, most recently from ed82e0d to 175fa1d Compare March 13, 2019 03:56
Signed-off-by: Iori YONEJI <fivo.11235813@gmail.com>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
@yurishkuro
Copy link
Member

I feel like this is adding way too much code to the client.

- jaeger-collector

jaeger-collector-https-proxy:
image: nginx:alpine
Copy link
Member

Choose a reason for hiding this comment

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

it would be better to support TLS in the collector directly, rather than use nginx, since we already added similar functionality to gRPC:

      --collector.grpc.tls                              Enable TLS
      --collector.grpc.tls.cert string                  Path to TLS certificate file
      --collector.grpc.tls.key string                   Path to TLS key file
      --collector.http-port int                         The HTTP port for the collector service (default 14268)

Copy link
Author

Choose a reason for hiding this comment

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

This is nice to have for jaeger, and I may add the option to jaeger soon, but this proxy is for client test for now. I will replace this after --collector.http.tls options are merged.

ADD build/proxy.key /etc/nginx/proxy.key

EXPOSE 8080 14443
CMD ["sh", "-c", "while ! { wget --spider -S http://jaeger-collector:14269; }; do echo waiting...; sleep 1; done; nginx -g 'daemon off;'"]
Copy link
Member

Choose a reason for hiding this comment

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

comment would be useful, what is being queried on port 14269?

Copy link
Author

Choose a reason for hiding this comment

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

added.

crossdock: gradle-compile crossdock-proxy-secret-gen crossdock-download-jaeger
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) kill java-udp java-http java-https
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) rm -f java-udp java-http java-https
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) build java-udp java-http java-https
Copy link
Member

Choose a reason for hiding this comment

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

  • should this also include jaeger-collector-https-proxy ?
  • I would move these into a make var, to DRY

MHcCAQEEIAA24MB8sxrLG1an0nG1DCH6J32iqrtborxFOjdqWNCmoAoGCCqGSM49
AwEHoUQDQgAEAhYnw9zYU0G3VZ48nNlT5jAs096pX0zeHM/yxiJe+DS5Yj0EJXM/
0A1Of2zqxbyJpaEIFcqTmTTyTXCE7I6B6Q==
-----END EC PRIVATE KEY-----
Copy link
Member

Choose a reason for hiding this comment

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

are these being generated by jaeger-crossdock/https-proxy/gen.sh every time?

Copy link
Author

Choose a reason for hiding this comment

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

No, this is root certificate which is used to generate proxy's certificate by gen.sh. Hard-coding is ugly by the way, but sometimes test project includes.

The Public key pin is coded here:
https://github.com/jaegertracing/jaeger-client-java/pull/602/files#diff-9d280e9b01ffb0fc60647fe5d2a8286cR74

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please document somewhere how you generated these files?

Copy link
Author

Choose a reason for hiding this comment

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

OK, I will.

@@ -66,6 +77,7 @@ public void send(Process process, List<Span> spans) throws SenderException {
try {
response = httpClient.newCall(request).execute();
} catch (IOException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

don't we have that elsewhere?

Copy link
Author

Choose a reason for hiding this comment

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

That was my mistake not to remove debug code. I will delete.

}

public Builder acceptSelfSigned() {
// This dangerous operation will only take effect if pinning is used.
Copy link
Member

Choose a reason for hiding this comment

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

method comment?

Copy link
Author

Choose a reason for hiding this comment

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

Not intended but should be.

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
@@ -0,0 +1,10 @@
-----BEGIN CERTIFICATE-----
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the expiration date for this one?

Copy link
Author

Choose a reason for hiding this comment

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

Noted in gen.sh

(#602 (comment))

MHcCAQEEIAA24MB8sxrLG1an0nG1DCH6J32iqrtborxFOjdqWNCmoAoGCCqGSM49
AwEHoUQDQgAEAhYnw9zYU0G3VZ48nNlT5jAs096pX0zeHM/yxiJe+DS5Yj0EJXM/
0A1Of2zqxbyJpaEIFcqTmTTyTXCE7I6B6Q==
-----END EC PRIVATE KEY-----
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please document somewhere how you generated these files?

server_name ${SERVER};
location / {
root /usr/share/nginx/html/;
index index.html;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The indentation here seems a bit off

jaeger-crossdock/https-proxy/proxy.template.conf Outdated Show resolved Hide resolved
listen 14443 ssl;
server_name ${SERVER};

ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is only for testing, but could we have a production-grade configuration here? Like, a secure set of ciphers and options like ssl_prefer_server_ciphers on. This way, we'd be testing with production settings, making sure that all involved components are up to modern standards.

For reference, this is what I used to have on a few servers of mine, which was "current" a couple of years ago:

server {
	listen [::]:443 ssl http2;
	listen 443 ssl http2;
	server_name acme.example.com;
	ssl_certificate /etc/pki/tls/certs/acme.example.com.bundle.cert;
	ssl_certificate_key /etc/pki/tls/private/acme.example.com.key;
	ssl_session_cache shared:SSL:10m;
	ssl_session_timeout 10m;
	ssl_protocols TLSv1 TLSv1.1 TLSv1.2; # probably needs an update to include TLSv1.3?
	ssl_prefer_server_ciphers on;
	ssl_ciphers "EECDH+ECDSA+AESGCM EECDH+aRSA+AESGCM EECDH+ECDSA+SHA384 EECDH+ECDSA+SHA256 EECDH+aRSA+SHA384 EECDH+aRSA+SHA256 EECDH !EDH+aRSA !RC4 !aNULL !eNULL !LOW !3DES !MD5 !EXP !PSK !SRP !DSS";
	add_header Strict-Transport-Security max-age=31536000;
	root /usr/share/nginx/html/acme.example.com;
}

* Instead, check the sha256 hash value by custom verifier. */
acceptSelfSigned(clientBuilder, hostname, pins);
}
} catch (Exception e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This deserves a log message, even if at debug level (but possibly at info or even warn, as this is called only once)

Copy link
Author

Choose a reason for hiding this comment

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

I afraid it isn't, because it is very similar to https://github.com/jaegertracing/jaeger-client-java/pull/602/files/a0607f48cd7370959b8f937a208fdc517a03b82f#diff-3330f8a6420c943a90a1b22b0abd0815R56. But it might be OK to fail earlier than build itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understood what you mean. The code you linked throw an exception, the code here is swallowing one. Instead of swallowing, we should log it at some level (info in my opinion).

Copy link
Author

Choose a reason for hiding this comment

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

The try above the catch is needed because of URI to be constructed by the endpoint which is supplied by the configuration. If the endpoint is invalid, building HttpSender at few lines later would fail. But I agree with that it shouldn't swallow all kind of Exception, so I changed it to catch only URISyntaxException and throw it earlier.

}
}
// TODO: For TOFU (trust on first use) usecase, print the chain
throw new CertificateException();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's anything wrong, it would be very hard to figure out what is wrong. I see the following possible error conditions:

  • Empty chain (bad configuration from the user's part)
  • No matches between the hostname and the subject CNs from the chain
  • The check failed

It would be good to have an exception thrown with the appropriate message, even if it's a bit more code.

Copy link
Author

@iori-yja iori-yja Mar 20, 2019

Choose a reason for hiding this comment

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

CertificateException is thrown when the certificate couldn't be verified by the TrustManager. If another (like default) manager might succeed the verification even after this manager failed, and if it eventually turned to be successful, the connection must be sane.

To follow this (strange?) interface, I throw CertificateException whenever this manager fails to verify the connection.

But probably other exception could be thrown to fail in case of "The check failed" but CN matches including server's chain was empty.

Copy link
Author

Choose a reason for hiding this comment

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

But probably other exception could be thrown to fail in case of "The check failed" but CN matches including server's chain was empty.

This is good for trust-on-first-use use-case to print actual pins of the server, because calculating the pin by openssl command is a little bothering.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for having different exceptions for the check failed case

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
@iori-yja iori-yja force-pushed the tls-http-thrift branch 2 times, most recently from a2daf6d to 1c3cb04 Compare March 21, 2019 11:10
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
@pavolloffay
Copy link
Member

@iori-yja is this ready from your side? @jpkrohling could you please check if your review has been fixed?

Copy link
Collaborator

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

This PR is missing some tests, but the actual code looks sane to me.

Tests I'd like to see:

  • By default, HTTPS URL should not accept self-signed certs
  • HTTPS URL succeeds when the server sends a self-signed cert and acceptSelfSigned() is used
  • HTTP + acceptSelfSigned() is noop (or has no bad consequences)

@iori-yja
Copy link
Author

iori-yja commented Jun 15, 2019

Sorry, forgot to check; I will catch up this week

Edit: will add some tests

iori-yja added a commit to iori-yja/jaeger-client-java that referenced this pull request Jun 21, 2019
jaegertracing#602 (review)
Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
@iori-yja
Copy link
Author

HTTPS URL succeeds when the server sends a self-signed cert and acceptSelfSigned() is used

java-https does this in crossdock tests.

By default, HTTPS URL should not accept self-signed certs

This is the difficult part. How can I set "must-fail" test in crossdock?

HTTP + acceptSelfSigned() is noop (or has no bad consequences)

setTLSCertificatePinning() test does this check, but the name may be misleading. I've changed it to sendPlainHttpWithCertificatePinning().

…hCertificatePinning

Signed-off-by: Iori Yoneji <yoneji_i@worksap.co.jp>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants