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
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9ce0e90
add certificate pinner method in HttpSender
iori-yja Mar 1, 2019
1d492c8
add senderConfiguration option to specify the endpoint certificate
iori-yja Mar 4, 2019
c99e96f
add unsafeNoopVerifier
iori-yja Mar 5, 2019
7a02c67
change server certificate pinning interfaces
iori-yja Mar 5, 2019
59b1b0f
xdock test
iori-yja Mar 7, 2019
9240b22
allow disabling certificate verification for integration test
iori-yja Mar 7, 2019
81b429a
add test-purpose CA key and enforce "verification-disabling" feature …
iori-yja Mar 7, 2019
264db82
fix crossdock checkstyle failure
iori-yja Mar 7, 2019
d5127ff
re-allow to disable certificate verification
iori-yja Mar 7, 2019
aec7e1e
fix two simple typoes involving with CertPinning configuration
iori-yja Mar 11, 2019
5e9e6e1
restrict and rename relaxation option of HTTPS cert verification in H…
iori-yja Mar 11, 2019
cdd4c79
remove debug code with HTTPS from HTTPSender
iori-yja Mar 11, 2019
9892e8d
add hand-check pinning verifier for self-signed certification without…
iori-yja Mar 11, 2019
9479bb3
crossdock fix (wait for nginx proxy that waits for collector)
iori-yja Mar 12, 2019
59342f1
fix crossdock test avoiding ambiguous volume behavior
iori-yja Mar 12, 2019
74f8ffb
Merge remote-tracking branch 'origin/master' into tls-http-thrift
iori-yja Mar 13, 2019
da96565
add test for HTTPSender's new feature (pinning)
iori-yja Mar 13, 2019
386b537
increase crossdock timeout
iori-yja Mar 13, 2019
a0607f4
refine comments and remove debug print as review in #602
iori-yja Mar 15, 2019
bc856fd
add more careful error handling in HttpSender
iori-yja Mar 20, 2019
8530278
print pins if pinning failed for self-signed server
iori-yja Mar 20, 2019
17b1e7b
add note for pre-generated, for-crossdock certificate and fix its conf
iori-yja Mar 20, 2019
b10ab02
Log SenderException in RemoteReporter, including Trust verification
iori-yja Mar 21, 2019
f45ed78
add a test "testReporterWithSenderException" for RemoteReporter
iori-yja Mar 21, 2019
55ac341
fix as review comments in pullrequestreview-232113179
iori-yja Jun 21, 2019
4e0a704
change a test case name setTLSCeritificatePinning -> sendPlainHttpWit…
iori-yja Jun 24, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions jaeger-core/src/main/java/io/jaegertracing/Configuration.java
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ public class Configuration {
*/
public static final String JAEGER_TRACEID_128BIT = JAEGER_PREFIX + "TRACEID_128BIT";

/**
* 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.


/**
* The supported trace context propagation formats.
*/
Expand Down Expand Up @@ -614,6 +620,11 @@ public static class SenderConfiguration {
*/
private String authPassword;

/**
* The comma-separated certificate list for the endpoint which is self-signed, for example
*/
private String[] serverCertificateHashes;

public SenderConfiguration() {
}

Expand Down Expand Up @@ -647,6 +658,13 @@ public SenderConfiguration withAuthPassword(String password) {
return this;
}

public SenderConfiguration withCertificatePinning(String certs) {
if (certs != null) {
this.serverCertificateHashes = certs.split(",");
}
return this;
}

/**
* Returns a sender if one was given when creating the configuration, or attempts to create a sender based on the
* configuration's state.
Expand All @@ -671,13 +689,15 @@ public static SenderConfiguration fromEnv() {
String authToken = getProperty(JAEGER_AUTH_TOKEN);
String authUsername = getProperty(JAEGER_USER);
String authPassword = getProperty(JAEGER_PASSWORD);
String certHashes = getProperty(JAEGER_TLS_CERTIFICATE_PINNING);

return new SenderConfiguration()
.withAgentHost(agentHost)
.withAgentPort(agentPort)
.withEndpoint(collectorEndpoint)
.withAuthToken(authToken)
.withAuthUsername(authUsername)
.withCertificatePinning(certHashes)
.withAuthPassword(authPassword);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ public void run() {
try {
command.execute();
} catch (SenderException e) {
log.warn("RemoteReporter failed to send:", e);
metrics.reporterFailure.inc(e.getDroppedSpanCount());
}
} catch (Exception e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public void clearProperties() throws NoSuchFieldException, IllegalAccessExceptio
System.clearProperty(Configuration.JAEGER_TAGS);
System.clearProperty(Configuration.JAEGER_ENDPOINT);
System.clearProperty(Configuration.JAEGER_AUTH_TOKEN);
System.clearProperty(Configuration.JAEGER_TLS_CERTIFICATE_PINNING);
System.clearProperty(Configuration.JAEGER_USER);
System.clearProperty(Configuration.JAEGER_PASSWORD);
System.clearProperty(Configuration.JAEGER_PROPAGATION);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import io.jaegertracing.internal.samplers.ConstSampler;
import io.jaegertracing.internal.senders.InMemorySender;
import io.jaegertracing.spi.Reporter;
import io.jaegertracing.spi.Sender;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CountDownLatch;
Expand Down Expand Up @@ -328,4 +329,56 @@ public int append(JaegerSpan span) {
private JaegerSpan newSpan() {
return tracer.buildSpan("x").start();
}

private static class FailingSender implements Sender {
private static boolean invoked = false;

@Override
public int append(JaegerSpan span) throws SenderException {
invoked = true;
throw new SenderException("FailingSender is to fail", 1);
}

@Override
public int flush() throws SenderException {
throw new SenderException("FailingSender is to fail", 1);
}

@Override
public int close() throws SenderException {
throw new SenderException("FailingSender is to fail", 1);
}

public boolean invoked() {
return invoked;
}
}

@Test
public void testReporterWithSenderException() {
FailingSender failingSender = new FailingSender();
Reporter failingReporter = new RemoteReporter.Builder()
.withSender(failingSender)
.withFlushInterval(flushInterval)
.withMaxQueueSize(maxQueueSize)
.withMetrics(metrics)
.build();
JaegerTracer failingTracer = new JaegerTracer.Builder("test-failing-reporter")
.withReporter(failingReporter)
.withSampler(new ConstSampler(true))
.withMetrics(metrics)
.build();
JaegerSpan span = failingTracer.buildSpan("raza").start();
failingReporter.report(span);
// do sleep until automatic flush happens on 'reporter'
// added 20ms on top of 'flushInterval' to avoid corner cases
await()
.with()
.pollInterval(1, TimeUnit.MILLISECONDS)
.atMost(flushInterval + 20, TimeUnit.MILLISECONDS)
.until(() -> failingSender.invoked());

assertEquals(true, failingSender.invoked());
assertEquals(1, metricsFactory.getCounter("jaeger_tracer_reporter_spans", "result=err"));
}
}
29 changes: 25 additions & 4 deletions jaeger-crossdock/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ services:
- go
- java-udp
- java-http
- java-https
- python
- nodejs
environment:
- WAIT_FOR=test_driver,go,java-udp,java-http,python,nodejs
- WAIT_FOR_TIMEOUT=60s
- WAIT_FOR=test_driver,go,java-udp,java-http,java-https,python,nodejs,jaeger-collector-https-proxy
- WAIT_FOR_TIMEOUT=90s

- CALL_TIMEOUT=60s
- CALL_TIMEOUT=90s

- AXIS_CLIENT=go
- AXIS_S1NAME=go,java-udp,python,nodejs
Expand All @@ -27,7 +28,7 @@ services:
- BEHAVIOR_TRACE=client,s1name,sampled,s2name,s2transport,s3name,s3transport

- AXIS_TESTDRIVER=test_driver
- AXIS_SERVICES=java-udp,java-http
- AXIS_SERVICES=java-udp,java-http,java-https

- BEHAVIOR_ENDTOEND=testdriver,services

Expand Down Expand Up @@ -64,11 +65,31 @@ services:
environment:
- SENDER=http

java-https:
build: .
ports:
- "8080-8082"
environment:
- SENDER=https
- COLLECTOR_HOST_PIN=sha256/n6Ovey/sJws9vKJpESmWyQf9Oocak9J51mmPKGm4S0E=
depends_on:
- jaeger-collector

jaeger-collector-https-proxy:
build: https-proxy
ports:
- "8080"
- "14443"
restart: on-failure
depends_on:
- jaeger-collector

test_driver:
image: jaegertracing/test-driver
depends_on:
- jaeger-query
- jaeger-collector
- jaeger-agent
- jaeger-collector-https-proxy
ports:
- "8080"
12 changes: 12 additions & 0 deletions jaeger-crossdock/https-proxy/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
FROM nginx:alpine

# Overwrite default configuration
ADD build/proxy.conf /etc/nginx/conf.d/default.conf
# Install generated certificates; run `gen.sh` first
ADD build/proxy.crt /etc/nginx/proxy.crt
ADD build/proxy.key /etc/nginx/proxy.key

EXPOSE 8080 14443

# Poll healthcheck port until it returns 2xx or 3xx.
CMD ["sh", "-c", "while ! { wget --spider -S http://jaeger-collector:14269; }; do echo waiting for healthcheck; sleep 1; done; nginx -g 'daemon off;'"]
10 changes: 10 additions & 0 deletions jaeger-crossdock/https-proxy/authority.crt
Original file line number Diff line number Diff line change
@@ -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))

MIIBbTCCARSgAwIBAgIJAJXCC2VdhrxqMAoGCCqGSM49BAMCMBIxEDAOBgNVBAMM
B1RFU1QgQ0EwHhcNMTkwMzA3MTQzODIwWhcNMjkwMzA3MTQzODIwWjASMRAwDgYD
VQQDDAdURVNUIENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEAhYnw9zYU0G3
VZ48nNlT5jAs096pX0zeHM/yxiJe+DS5Yj0EJXM/0A1Of2zqxbyJpaEIFcqTmTTy
TXCE7I6B6aNTMFEwHQYDVR0OBBYEFLXSxii6ZFvw427zs7ct/B+eHv2QMB8GA1Ud
IwQYMBaAFLXSxii6ZFvw427zs7ct/B+eHv2QMA8GA1UdEwEB/wQFMAMBAf8wCgYI
KoZIzj0EAwIDRwAwRAIgBrX7CX8zNoRLAZ48jGcqI8RuNlpkj0S+UShIQjwez3AC
IGuqhnGb9JZSiZmIQaYdSE6T/sQaX7iDnwEgKGMI8OB7
-----END CERTIFICATE-----
8 changes: 8 additions & 0 deletions jaeger-crossdock/https-proxy/authority.key
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
-----BEGIN EC PARAMETERS-----
BggqhkjOPQMBBw==
-----END EC PARAMETERS-----
-----BEGIN EC PRIVATE KEY-----
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.

45 changes: 45 additions & 0 deletions jaeger-crossdock/https-proxy/gen.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#!/usr/bin/env bash

set -e

if [[ $# -ne 2 ]]; then
echo usage: $0 '<proxy-name> <forward-target>'
exit 1
fi

SERVER=$1
FORWARD=$2

# Authority.crt & authority.key is pre-generated with hash code of sha256/n6Ovey/sJws9vKJpESmWyQf9Oocak9J51mmPKGm4S0E=
# About this file:
# CN: TEST CA
# Algorithm: ECDSA-SHA256
# Validity:
# Not Before: Mar 7 14:38:20 2019 GMT
# Not After: Mar 7 14:38:20 2029 GMT
#
# To generate key: (not needed for cert renewal)
# openssl ecparam -genkey -name prime256v1 -out authority.key
#
# To regenerate:
# openssl req -new -sha256 -key authority.key -out authority.csr -subj "/CN=TEST CA/"
# openssl x509 -trustout -signkey authority.key -days 3652 -req -in authority.csr -out authority.crt
#
# To check pin:
# openssl ec -in authority.key -outform der -pubout | openssl dgst -sha256 -binary | openssl enc -base64
AUTHORITY=authority
TARGET=build/proxy

# generate secrets
cd $(dirname $0)
mkdir -p ./build
openssl ecparam -genkey -name prime256v1 -out ${TARGET}.key
openssl req -new -sha256 -key ${TARGET}.key -out ${TARGET}.csr -subj \
"/CN=${SERVER}/"
openssl x509 -req -sha256 -days 1 -CA ${AUTHORITY}.crt -CAkey ${AUTHORITY}.key -CAcreateserial -in ${TARGET}.csr -out ${TARGET}.crt
chmod 644 ${TARGET}.key

cat ${AUTHORITY}.crt >> ${TARGET}.crt

# generate nginx settings
SERVER=${SERVER} FORWARD=${FORWARD} envsubst < "proxy.template.conf" > ${TARGET}.conf
24 changes: 24 additions & 0 deletions jaeger-crossdock/https-proxy/proxy.template.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
server {
listen 8080;
server_name ${SERVER};
location / {
root /usr/share/nginx/html/;
index index.html;
}
}

server {
listen 14443 ssl http2;
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;
}

ssl_session_cache shared:SSL:10m;
ssl_certificate /etc/nginx/proxy.crt;
ssl_certificate_key /etc/nginx/proxy.key;
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";

location / {
proxy_pass http://${FORWARD};
}
}
14 changes: 9 additions & 5 deletions jaeger-crossdock/rules.mk
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ JAEGER_COMPOSE_URL=https://raw.githubusercontent.com/jaegertracing/jaeger/master
XDOCK_JAEGER_YAML=$(PROJECT)/jaeger-docker-compose.yml

.PHONY: crossdock
crossdock: gradle-compile crossdock-download-jaeger
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) kill java-udp java-http
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) rm -f java-udp java-http
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) build java-udp java-http
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

docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) run crossdock

.PHONY: crossdock-fresh
crossdock-fresh: gradle-compile crossdock-download-jaeger
crossdock-fresh: gradle-compile crossdock-proxy-secret-gen crossdock-download-jaeger
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) down --rmi all
docker-compose -f $(XDOCK_YAML) -f $(XDOCK_JAEGER_YAML) run crossdock

Expand All @@ -30,3 +30,7 @@ crossdock-clean:
.PHONY: crossdock-download-jaeger
crossdock-download-jaeger:
curl -o $(XDOCK_JAEGER_YAML) $(JAEGER_COMPOSE_URL)

.PHONY: crossdock-proxy-secret-gen
crossdock-proxy-secret-gen:
$(PROJECT)/https-proxy/gen.sh jaeger-collector-https-proxy jaeger-collector:14268
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class JerseyServer {
private static final String SAMPLING_HOST_PORT = "SAMPLING_HOST_PORT";
private static final String AGENT_HOST = "AGENT_HOST";
private static final String COLLECTOR_HOST_PORT = "COLLECTOR_HOST_PORT";
private static final String COLLECTOR_HOST_PIN = "COLLECTOR_HOST_PIN";

// TODO should not be static, should be final
public static Client client;
Expand Down Expand Up @@ -125,6 +126,8 @@ public static void main(String[] args) throws Exception {
new EndToEndBehaviorResource(new EndToEndBehavior(getEvn(SAMPLING_HOST_PORT, "jaeger-agent:5778"),
"crossdock-" + serviceName,
senderFromEnv(getEvn(COLLECTOR_HOST_PORT, "jaeger-collector:14268"),
getEvn(COLLECTOR_HOST_PORT, "jaeger-collector-https-proxy:14443"),
getEvn(COLLECTOR_HOST_PIN, "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="),
getEvn(AGENT_HOST, "jaeger-agent")))),
new HealthResource()));

Expand All @@ -141,17 +144,23 @@ private static String getEvn(String envName, String defaultValue) {
return env;
}

private static Sender senderFromEnv(String collectorHostPort, String agentHost) {
private static Sender senderFromEnv(String collectorHostPort,
String collectorHttpsHostPort, String collectorHttpsPin, String agentHost) {
String senderEnvVar = System.getenv(Constants.ENV_PROP_SENDER_TYPE);
if ("http".equalsIgnoreCase(senderEnvVar)) {
return new HttpSender.Builder(String.format("http://%s/api/traces", collectorHostPort))
.build();
} else if ("https".equalsIgnoreCase(senderEnvVar)) {
return new HttpSender.Builder(String.format("https://%s/api/traces", collectorHttpsHostPort))
.withCertificatePinning(new String[] {collectorHttpsPin})
.acceptSelfSigned()
.build();
} else if ("udp".equalsIgnoreCase(senderEnvVar) || senderEnvVar == null || senderEnvVar.isEmpty()) {
return new UdpSender(agentHost, 0, 0);
}

throw new IllegalStateException("Env variable " + Constants.ENV_PROP_SENDER_TYPE
+ ", is not valid, choose 'udp' or 'http'");
+ ", is not valid, choose 'udp', 'http' or 'https'");
}

private static String serviceNameFromEnv() {
Expand Down
Loading