Check parameters and types

Test for null pointers and invalid parameters.
Check if json content matches challenge type.
Enforce PublicKey when no private key instance should be used.

README.md

@@ -38,7 +38,6 @@ The following features are planned to be completed for the first beta release, b
 
 * Support of account recovery.
 * `proofOfPossession-01` and `tls-sni-01` challenge support.
-* Some hardening (like plausibility checks).
 
 ## License
 

acme4j-client/src/main/java/org/shredzone/acme4j/Account.java

@@ -34,7 +34,7 @@ public class Account {
      */
     public Account(KeyPair keyPair) {
         if (keyPair == null) {
-            throw new NullPointerException("keypair must be set");
+            throw new NullPointerException("keypair must not be null");
         }
 
         this.keyPair = keyPair;

acme4j-client/src/main/java/org/shredzone/acme4j/AcmeClientFactory.java

@@ -64,6 +64,10 @@ public final class AcmeClientFactory {
      * @return {@link AcmeClient} for communication with the server
      */
     public static AcmeClient connect(URI serverUri) throws AcmeException {
+        if (serverUri == null) {
+            throw new NullPointerException("serverUri must not be null");
+        }
+
         List<AcmeClientProvider> candidates = new ArrayList<>();
         for (AcmeClientProvider acp : ServiceLoader.load(AcmeClientProvider.class)) {
             if (acp.accepts(serverUri)) {

acme4j-client/src/main/java/org/shredzone/acme4j/challenge/DnsChallenge.java

@@ -54,7 +54,7 @@ public class DnsChallenge extends GenericChallenge {
      */
     public String getAuthorization() {
         if (authorization == null) {
-            throw new IllegalStateException("not yet authorized");
+            throw new IllegalStateException("Challenge is not authorized yet");
         }
         return authorization;
     }
@@ -83,12 +83,14 @@ public class DnsChallenge extends GenericChallenge {
 
     @Override
     public void marshall(ClaimBuilder cb) {
-        if (authorization == null) {
+        cb.put(KEY_KEY_AUTHORIZSATION, getAuthorization());
-            throw new IllegalStateException("Challenge has not been authorized yet.");
-        }
         cb.put(KEY_TYPE, getType());
         cb.put(KEY_TOKEN, getToken());
-        cb.put(KEY_KEY_AUTHORIZSATION, authorization);
+    }
+
+    @Override
+    protected boolean acceptable(String type) {
+        return TYPE.equals(type);
     }
 
 }

acme4j-client/src/main/java/org/shredzone/acme4j/challenge/GenericChallenge.java

@@ -16,9 +16,9 @@ package org.shredzone.acme4j.challenge;
 import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.security.Key;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
+import java.security.PublicKey;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -78,11 +78,21 @@ public class GenericChallenge implements Challenge {
 
     @Override
     public void authorize(Account account) {
-        // Standard implementation does nothing...
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
     }
 
     @Override
     public void unmarshall(Map<String, Object> map) {
+        String type = map.get(KEY_TYPE).toString();
+        if (type == null) {
+            throw new IllegalArgumentException("map does not contain a type");
+        }
+        if (!acceptable(type)) {
+            throw new IllegalArgumentException("wrong type: " + type);
+        }
+
         data.clear();
         data.putAll(map);
     }
@@ -92,6 +102,17 @@ public class GenericChallenge implements Challenge {
         cb.putAll(data);
     }
 
+    /**
+     * Checks if the type is acceptable to this challenge.
+     *
+     * @param type
+     *            Type to check
+     * @return {@code true} if acceptable, {@code false} if not
+     */
+    protected boolean acceptable(String type) {
+        return true;
+    }
+
     /**
      * Gets a value from the challenge state.
      *
@@ -120,11 +141,15 @@ public class GenericChallenge implements Challenge {
      * Computes a JWK Thumbprint. It is frequently used in responses.
      *
      * @param key
-     *            {@link Key} to create a thumbprint of
+     *            {@link PublicKey} to create a thumbprint of
      * @return Thumbprint, SHA-256 hashed
      * @see <a href="https://tools.ietf.org/html/rfc7638">RFC 7638</a>
      */
-    public static byte[] jwkThumbprint(Key key) {
+    public static byte[] jwkThumbprint(PublicKey key) {
+        if (key == null) {
+            throw new NullPointerException("key must not be null");
+        }
+
         try {
             final JsonWebKey jwk = JsonWebKey.Factory.newJwk(key);
 

acme4j-client/src/main/java/org/shredzone/acme4j/challenge/HttpChallenge.java

@@ -54,7 +54,7 @@ public class HttpChallenge extends GenericChallenge {
      */
     public String getAuthorization() {
         if (authorization == null) {
-            throw new IllegalStateException("not yet authorized");
+            throw new IllegalStateException("Challenge is not authorized yet");
         }
         return authorization;
     }
@@ -67,12 +67,14 @@ public class HttpChallenge extends GenericChallenge {
 
     @Override
     public void marshall(ClaimBuilder cb) {
-        if (authorization == null) {
+        cb.put(KEY_KEY_AUTHORIZSATION, getAuthorization());
-            throw new IllegalStateException("Challenge has not been authorized yet.");
-        }
         cb.put(KEY_TYPE, getType());
         cb.put(KEY_TOKEN, getToken());
-        cb.put(KEY_KEY_AUTHORIZSATION, authorization);
+    }
+
+    @Override
+    protected boolean acceptable(String type) {
+        return TYPE.equals(type);
     }
 
 }

acme4j-client/src/main/java/org/shredzone/acme4j/challenge/ProofOfPossessionChallenge.java

@@ -13,7 +13,7 @@
  */
 package org.shredzone.acme4j.challenge;
 
-import java.security.Key;
+import java.security.PublicKey;
 
 import org.shredzone.acme4j.Account;
 import org.shredzone.acme4j.util.ClaimBuilder;
@@ -32,7 +32,7 @@ public class ProofOfPossessionChallenge extends GenericChallenge {
      */
     public static final String TYPE = "proofOfPossession-01";
 
-    private Key accountKey;
+    private PublicKey accountKey;
 
     @Override
     public void authorize(Account account) {
@@ -46,4 +46,9 @@ public class ProofOfPossessionChallenge extends GenericChallenge {
         cb.putKey("accountKey", accountKey);
     }
 
+    @Override
+    protected boolean acceptable(String type) {
+        return TYPE.equals(type);
+    }
+
 }

acme4j-client/src/main/java/org/shredzone/acme4j/challenge/TlsSniChallenge.java

@@ -43,4 +43,9 @@ public class TlsSniChallenge extends GenericChallenge {
         put("n", n);
     }
 
+    @Override
+    protected boolean acceptable(String type) {
+        return TYPE.equals(type);
+    }
+
 }

acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java

@@ -82,6 +82,19 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public void newRegistration(Account account, Registration registration) throws AcmeException {
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
+        if (registration == null) {
+            throw new NullPointerException("registration must not be null");
+        }
+        if (registration.getLocation() != null) {
+            throw new IllegalArgumentException("registration location must be null");
+        }
+        if (registration.getAgreement() != null) {
+            throw new IllegalArgumentException("registration agreement must be null");
+        }
+
         LOG.debug("newRegistration");
         try (Connection conn = createConnection()) {
             ClaimBuilder claims = new ClaimBuilder();
@@ -113,11 +126,17 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public void updateRegistration(Account account, Registration registration) throws AcmeException {
-        LOG.debug("updateRegistration");
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
+        if (registration == null) {
+            throw new NullPointerException("registration must not be null");
+        }
         if (registration.getLocation() == null) {
-            throw new IllegalArgumentException("location must be set. Use newRegistration() if not known.");
+            throw new IllegalArgumentException("registration location must not be null. Use newRegistration() if not known.");
         }
 
+        LOG.debug("updateRegistration");
         try (Connection conn = createConnection()) {
             ClaimBuilder claims = new ClaimBuilder();
             claims.putResource("reg");
@@ -144,6 +163,16 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public void newAuthorization(Account account, Authorization auth) throws AcmeException {
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
+        if (auth == null) {
+            throw new NullPointerException("auth must not be null");
+        }
+        if (auth.getDomain() == null || auth.getDomain().isEmpty()) {
+            throw new IllegalArgumentException("auth domain must not be empty or null");
+        }
+
         LOG.debug("newAuthorization");
         try (Connection conn = createConnection()) {
             ClaimBuilder claims = new ClaimBuilder();
@@ -197,6 +226,16 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public void triggerChallenge(Account account, Challenge challenge) throws AcmeException {
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
+        if (challenge == null) {
+            throw new NullPointerException("challenge must not be null");
+        }
+        if (challenge.getLocation() == null) {
+            throw new IllegalArgumentException("challenge location is not set");
+        }
+
         LOG.debug("triggerChallenge");
         try (Connection conn = createConnection()) {
             ClaimBuilder claims = new ClaimBuilder();
@@ -214,6 +253,13 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public void updateChallenge(Challenge challenge) throws AcmeException {
+        if (challenge == null) {
+            throw new NullPointerException("challenge must not be null");
+        }
+        if (challenge.getLocation() == null) {
+            throw new IllegalArgumentException("challenge location is not set");
+        }
+
         LOG.debug("updateChallenge");
         try (Connection conn = createConnection()) {
             int rc = conn.sendRequest(challenge.getLocation());
@@ -228,6 +274,10 @@ public abstract class AbstractAcmeClient implements AcmeClient {
     @Override
     @SuppressWarnings("unchecked")
     public <T extends Challenge> T restoreChallenge(URI challengeUri) throws AcmeException {
+        if (challengeUri == null) {
+            throw new NullPointerException("challengeUri must not be null");
+        }
+
         LOG.debug("restoreChallenge");
         try (Connection conn = createConnection()) {
             int rc = conn.sendRequest(challengeUri);
@@ -248,6 +298,13 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public URI requestCertificate(Account account, byte[] csr) throws AcmeException {
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
+        if (csr == null) {
+            throw new NullPointerException("csr must not be null");
+        }
+
         LOG.debug("requestCertificate");
         try (Connection conn = createConnection()) {
             ClaimBuilder claims = new ClaimBuilder();
@@ -270,6 +327,10 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public X509Certificate downloadCertificate(URI certUri) throws AcmeException {
+        if (certUri == null) {
+            throw new NullPointerException("certUri must not be null");
+        }
+
         LOG.debug("downloadCertificate");
         try (Connection conn = createConnection()) {
             int rc = conn.sendRequest(certUri);
@@ -283,6 +344,13 @@ public abstract class AbstractAcmeClient implements AcmeClient {
 
     @Override
     public void revokeCertificate(Account account, X509Certificate certificate) throws AcmeException {
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
+        if (certificate == null) {
+            throw new NullPointerException("certificate must not be null");
+        }
+
         LOG.debug("revokeCertificate");
         URI resUri = resourceUri(Resource.REVOKE_CERT);
         if (resUri == null) {

acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java

@@ -63,11 +63,22 @@ public class DefaultConnection implements Connection {
     protected HttpURLConnection conn;
 
     public DefaultConnection(HttpConnector httpConnector) {
+        if (httpConnector == null) {
+            throw new NullPointerException("httpConnector must not be null");
+        }
+
         this.httpConnector = httpConnector;
     }
 
     @Override
     public int sendRequest(URI uri) throws AcmeException {
+        if (uri == null) {
+            throw new NullPointerException("uri must not be null");
+        }
+        if (conn != null) {
+            throw new IllegalStateException("Connection was not closed. Race condition?");
+        }
+
         try {
             LOG.debug("GET {}", uri);
 
@@ -88,6 +99,22 @@ public class DefaultConnection implements Connection {
 
     @Override
     public int sendSignedRequest(URI uri, ClaimBuilder claims, Session session, Account account) throws AcmeException {
+        if (uri == null) {
+            throw new NullPointerException("uri must not be null");
+        }
+        if (claims == null) {
+            throw new NullPointerException("claims must not be null");
+        }
+        if (session == null) {
+            throw new NullPointerException("session must not be null");
+        }
+        if (account == null) {
+            throw new NullPointerException("account must not be null");
+        }
+        if (conn != null) {
+            throw new IllegalStateException("Connection was not closed. Race condition?");
+        }
+
         try {
             KeyPair keypair = account.getKeyPair();
 

acme4j-client/src/main/java/org/shredzone/acme4j/impl/GenericAcmeClient.java

@@ -46,12 +46,20 @@ public class GenericAcmeClient extends AbstractAcmeClient {
      *            {@link URI} of the ACME server's directory service
      */
     public GenericAcmeClient(AcmeClientProvider provider, URI directoryUri) {
+        if (provider == null) {
+            throw new NullPointerException("provider must not be null");
+        }
+
         this.provider = provider;
         this.directoryUri = directoryUri;
     }
 
     @Override
     protected Challenge createChallenge(String type) {
+        if (type == null || type.isEmpty()) {
+            throw new IllegalArgumentException("type must not be empty or null");
+        }
+
         return provider.createChallenge(type);
     }
 
@@ -62,7 +70,15 @@ public class GenericAcmeClient extends AbstractAcmeClient {
 
     @Override
     protected URI resourceUri(Resource resource) throws AcmeException {
+        if (resource == null) {
+            throw new NullPointerException("resource must not be null");
+        }
+
         if (directoryMap.isEmpty()) {
+            if (directoryUri == null) {
+                throw new IllegalStateException("directoryUri was null on construction time");
+            }
+
             try (Connection conn = createConnection()) {
                 int rc = conn.sendRequest(directoryUri);
                 if (rc != HttpURLConnection.HTTP_OK) {

acme4j-client/src/main/java/org/shredzone/acme4j/provider/AbstractAcmeClientProvider.java

@@ -51,6 +51,10 @@ public abstract class AbstractAcmeClientProvider implements AcmeClientProvider {
 
     @Override
     public AcmeClient connect(URI serverUri) {
+        if (serverUri == null) {
+            throw new NullPointerException("serverUri must not be null");
+        }
+
         if (!accepts(serverUri)) {
             throw new IllegalArgumentException("This provider does not accept " + serverUri);
         }

acme4j-client/src/main/java/org/shredzone/acme4j/util/ClaimBuilder.java

@@ -14,6 +14,7 @@
 package org.shredzone.acme4j.util;
 
 import java.security.Key;
+import java.security.PublicKey;
 import java.util.Map;
 import java.util.TreeMap;
 
@@ -50,6 +51,10 @@ public class ClaimBuilder {
      * @return {@code this}
      */
     public ClaimBuilder put(String key, Object value) {
+        if (key == null) {
+            throw new NullPointerException("key must not be null");
+        }
+
         data.put(key, value);
         return this;
     }
@@ -107,10 +112,14 @@ public class ClaimBuilder {
      * @param key
      *            Claim key
      * @param publickey
-     *            {@link Key} to serialize
+     *            {@link PublicKey} to serialize
      * @return {@code this}
      */
-    public ClaimBuilder putKey(String key, Key publickey) {
+    public ClaimBuilder putKey(String key, PublicKey publickey) {
+        if (publickey == null) {
+            throw new NullPointerException("publickey must not be null");
+        }
+
         try {
             final JsonWebKey jwk = JsonWebKey.Factory.newJwk(publickey);
             Map<String, Object> jwkParams = jwk.toParams(JsonWebKey.OutputControlLevel.PUBLIC_ONLY);

acme4j-client/src/test/java/org/shredzone/acme4j/challenge/GenericChallengeTest.java

@@ -97,6 +97,15 @@ public class GenericChallengeTest {
         assertThat(cb.toString(), sameJSONAs(json));
     }
 
+    /**
+     * Test that an exception is thrown on challenge type mismatch.
+     */
+    @Test(expected = IllegalArgumentException.class)
+    public void testNotAcceptable() throws URISyntaxException {
+        HttpChallenge challenge = new HttpChallenge();
+        challenge.unmarshall(TestUtils.getJsonAsMap("dnsChallenge"));
+    }
+
     /**
      * Test that the test keypair's thumbprint is correct.
      */