Merge pull request #7768 from vespa-engine/bjorncs/fix

Bjorncs/fix

6 files changed, 158 insertions, 13 deletions

diff --git a/security-utils/pom.xml b/security-utils/pom.xml
index 7006a0f5f86..6f094f28362 100644
--- a/security-utils/pom.xml
+++ b/security-utils/pom.xml
@@ -43,6 +43,12 @@
       <artifactId>hamcrest-library</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>com.yahoo.vespa</groupId>
+      <artifactId>testutil</artifactId>
+      <version>${project.version}</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
   <build>
     <plugins>
diff --git a/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsEntity.java b/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsEntity.java
index 80ef06d9cac..fbb98d7c382 100644
--- a/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsEntity.java
+++ b/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsEntity.java
@@ -2,11 +2,13 @@
 package com.yahoo.security.tls.json;
 
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;
 
-import java.util.ArrayList;
 import java.util.List;
 
+import static com.fasterxml.jackson.annotation.JsonInclude.Include.NON_EMPTY;
+
 /**
  * Jackson bindings for transport security options
  *
@@ -16,7 +18,7 @@ import java.util.List;
 class TransportSecurityOptionsEntity {
 
     @JsonProperty("files") Files files;
-    @JsonProperty("authorized-peers") List<AuthorizedPeer> authorizedPeers = new ArrayList<>();
+    @JsonProperty("authorized-peers") @JsonInclude(NON_EMPTY) List<AuthorizedPeer> authorizedPeers;
 
     static class Files {
         @JsonProperty("private-key") String privateKeyFile;
@@ -25,9 +27,9 @@ class TransportSecurityOptionsEntity {
     }
 
     static class AuthorizedPeer {
-        @JsonProperty("required-credentials") List<RequiredCredential> requiredCredentials = new ArrayList<>();
+        @JsonProperty("required-credentials") List<RequiredCredential> requiredCredentials;
         @JsonProperty("name") String name;
-        @JsonProperty("roles") List<String> roles = new ArrayList<>();
+        @JsonProperty("roles") @JsonInclude(NON_EMPTY) List<String> roles;
     }
 
     static class RequiredCredential {
diff --git a/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializer.java b/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializer.java
index 2e2148628e8..f75cb4bcfff 100644
--- a/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializer.java
+++ b/security-utils/src/main/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializer.java
@@ -18,6 +18,8 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
 import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 
@@ -42,7 +44,7 @@ public class TransportSecurityOptionsJsonSerializer {
 
     public void serialize(OutputStream out, TransportSecurityOptions options) {
         try {
-            mapper.writeValue(out, toTransportSecurityOptionsEntity(options));
+            mapper.writerWithDefaultPrettyPrinter().writeValue(out, toTransportSecurityOptionsEntity(options));
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         }
@@ -62,7 +64,10 @@ public class TransportSecurityOptionsJsonSerializer {
             }
         }
         List<AuthorizedPeer> authorizedPeersEntity = entity.authorizedPeers;
-        if (authorizedPeersEntity.size() > 0) {
+        if (authorizedPeersEntity != null) {
+            if (authorizedPeersEntity.size() == 0) {
+                throw new IllegalArgumentException("'authorized-peers' cannot be empty");
+            }
             builder.withAuthorizedPeers(new AuthorizedPeers(toPeerPolicies(authorizedPeersEntity)));
         }
         return builder.build();
@@ -78,16 +83,14 @@ public class TransportSecurityOptionsJsonSerializer {
         if (authorizedPeer.name == null) {
             throw missingFieldException("name");
         }
-        if (authorizedPeer.requiredCredentials.isEmpty()) {
+        if (authorizedPeer.requiredCredentials == null) {
             throw missingFieldException("required-credentials");
         }
-        if (authorizedPeer.roles.isEmpty()) {
-            throw missingFieldException("roles");
-        }
         return new PeerPolicy(authorizedPeer.name, toRoles(authorizedPeer.roles), toRequestPeerCredentials(authorizedPeer.requiredCredentials));
     }
 
     private static Set<Role> toRoles(List<String> roles) {
+        if (roles == null) return Collections.emptySet();
         return roles.stream()
                 .map(Role::new)
                 .collect(toSet());
@@ -124,16 +127,21 @@ public class TransportSecurityOptionsJsonSerializer {
         options.getCertificatesFile().ifPresent(value -> entity.files.certificatesFile = value.toString());
         options.getPrivateKeyFile().ifPresent(value -> entity.files.privateKeyFile = value.toString());
         options.getAuthorizedPeers().ifPresent( authorizedPeers -> {
+            entity.authorizedPeers = new ArrayList<>();
             for (PeerPolicy peerPolicy : authorizedPeers.peerPolicies()) {
                 AuthorizedPeer authorizedPeer = new AuthorizedPeer();
                 authorizedPeer.name = peerPolicy.policyName();
+                authorizedPeer.requiredCredentials = new ArrayList<>();
                 for (RequiredPeerCredential requiredPeerCredential : peerPolicy.requiredCredentials()) {
                     RequiredCredential requiredCredential = new RequiredCredential();
                     requiredCredential.field = toField(requiredPeerCredential.field());
                     requiredCredential.matchExpression = requiredPeerCredential.pattern().asString();
                     authorizedPeer.requiredCredentials.add(requiredCredential);
                 }
-                peerPolicy.assumedRoles().forEach(role -> authorizedPeer.roles.add(role.name()));
+                if (!peerPolicy.assumedRoles().isEmpty()) {
+                    authorizedPeer.roles = new ArrayList<>();
+                    peerPolicy.assumedRoles().forEach(role -> authorizedPeer.roles.add(role.name()));
+                }
                 entity.authorizedPeers.add(authorizedPeer);
             }
         });
diff --git a/security-utils/src/main/java/com/yahoo/security/tls/policy/HostGlobPattern.java b/security-utils/src/main/java/com/yahoo/security/tls/policy/HostGlobPattern.java
index dbe500836e5..c7acf5dfbeb 100644
--- a/security-utils/src/main/java/com/yahoo/security/tls/policy/HostGlobPattern.java
+++ b/security-utils/src/main/java/com/yahoo/security/tls/policy/HostGlobPattern.java
@@ -2,6 +2,7 @@
 package com.yahoo.security.tls.policy;
 
 import java.util.Objects;
+import java.util.regex.Pattern;
 
 /**
  * @author bjorncs
@@ -9,15 +10,46 @@ import java.util.Objects;
 public class HostGlobPattern {
 
     private final String pattern;
+    private final Pattern regexPattern;
 
     public HostGlobPattern(String pattern) {
         this.pattern = pattern;
+        this.regexPattern = toRegexPattern(pattern);
     }
 
     public String asString() {
         return pattern;
     }
 
+    public boolean matches(String hostString) {
+        return regexPattern.matcher(hostString).matches();
+    }
+
+    private static Pattern toRegexPattern(String pattern) {
+        StringBuilder builder = new StringBuilder("^");
+        for (char c : pattern.toCharArray()) {
+            if (c == '*') {
+                // Note: we explicitly stop matching at a dot separator boundary.
+                // This is to make host name matching less vulnerable to dirty tricks.
+                builder.append("[^.]*");
+            } else if (c == '?') {
+                // Same applies for single chars; they should only match _within_ a dot boundary.
+                builder.append("[^.]");
+            } else if (isRegexMetaCharacter(c)){
+                builder.append("\\");
+                builder.append(c);
+            } else {
+                builder.append(c);
+            }
+        }
+        builder.append('$');
+        return Pattern.compile(builder.toString());
+    }
+
+    private static boolean isRegexMetaCharacter(char c) {
+        return "<([{\\^-=$!|]})?*+.>".indexOf(c) != -1; // note: includes '?' and '*'
+    }
+
     @Override
     public String toString() {
         return "HostGlobPattern{" +
diff --git a/security-utils/src/test/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializerTest.java b/security-utils/src/test/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializerTest.java
index c59647a02d0..5e611b1eba5 100644
--- a/security-utils/src/test/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializerTest.java
+++ b/security-utils/src/test/java/com/yahoo/security/tls/json/TransportSecurityOptionsJsonSerializerTest.java
@@ -7,24 +7,37 @@ import com.yahoo.security.tls.policy.HostGlobPattern;
 import com.yahoo.security.tls.policy.PeerPolicy;
 import com.yahoo.security.tls.policy.RequiredPeerCredential;
 import com.yahoo.security.tls.policy.Role;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
 
-import static com.yahoo.security.tls.policy.RequiredPeerCredential.Field.*;
+import static com.yahoo.security.tls.policy.RequiredPeerCredential.Field.CN;
+import static com.yahoo.security.tls.policy.RequiredPeerCredential.Field.SAN_DNS;
+import static com.yahoo.test.json.JsonTestHelper.assertJsonEquals;
 import static java.util.Collections.singleton;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
 
 /**
  * @author bjorncs
  */
 public class TransportSecurityOptionsJsonSerializerTest {
 
+    @Rule public TemporaryFolder tempDirectory = new TemporaryFolder();
+
+    private static final Path TEST_CONFIG_FILE = Paths.get("src/test/resources/transport-security-options.json");
+
     @Test
     public void can_serialize_and_deserialize_transport_security_options() {
         TransportSecurityOptions options = new TransportSecurityOptions.Builder()
@@ -46,4 +59,19 @@ public class TransportSecurityOptionsJsonSerializerTest {
         assertEquals(options, deserializedOptions);
     }
 
+    @Test
+    public void can_serialize_options_without_authorized_peers() throws IOException {
+        TransportSecurityOptions options = new TransportSecurityOptions.Builder()
+                .withCertificates(Paths.get("certs.pem"), Paths.get("myhost.key"))
+                .withCaCertificates(Paths.get("my_cas.pem"))
+                .build();
+        File outputFile = tempDirectory.newFile();
+        try (OutputStream out = Files.newOutputStream(outputFile.toPath())) {
+            new TransportSecurityOptionsJsonSerializer().serialize(out, options);
+        }
+        String expectedOutput = new String(Files.readAllBytes(TEST_CONFIG_FILE));
+        String actualOutput = new String(Files.readAllBytes(outputFile.toPath()));
+        assertJsonEquals(expectedOutput, actualOutput);
+    }
+
 }
diff --git a/security-utils/src/test/java/com/yahoo/security/tls/policy/HostGlobPatternTest.java b/security-utils/src/test/java/com/yahoo/security/tls/policy/HostGlobPatternTest.java
new file mode 100644
index 00000000000..ebec5605621
--- /dev/null
+++ b/security-utils/src/test/java/com/yahoo/security/tls/policy/HostGlobPatternTest.java
@@ -0,0 +1,69 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.security.tls.policy;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+
+/**
+ * @author bjorncs
+ */
+public class HostGlobPatternTest {
+
+    @Test
+    public void glob_without_wildcards_matches_entire_string() {
+        assertTrue(globMatches("foo", "foo"));
+        assertFalse(globMatches("foo", "fooo"));
+        assertFalse(globMatches("foo", "ffoo"));
+    }
+
+    @Test
+    public void wildcard_glob_can_match_prefix() {
+        assertTrue(globMatches("foo*", "foo"));
+        assertTrue(globMatches("foo*", "foobar"));
+        assertFalse(globMatches("foo*", "ffoo"));
+    }
+
+    @Test
+    public void wildcard_glob_can_match_suffix() {
+        assertTrue(globMatches("*foo", "foo"));
+        assertTrue(globMatches("*foo", "ffoo"));
+        assertFalse(globMatches("*foo", "fooo"));
+    }
+
+    @Test
+    public void wildcard_glob_can_match_substring() {
+        assertTrue(globMatches("f*o", "fo"));
+        assertTrue(globMatches("f*o", "foo"));
+        assertTrue(globMatches("f*o", "ffoo"));
+        assertFalse(globMatches("f*o", "boo"));
+    }
+
+    @Test
+    public void wildcard_glob_does_not_cross_multiple_dot_delimiter_boundaries() {
+        assertTrue(globMatches("*.bar.baz", "foo.bar.baz"));
+        assertTrue(globMatches("*.bar.baz", ".bar.baz"));
+        assertFalse(globMatches("*.bar.baz", "zoid.foo.bar.baz"));
+        assertTrue(globMatches("foo.*.baz", "foo.bar.baz"));
+        assertFalse(globMatches("foo.*.baz", "foo.bar.zoid.baz"));
+    }
+
+    @Test
+    public void single_char_glob_matches_non_dot_characters() {
+        assertTrue(globMatches("f?o", "foo"));
+        assertFalse(globMatches("f?o", "fooo"));
+        assertFalse(globMatches("f?o", "ffoo"));
+        assertFalse(globMatches("f?o", "f.o"));
+    }
+
+    @Test
+    public void other_regex_meta_characters_are_matched_as_literal_characters() {
+        assertTrue(globMatches("<([{\\^-=$!|]})+.>", "<([{\\^-=$!|]})+.>"));
+    }
+
+    private static boolean globMatches(String pattern, String value) {
+        return new HostGlobPattern(pattern).matches(value);
+    }
+}