From 46daaa8cfd0067226b98aab4f31bc2b5249b3c9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20K=C3=B6rber?= Date: Sun, 20 Dec 2015 17:39:07 +0100 Subject: [PATCH] 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 | 1 - .../java/org/shredzone/acme4j/Account.java | 2 +- .../shredzone/acme4j/AcmeClientFactory.java | 4 ++ .../acme4j/challenge/DnsChallenge.java | 12 ++-- .../acme4j/challenge/GenericChallenge.java | 33 +++++++-- .../acme4j/challenge/HttpChallenge.java | 12 ++-- .../challenge/ProofOfPossessionChallenge.java | 9 ++- .../acme4j/challenge/TlsSniChallenge.java | 5 ++ .../acme4j/impl/AbstractAcmeClient.java | 72 ++++++++++++++++++- .../acme4j/impl/DefaultConnection.java | 27 +++++++ .../acme4j/impl/GenericAcmeClient.java | 16 +++++ .../provider/AbstractAcmeClientProvider.java | 4 ++ .../shredzone/acme4j/util/ClaimBuilder.java | 13 +++- .../challenge/GenericChallengeTest.java | 9 +++ 14 files changed, 197 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 8fa7e3c2..9b7a8a2a 100644 --- a/README.md +++ b/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 diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Account.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Account.java index 64324d77..24de24d4 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/Account.java +++ b/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; diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/AcmeClientFactory.java b/acme4j-client/src/main/java/org/shredzone/acme4j/AcmeClientFactory.java index 0002ae48..605895f6 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/AcmeClientFactory.java +++ b/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 candidates = new ArrayList<>(); for (AcmeClientProvider acp : ServiceLoader.load(AcmeClientProvider.class)) { if (acp.accepts(serverUri)) { diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/DnsChallenge.java b/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/DnsChallenge.java index 06c68eb3..ea9590df 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/DnsChallenge.java +++ b/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) { - throw new IllegalStateException("Challenge has not been authorized yet."); - } + cb.put(KEY_KEY_AUTHORIZSATION, getAuthorization()); 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); } } diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/GenericChallenge.java b/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/GenericChallenge.java index 3121fcca..78aaf9f7 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/GenericChallenge.java +++ b/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 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 RFC 7638 */ - 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); diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/HttpChallenge.java b/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/HttpChallenge.java index c6c2e4a0..8b8f7e26 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/HttpChallenge.java +++ b/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) { - throw new IllegalStateException("Challenge has not been authorized yet."); - } + cb.put(KEY_KEY_AUTHORIZSATION, getAuthorization()); 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); } } diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/ProofOfPossessionChallenge.java b/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/ProofOfPossessionChallenge.java index f3c5e33a..59050ac7 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/ProofOfPossessionChallenge.java +++ b/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); + } + } diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/TlsSniChallenge.java b/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/TlsSniChallenge.java index e2205ac3..fee9080e 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/challenge/TlsSniChallenge.java +++ b/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); + } + } diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java b/acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java index d785e3df..a98d5812 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/AbstractAcmeClient.java +++ b/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 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) { diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java b/acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java index 87063804..87df941b 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/DefaultConnection.java +++ b/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(); diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/GenericAcmeClient.java b/acme4j-client/src/main/java/org/shredzone/acme4j/impl/GenericAcmeClient.java index c7e6dd84..15464059 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/impl/GenericAcmeClient.java +++ b/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) { diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/provider/AbstractAcmeClientProvider.java b/acme4j-client/src/main/java/org/shredzone/acme4j/provider/AbstractAcmeClientProvider.java index d3da809b..70ddbeb0 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/provider/AbstractAcmeClientProvider.java +++ b/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); } diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/util/ClaimBuilder.java b/acme4j-client/src/main/java/org/shredzone/acme4j/util/ClaimBuilder.java index 59b316bc..3d70d71e 100644 --- a/acme4j-client/src/main/java/org/shredzone/acme4j/util/ClaimBuilder.java +++ b/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 jwkParams = jwk.toParams(JsonWebKey.OutputControlLevel.PUBLIC_ONLY); diff --git a/acme4j-client/src/test/java/org/shredzone/acme4j/challenge/GenericChallengeTest.java b/acme4j-client/src/test/java/org/shredzone/acme4j/challenge/GenericChallengeTest.java index 61f55ced..ab21fd58 100644 --- a/acme4j-client/src/test/java/org/shredzone/acme4j/challenge/GenericChallengeTest.java +++ b/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. */