From 6b9bc13493def8828be0890eb42e63676c02e338 Mon Sep 17 00:00:00 2001 From: Ryan Michela Date: Sat, 2 Jun 2018 10:16:07 -0700 Subject: [PATCH 1/2] Minor XFCC refactoring --- .../contrib/xfcc/XForwardedClientCert.java | 35 +++---- .../grpc/contrib/xfcc/XfccParser.java | 96 ++++--------------- .../grpc/contrib/xfcc/XfccQuoteUtil.java | 90 +++++++++++++++++ .../grpc/contrib/xfcc/XfccMarshallerTest.java | 15 +++ .../grpc/contrib/xfcc/XfccParserTest.java | 8 ++ 5 files changed, 151 insertions(+), 93 deletions(-) create mode 100644 grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java diff --git a/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XForwardedClientCert.java b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XForwardedClientCert.java index cfd8efa2..67b69518 100644 --- a/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XForwardedClientCert.java +++ b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XForwardedClientCert.java @@ -14,11 +14,20 @@ import java.util.Collections; import java.util.List; +import static com.salesforce.grpc.contrib.xfcc.XfccQuoteUtil.*; + /** * x-forwarded-client-cert (XFCC) is a proxy header which indicates certificate information of part or all of the * clients or proxies that a request has flowed through, on its way from the client to the server. */ public final class XForwardedClientCert { + static final String XFCC_BY = "By"; + static final String XFCC_HASH = "Hash"; + static final String XFCC_SAN = "SAN"; + static final String XFCC_URI = "URI"; + static final String XFCC_DNS = "DNS"; + static final String XFCC_SUBJECT = "Subject"; + /** * The metadata key used to access any present {@link XForwardedClientCert} objects. */ @@ -89,33 +98,27 @@ public String getSubject() { public String toString() { List kvp = new ArrayList<>(); if (!by.isEmpty()) { - kvp.add("By=" + enquote(by)); + kvp.add(toKvp(XFCC_BY, enquote(by))); } if (!hash.isEmpty()) { - kvp.add("Hash=" + enquote(hash)); + kvp.add(toKvp(XFCC_HASH, enquote(hash))); } if (!sanUri.isEmpty()) { - kvp.add("URI=" + enquote(sanUri)); + kvp.add(toKvp(XFCC_URI, enquote(sanUri))); } - for (String dns : sanDns) { - kvp.add("DNS=" + enquote(dns)); + if (!sanDns.isEmpty()) { + for (String dns : sanDns) { + kvp.add(toKvp(XFCC_DNS, enquote(dns))); + } } if (!subject.isEmpty()) { - kvp.add("Subject=" + enquote(subject)); + kvp.add(toKvp(XFCC_SUBJECT, enquote(subject))); } return String.join(";", kvp); } - private String enquote(String value) { - // Escape inner quotes with \" - value = value.replace("\"", "\\\""); - - // Wrap in quotes if ,;= is present - if (value.contains(",") || value.contains(";") || value.contains("=")) { - value = "\"" + value + "\""; - } - - return value; + private String toKvp(String key, String value) { + return key + "=" + value; } } diff --git a/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccParser.java b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccParser.java index 8137987c..0ea4bf3f 100644 --- a/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccParser.java +++ b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccParser.java @@ -10,6 +10,9 @@ import java.util.ArrayList; import java.util.List; +import static com.salesforce.grpc.contrib.xfcc.XfccQuoteUtil.*; +import static com.salesforce.grpc.contrib.xfcc.XForwardedClientCert.*; + /** * {@code XfccParser} parses the {@code x-forwarded-client-cert} (XFCC) header populated by TLS-terminating * reverse proxies. @@ -27,25 +30,27 @@ static List parse(String header) { for (String element : quoteAwareSplit(header, ',')) { XForwardedClientCert cert = new XForwardedClientCert(); - List kvps = quoteAwareSplit(element, ';'); - for (String kvp : kvps) { - List l = quoteAwareSplit(kvp, '='); - - if (l.get(0).toLowerCase().equals("by")) { - cert.setBy(dequote(l.get(1))); + List substrings = quoteAwareSplit(element, ';'); + for (String substring : substrings) { + List kvp = quoteAwareSplit(substring, '='); + String key = kvp.get(0).toLowerCase(); + String value = kvp.get(1); + + if (key.equalsIgnoreCase(XFCC_BY)) { + cert.setBy(dequote(value)); } - if (l.get(0).toLowerCase().equals("hash")) { - cert.setHash(dequote(l.get(1))); + if (key.equalsIgnoreCase(XFCC_HASH)) { + cert.setHash(dequote(value)); } // Use "SAN:" instead of "URI:" for backward compatibility with previous mesh proxy releases. - if (l.get(0).toLowerCase().equals("san") || l.get(0).toLowerCase().equals("uri")) { - cert.setSanUri(dequote(l.get(1))); + if (key.equalsIgnoreCase(XFCC_SAN) || key.equalsIgnoreCase(XFCC_URI)) { + cert.setSanUri(dequote(value)); } - if (l.get(0).toLowerCase().equals("dns")) { - cert.addSanDns(dequote(l.get(1))); + if (key.equalsIgnoreCase(XFCC_DNS)) { + cert.addSanDns(dequote(value)); } - if (l.get(0).toLowerCase().equals("subject")) { - cert.setSubject(dequote(l.get(1))); + if (key.equalsIgnoreCase(XFCC_SUBJECT)) { + cert.setSubject(dequote(value)); } } certs.add(cert); @@ -53,67 +58,4 @@ static List parse(String header) { return certs; } - - // Break str into individual elements, splitting on delim (not in quotes) - private static List quoteAwareSplit(String str, char delim) { - boolean inQuotes = false; - boolean inEscape = false; - - List elements = new ArrayList<>(); - StringBuilder buffer = new StringBuilder(); - for (char c : str.toCharArray()) { - if (c == delim && !inQuotes) { - elements.add(buffer.toString()); - buffer = new StringBuilder(); - inEscape = false; - continue; - } - - if (c == '"') { - if (inQuotes) { - if (!inEscape) { - inQuotes = false; - } - } else { - inQuotes = true; - - } - inEscape = false; - buffer.append(c); - continue; - } - - if (c == '\\') { - if (!inEscape) { - inEscape = true; - buffer.append(c); - continue; - } - } - - // all other characters - inEscape = false; - buffer.append(c); - } - - if (inQuotes) { - throw new RuntimeException("Quoted string not closed"); - } - - elements.add(buffer.toString()); - - return elements; - } - - // Remove leading and tailing unescaped quotes, remove escaping from escaped internal quotes - private static String dequote(String str) { - str = str.replace("\\\"", "\""); - if (str.startsWith("\"")) { - str = str.substring(1); - } - if (str.endsWith("\"")) { - str = str.substring(0, str.length() - 1); - } - return str; - } } diff --git a/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java new file mode 100644 index 00000000..8fe6d9e6 --- /dev/null +++ b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java @@ -0,0 +1,90 @@ +package com.salesforce.grpc.contrib.xfcc; + +import java.util.ArrayList; +import java.util.List; + +final class XfccQuoteUtil { + private XfccQuoteUtil() { } + + /** + * Break str into individual elements, splitting on delim (not in quotes) + */ + static List quoteAwareSplit(String str, char delim) { + boolean inQuotes = false; + boolean inEscape = false; + + List elements = new ArrayList<>(); + StringBuilder buffer = new StringBuilder(); + for (char c : str.toCharArray()) { + if (c == delim && !inQuotes) { + elements.add(buffer.toString()); + buffer.setLength(0); // clear + inEscape = false; + continue; + } + + if (c == '"') { + if (inQuotes) { + if (!inEscape) { + inQuotes = false; + } + } else { + inQuotes = true; + + } + inEscape = false; + buffer.append(c); + continue; + } + + if (c == '\\') { + if (!inEscape) { + inEscape = true; + buffer.append(c); + continue; + } + } + + // all other characters + inEscape = false; + buffer.append(c); + } + + if (inQuotes) { + throw new RuntimeException("Quoted string not closed"); + } + + elements.add(buffer.toString()); + + return elements; + } + + /** + * Add escaping around double quote characters; wrap with quotes if special characters are present + */ + static String enquote(String value) { + // Escape inner quotes with \" + value = value.replace("\"", "\\\""); + + // Wrap in quotes if ,;= is present + if (value.contains(",") || value.contains(";") || value.contains("=")) { + value = "\"" + value + "\""; + } + + return value; + } + + /** + * Remove leading and tailing unescaped quotes; remove escaping from escaped internal quotes + */ + static String dequote(String str) { + str = str.replace("\\\"", "\""); + if (str.startsWith("\"")) { + str = str.substring(1); + } + if (str.endsWith("\"")) { + str = str.substring(0, str.length() - 1); + } + return str; + } +} diff --git a/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccMarshallerTest.java b/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccMarshallerTest.java index 0ca0381a..cb2e504c 100644 --- a/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccMarshallerTest.java +++ b/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccMarshallerTest.java @@ -12,6 +12,7 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class XfccMarshallerTest { @Test @@ -80,7 +81,14 @@ public void roundTripSimpleTest() { @Test public void roundTripUriAndDnsTest() { + XfccMarshaller marshaller = new XfccMarshaller(); + + String header = "By=http://frontend.lyft.com;Hash=468ed33be74eee6556d90c0149c1309e9ba61d6425303443c0748a02dd8de688;URI=http://testclient.lyft.com;DNS=lyft.com;DNS=www.lyft.com;Subject=\"/C=US/ST=CA/L=San Francisco/OU=Lyft/CN=Test Client\""; + + List certs = marshaller.parseAsciiString(header); + String serialized = marshaller.toAsciiString(certs); + assertThat(serialized).isEqualTo(header); } @Test @@ -121,4 +129,11 @@ public void roundTripEscapedQuotedTest() { assertThat(serialized).isEqualTo(header); } + + @Test + public void malformedHeaderThrows() { + XfccMarshaller marshaller = new XfccMarshaller(); + String header = "KABOOM!"; + assertThatThrownBy(() -> marshaller.parseAsciiString(header)).isInstanceOf(RuntimeException.class); + } } diff --git a/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccParserTest.java b/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccParserTest.java index 408273c6..3b4085a1 100644 --- a/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccParserTest.java +++ b/grpc-contrib/src/test/java/com/salesforce/grpc/contrib/xfcc/XfccParserTest.java @@ -107,4 +107,12 @@ public void mismatchedQuotesThrows() { assertThatThrownBy(() -> XfccParser.parse(header)).isInstanceOf(RuntimeException.class); } + + @Test + public void quotedKeyThrows() { + String header = "\"By\"=http://frontend.lyft.com;Hash=468ed33be74eee6556d90c0149c1309e9ba61d6425303443c0748a02dd8de688;" + + "Subject=\"/C=US/ST=CA/L=\\\"San Francisco\"/OU=Lyft/CN=Test Client\";SAN=http://testclient.lyft.com"; + + assertThatThrownBy(() -> XfccParser.parse(header)).isInstanceOf(RuntimeException.class); + } } From e0c519fbca335b72d779ec9b3033b5055e11890b Mon Sep 17 00:00:00 2001 From: Ryan Michela Date: Sat, 2 Jun 2018 10:19:48 -0700 Subject: [PATCH 2/2] Checkstyle --- .../grpc/contrib/xfcc/XfccQuoteUtil.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java index 8fe6d9e6..fbca7aca 100644 --- a/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java +++ b/grpc-contrib/src/main/java/com/salesforce/grpc/contrib/xfcc/XfccQuoteUtil.java @@ -1,13 +1,22 @@ +/* + * Copyright (c) 2017, salesforce.com, inc. + * All rights reserved. + * Licensed under the BSD 3-Clause license. + * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ package com.salesforce.grpc.contrib.xfcc; import java.util.ArrayList; import java.util.List; +/** + * Utility methods for quote escaping in XFCC headers. + */ final class XfccQuoteUtil { private XfccQuoteUtil() { } /** - * Break str into individual elements, splitting on delim (not in quotes) + * Break str into individual elements, splitting on delim (not in quotes). */ static List quoteAwareSplit(String str, char delim) { boolean inQuotes = false; @@ -60,7 +69,7 @@ static List quoteAwareSplit(String str, char delim) { } /** - * Add escaping around double quote characters; wrap with quotes if special characters are present + * Add escaping around double quote characters; wrap with quotes if special characters are present. */ static String enquote(String value) { // Escape inner quotes with \" @@ -75,7 +84,7 @@ static String enquote(String value) { } /** - * Remove leading and tailing unescaped quotes; remove escaping from escaped internal quotes + * Remove leading and tailing unescaped quotes; remove escaping from escaped internal quotes. */ static String dequote(String str) { str = str.replace("\\\"", "\"");