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 19 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 @@ -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
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.

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

set -e

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

SERVER=$1
FORWARD=$2
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
20 changes: 20 additions & 0 deletions jaeger-crossdock/https-proxy/proxy.template.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
server {
listen 8080;
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

}
}

server {
listen 14443 ssl;
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved
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_certificate /etc/nginx/proxy.crt;
ssl_certificate_key /etc/nginx/proxy.key;
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