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 10 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.
Jump to
Jump to file
Failed to load files.
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
29 changes: 27 additions & 2 deletions jaeger-crossdock/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ services:
- go
- java-udp
- java-http
- java-https
- python
- nodejs
environment:
- WAIT_FOR=test_driver,go,java-udp,java-http,python,nodejs
- WAIT_FOR=test_driver,go,java-udp,java-http,java-https,python,nodejs
- WAIT_FOR_TIMEOUT=60s

- CALL_TIMEOUT=60s
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,35 @@ 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:
image: nginx
ports:
- "14443:14443"
volumes:
- ./https-proxy/build/proxy.conf:/etc/nginx/conf.d/proxy.conf
- ./https-proxy/build/proxy.crt:/etc/nginx/proxy.crt
- ./https-proxy/build/proxy.key:/etc/nginx/proxy.key
restart: on-failure
depends_on:
- jaeger-collector
command: ["nginx", "-g", "daemon off;"]

test_driver:
image: jaegertracing/test-driver
depends_on:
- jaeger-query
- jaeger-collector
- jaeger-agent
- jaeger-collector-https-proxy
ports:
- "8080"
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.

24 changes: 24 additions & 0 deletions jaeger-crossdock/https-proxy/gen.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#!/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

# generate nginx settings
SERVER=${SERVER} FORWARD=${FORWARD} envsubst < "proxy.template.conf" > build/proxy.conf
11 changes: 11 additions & 0 deletions jaeger-crossdock/https-proxy/proxy.template.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
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};
}
}
12 changes: 8 additions & 4 deletions jaeger-crossdock/rules.mk
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ 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
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})
.disableCertVerification()
.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
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,16 @@
import io.jaegertracing.thriftjava.Process;
import io.jaegertracing.thriftjava.Span;
import java.io.IOException;
import java.net.URI;
import java.util.List;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.X509TrustManager;
import javax.net.ssl.TrustManager;
import lombok.ToString;
import okhttp3.CertificatePinner;
import okhttp3.Credentials;
import okhttp3.HttpUrl;
import okhttp3.Interceptor;
Expand All @@ -46,6 +54,7 @@ protected HttpSender(Builder builder) {
if (collectorUrl == null) {
throw new IllegalArgumentException("Could not parse url.");
}

this.httpClient = builder.clientBuilder.build();
this.requestBuilder = new Request.Builder().url(collectorUrl);
}
Expand Down Expand Up @@ -88,6 +97,9 @@ public void send(Process process, List<Span> spans) throws SenderException {

public static class Builder {
private final String endpoint;
private CertificatePinner.Builder certificatePinnerBuilder = new CertificatePinner.Builder();
private boolean pinning;
private boolean disableVerification = false;
private int maxPacketSize = ONE_MB_IN_BYTES;
private Interceptor authInterceptor;
private OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder();
Expand Down Expand Up @@ -119,13 +131,61 @@ public Builder withAuth(String authToken) {
return this;
}

public Builder withCertificatePinning(String sha256certs[] /* comma separated */) {
String hostname;
try {
final URI uri = new URI(endpoint);
if ("https".equals(uri.getScheme())) {
hostname = uri.getHost();
this.pinning = true;
for (String cert: sha256certs) {
certificatePinnerBuilder.add(hostname, String.format("sha256/%s", cert));
}
}
} finally {
return this;
}
}

public Builder disableCertVerification() {
/* This dangerous operation will only take effect if pinning is used. */
this.disableVerification = true;
return this;
}

public HttpSender build() {
if (authInterceptor != null) {
clientBuilder.addInterceptor(authInterceptor);
}
if (pinning) {
clientBuilder.certificatePinner(certificatePinnerBuilder.build());
}
if (disableVerification) {
disableCertVerification(clientBuilder);
}
return new HttpSender(this);
}

private void disableCertVerification(OkHttpClient.Builder clientBuilder) {
try {
final TrustManager[] unsafeNoopVerificator = new TrustManager[] {
new X509TrustManager() {
@Override public void checkServerTrusted(java.security.cert.X509Certificate[] chain, String authType) throws CertificateException {}
@Override public void checkClientTrusted(java.security.cert.X509Certificate[] chain, String authType) throws CertificateException {}
@Override public java.security.cert.X509Certificate[] getAcceptedIssuers() {
return new X509Certificate[0];
}
}
};
final SSLContext sslContext = SSLContext.getInstance("TLS");
sslContext.init(null, unsafeNoopVerificator, null);
clientBuilder.sslSocketFactory(sslContext.getSocketFactory(), (X509TrustManager) unsafeNoopVerificator[0]);

} catch (Exception e) {
throw new RuntimeException(e);
}
}

private Interceptor getAuthInterceptor(final String headerValue) {
return new Interceptor() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ public Sender getSender(Configuration.SenderConfiguration conf) {
httpSenderBuilder.withAuth(conf.getAuthToken());
}

if (null != conf.getServerCertificateHashes() && 0 != conf.getServerCertificateHashes().length) {
httpSenderBuilder.withCertificatePinning(conf.getServerCertificateHashes());
}

log.debug("Using the HTTP Sender to send spans directly to the endpoint.");
return httpSenderBuilder.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public void reset() {
System.clearProperty(Configuration.JAEGER_AUTH_TOKEN);
System.clearProperty(Configuration.JAEGER_USER);
System.clearProperty(Configuration.JAEGER_PASSWORD);
System.clearProperty(Configuration.JAEGER_TLS_CERTIFICATE_PINNING);
}

@Override
Expand Down