Bug 986664 - Make Android FxAccountClient* HAWK requests always include request payload hash. r=rnewman
authorNick Alexander <nalexander@mozilla.com>
Thu, 17 Apr 2014 12:04:17 -0700
changeset 179591 4a44ad0248ba002bd1ced06096e1a20c631cb2b4
parent 179590 64bfb458f73901560f1f543bc9a5a7f14f4a490f
child 179592 c2c8796a92126b7a2c8709a730cf8ce2eb222c2a
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewersrnewman
bugs986664
milestone31.0a1
Bug 986664 - Make Android FxAccountClient* HAWK requests always include request payload hash. r=rnewman Relanding with correct bug number.
mobile/android/base/background/fxa/FxAccountClient10.java
mobile/android/base/fxa/activities/FxAccountCreateAccountFragment.java
mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
mobile/android/base/sync/net/HawkAuthHeaderProvider.java
--- a/mobile/android/base/background/fxa/FxAccountClient10.java
+++ b/mobile/android/base/background/fxa/FxAccountClient10.java
@@ -171,42 +171,45 @@ public class FxAccountClient10 {
    */
   protected abstract class ResourceDelegate<T> extends BaseResourceDelegate {
     protected abstract void handleSuccess(final int status, HttpResponse response, final ExtendedJSONObject body);
 
     protected final RequestDelegate<T> delegate;
 
     protected final byte[] tokenId;
     protected final byte[] reqHMACKey;
-    protected final boolean payload;
     protected final SkewHandler skewHandler;
 
     /**
      * Create a delegate for an un-authenticated resource.
      */
     public ResourceDelegate(final Resource resource, final RequestDelegate<T> delegate) {
-      this(resource, delegate, null, null, false);
+      this(resource, delegate, null, null);
     }
 
     /**
      * Create a delegate for a Hawk-authenticated resource.
+     * <p>
+     * Every Hawk request that encloses an entity (PATCH, POST, and PUT) will
+     * include the payload verification hash.
      */
-    public ResourceDelegate(final Resource resource, final RequestDelegate<T> delegate, final byte[] tokenId, final byte[] reqHMACKey, final boolean authenticatePayload) {
+    public ResourceDelegate(final Resource resource, final RequestDelegate<T> delegate, final byte[] tokenId, final byte[] reqHMACKey) {
       super(resource);
       this.delegate = delegate;
       this.reqHMACKey = reqHMACKey;
       this.tokenId = tokenId;
-      this.payload = authenticatePayload;
       this.skewHandler = SkewHandler.getSkewHandlerForResource(resource);
     }
 
     @Override
     public AuthHeaderProvider getAuthHeaderProvider() {
       if (tokenId != null && reqHMACKey != null) {
-        return new HawkAuthHeaderProvider(Utils.byte2Hex(tokenId), reqHMACKey, payload, skewHandler.getSkewInSeconds());
+        // We always include the payload verification hash for FxA Hawk-authenticated requests.
+        final boolean includePayloadVerificationHash = true;
+        return new HawkAuthHeaderProvider(Utils.byte2Hex(tokenId), reqHMACKey, includePayloadVerificationHash, skewHandler.getSkewInSeconds());
       }
       return super.getAuthHeaderProvider();
     }
 
     @Override
     public String getUserAgent() {
       return FxAccountConstants.USER_AGENT;
     }
@@ -478,17 +481,17 @@ public class FxAccountClient10 {
     BaseResource resource;
     try {
       resource = new BaseResource(new URI(serverURI + "session/create"));
     } catch (URISyntaxException e) {
       invokeHandleError(delegate, e);
       return;
     }
 
-    resource.delegate = new ResourceDelegate<TwoTokens>(resource, delegate, tokenId, reqHMACKey, false) {
+    resource.delegate = new ResourceDelegate<TwoTokens>(resource, delegate, tokenId, reqHMACKey) {
       @Override
       public void handleSuccess(int status, HttpResponse response, ExtendedJSONObject body) {
         try {
           byte[] keyFetchToken = new byte[32];
           byte[] sessionToken = new byte[32];
           unbundleBody(body, requestKey, FxAccountUtils.KW("session/create"), keyFetchToken, sessionToken);
           delegate.handleSuccess(new TwoTokens(keyFetchToken, sessionToken));
           return;
@@ -514,17 +517,17 @@ public class FxAccountClient10 {
     BaseResource resource;
     try {
       resource = new BaseResource(new URI(serverURI + "session/destroy"));
     } catch (URISyntaxException e) {
       invokeHandleError(delegate, e);
       return;
     }
 
-    resource.delegate = new ResourceDelegate<Void>(resource, delegate, tokenId, reqHMACKey, false) {
+    resource.delegate = new ResourceDelegate<Void>(resource, delegate, tokenId, reqHMACKey) {
       @Override
       public void handleSuccess(int status, HttpResponse response, ExtendedJSONObject body) {
         delegate.handleSuccess(null);
       }
     };
     post(resource, null, delegate);
   }
 
@@ -603,17 +606,17 @@ public class FxAccountClient10 {
     BaseResource resource;
     try {
       resource = new BaseResource(new URI(serverURI + "account/keys"));
     } catch (URISyntaxException e) {
       invokeHandleError(delegate, e);
       return;
     }
 
-    resource.delegate = new ResourceDelegate<TwoKeys>(resource, delegate, tokenId, reqHMACKey, false) {
+    resource.delegate = new ResourceDelegate<TwoKeys>(resource, delegate, tokenId, reqHMACKey) {
       @Override
       public void handleSuccess(int status, HttpResponse response, ExtendedJSONObject body) {
         try {
           byte[] kA = new byte[FxAccountUtils.CRYPTO_KEY_LENGTH_BYTES];
           byte[] wrapkB = new byte[FxAccountUtils.CRYPTO_KEY_LENGTH_BYTES];
           unbundleBody(body, requestKey, FxAccountUtils.KW("account/keys"), kA, wrapkB);
           delegate.handleSuccess(new TwoKeys(kA, wrapkB));
           return;
@@ -665,17 +668,17 @@ public class FxAccountClient10 {
     BaseResource resource;
     try {
       resource = new BaseResource(new URI(serverURI + "recovery_email/status"));
     } catch (URISyntaxException e) {
       invokeHandleError(delegate, e);
       return;
     }
 
-    resource.delegate = new ResourceDelegate<StatusResponse>(resource, delegate, tokenId, reqHMACKey, false) {
+    resource.delegate = new ResourceDelegate<StatusResponse>(resource, delegate, tokenId, reqHMACKey) {
       @Override
       public void handleSuccess(int status, HttpResponse response, ExtendedJSONObject body) {
         try {
           String[] requiredStringFields = new String[] { JSON_KEY_EMAIL };
           body.throwIfFieldsMissingOrMisTyped(requiredStringFields, String.class);
           String email = body.getString(JSON_KEY_EMAIL);
           Boolean verified = body.getBoolean(JSON_KEY_VERIFIED);
           delegate.handleSuccess(new StatusResponse(email, verified));
@@ -707,17 +710,17 @@ public class FxAccountClient10 {
     BaseResource resource;
     try {
       resource = new BaseResource(new URI(serverURI + "certificate/sign"));
     } catch (URISyntaxException e) {
       invokeHandleError(delegate, e);
       return;
     }
 
-    resource.delegate = new ResourceDelegate<String>(resource, delegate, tokenId, reqHMACKey, true) {
+    resource.delegate = new ResourceDelegate<String>(resource, delegate, tokenId, reqHMACKey) {
       @Override
       public void handleSuccess(int status, HttpResponse response, ExtendedJSONObject body) {
         String cert = body.getString("cert");
         if (cert == null) {
           delegate.handleError(new FxAccountClientException("cert must be a non-null string"));
           return;
         }
         delegate.handleSuccess(cert);
@@ -748,17 +751,17 @@ public class FxAccountClient10 {
     BaseResource resource;
     try {
       resource = new BaseResource(new URI(serverURI + "recovery_email/resend_code"));
     } catch (URISyntaxException e) {
       invokeHandleError(delegate, e);
       return;
     }
 
-    resource.delegate = new ResourceDelegate<Void>(resource, delegate, tokenId, reqHMACKey, false) {
+    resource.delegate = new ResourceDelegate<Void>(resource, delegate, tokenId, reqHMACKey) {
       @Override
       public void handleSuccess(int status, HttpResponse response, ExtendedJSONObject body) {
         try {
           delegate.handleSuccess(null);
           return;
         } catch (Exception e) {
           delegate.handleError(e);
           return;
deleted file mode 100644
--- a/mobile/android/base/fxa/activities/FxAccountCreateAccountFragment.java
+++ /dev/null
@@ -1,191 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-package org.mozilla.gecko.fxa.activities;
-
-import android.support.v4.app.Fragment;
-
-public class FxAccountCreateAccountFragment extends Fragment { // implements OnClickListener {
-  protected static final String LOG_TAG = FxAccountCreateAccountFragment.class.getSimpleName();
-//
-//  protected FxAccountSetupActivity activity;
-//
-//  protected EditText emailEdit;
-//  protected EditText passwordEdit;
-//  protected EditText password2Edit;
-//  protected Button button;
-//
-//  protected TextView emailError;
-//  protected TextView passwordError;
-//
-//  protected TextChangedListener textChangedListener;
-//  protected EditorActionListener editorActionListener;
-//  protected OnFocusChangeListener focusChangeListener;
-//  @Override
-//  public void onCreate(Bundle savedInstanceState) {
-//    super.onCreate(savedInstanceState);
-//    // Retain this fragment across configuration changes. See, for example,
-//    // http://www.androiddesignpatterns.com/2013/04/retaining-objects-across-config-changes.html
-//    // This fragment will own AsyncTask instances which should not be
-//    // interrupted by configuration changes (and activity changes).
-//    setRetainInstance(true);
-//  }
-
-//  @Override
-//  public View onCreateView(LayoutInflater inflater, ViewGroup container,
-//      Bundle savedInstanceState) {
-//    View v = inflater.inflate(R.layout.fxaccount_create_account_fragment, container, false);
-//
-//    FxAccountSetupActivity.linkifyTextViews(v, new int[] { R.id.description, R.id.policy });
-//
-//    emailEdit = (EditText) ensureFindViewById(v, R.id.email, "email");
-//    passwordEdit = (EditText) ensureFindViewById(v, R.id.password, "password");
-//    // Second password can be null.
-//    password2Edit = (EditText) v.findViewById(R.id.password2);
-//
-//    emailError = (TextView) ensureFindViewById(v, R.id.email_error, "email error");
-//    passwordError = (TextView) ensureFindViewById(v, R.id.password_error, "password error");
-//
-//    textChangedListener = new TextChangedListener();
-//    editorActionListener = new EditorActionListener();
-//    focusChangeListener = new FocusChangeListener();
-//
-//    addListeners(emailEdit);
-//    addListeners(passwordEdit);
-//    if (password2Edit != null) {
-//      addListeners(password2Edit);
-//    }
-//
-//    button = (Button) ensureFindViewById(v, R.id.create_account_button, "button");
-//    button.setOnClickListener(this);
-//    return v;
-//  }
-
-//  protected void onCreateAccount(View button) {
-//    Logger.debug(LOG_TAG, "onCreateAccount: Asking for username/password for new account.");
-//    String email = emailEdit.getText().toString();
-//    String password = passwordEdit.getText().toString();
-//    activity.signUp(email, password);
-//  }
-//
-//  @Override
-//  public void onClick(View v) {
-//    switch (v.getId()) {
-//    case R.id.create_account_button:
-//      if (!validate(false)) {
-//        return;
-//      }
-//      onCreateAccount(v);
-//      break;
-//    }
-//  }
-//
-//  protected void addListeners(EditText editText) {
-//    editText.addTextChangedListener(textChangedListener);
-//    editText.setOnEditorActionListener(editorActionListener);
-//    editText.setOnFocusChangeListener(focusChangeListener);
-//  }
-//
-//  protected class FocusChangeListener implements OnFocusChangeListener {
-//    @Override
-//    public void onFocusChange(View v, boolean hasFocus) {
-//      if (hasFocus) {
-//        return;
-//      }
-//      validate(false);
-//    }
-//  }
-//
-//  protected class EditorActionListener implements OnEditorActionListener {
-//    @Override
-//    public boolean onEditorAction(TextView v, int actionId, KeyEvent event) {
-//      validate(false);
-//      return false;
-//    }
-//  }
-//
-//  protected class TextChangedListener implements TextWatcher {
-//    @Override
-//    public void afterTextChanged(Editable s) {
-//      validate(true);
-//    }
-//
-//    @Override
-//    public void beforeTextChanged(CharSequence s, int start, int count, int after) {
-//      // Do nothing.
-//    }
-//
-//    @Override
-//    public void onTextChanged(CharSequence s, int start, int before, int count) {
-//      // Do nothing.
-//    }
-//  }
-//
-//  /**
-//   * Show or hide error messaging.
-//   *
-//   * @param removeOnly
-//   *          if true, possibly remove existing error messages but do not set an
-//   *          error message if one was not present.
-//   * @param errorResourceId
-//   *          of error string, or -1 to hide.
-//   * @param errorView
-//   *          <code>TextView</code> instance to display error message in.
-//   * @param edits
-//   *          <code>EditText</code> instances to style.
-//   */
-//  protected void setError(boolean removeOnly, int errorResourceId, TextView errorView, EditText... edits) {
-//    if (removeOnly && errorResourceId != -1) {
-//      return;
-//    }
-//
-//    int res = errorResourceId == -1 ? R.drawable.fxaccount_textfield_background : R.drawable.fxaccount_textfield_error_background;
-//    for (EditText edit : edits) {
-//      if (edit == null) {
-//        continue;
-//      }
-//      edit.setBackgroundResource(res);
-//    }
-//    if (errorResourceId == -1) {
-//      errorView.setVisibility(View.GONE);
-//      errorView.setText(null);
-//    } else {
-//      errorView.setText(errorResourceId);
-//      errorView.setVisibility(View.VISIBLE);
-//    }
-//  }
-//
-//  protected boolean validate(boolean removeOnly) {
-//    boolean enabled = true;
-//    final String email = emailEdit.getText().toString();
-//    final String password = passwordEdit.getText().toString();
-//
-//    if (email.length() == 0 || Patterns.EMAIL_ADDRESS.matcher(email).matches()) {
-//      setError(removeOnly, -1, emailError, emailEdit);
-//    } else {
-//      enabled = false;
-//      setError(removeOnly, R.string.fxaccount_bad_email, emailError, emailEdit);
-//    }
-//
-//    if (password2Edit != null) {
-//      final String password2 = password2Edit.getText().toString();
-//      enabled = enabled && password2.length() > 0;
-//
-//      boolean passwordsMatch = password.equals(password2);
-//      if (passwordsMatch) {
-//        setError(removeOnly, -1, passwordError, passwordEdit, password2Edit);
-//      } else {
-//        enabled = false;
-//        setError(removeOnly, R.string.fxaccount_bad_passwords, passwordError, passwordEdit, password2Edit);
-//      }
-//    }
-//
-//    if (enabled != button.isEnabled()) {
-//      Logger.debug(LOG_TAG, (enabled ? "En" : "Dis") + "abling button.");
-//      button.setEnabled(enabled);
-//    }
-//
-//    return enabled;
-//  }
-}
--- a/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
+++ b/mobile/android/base/fxa/sync/FxAccountSyncAdapter.java
@@ -360,17 +360,22 @@ public class FxAccountSyncAdapter extend
           ClientsDataDelegate clientsDataDelegate = new SharedPreferencesClientsDataDelegate(sharedPrefs);
 
           // We compute skew over time using SkewHandler. This yields an unchanging
           // skew adjustment that the HawkAuthHeaderProvider uses to adjust its
           // timestamps. Eventually we might want this to adapt within the scope of a
           // global session.
           final SkewHandler storageServerSkewHandler = SkewHandler.getSkewHandlerForHostname(storageHostname);
           final long storageServerSkew = storageServerSkewHandler.getSkewInSeconds();
-          final AuthHeaderProvider authHeaderProvider = new HawkAuthHeaderProvider(token.id, token.key.getBytes("UTF-8"), false, storageServerSkew);
+          // We expect Sync to upload large sets of records. Calculating the
+          // payload verification hash for these record sets could be expensive,
+          // so we explicitly do not send payload verification hashes to the
+          // Sync storage endpoint.
+          final boolean includePayloadVerificationHash = false;
+          final AuthHeaderProvider authHeaderProvider = new HawkAuthHeaderProvider(token.id, token.key.getBytes("UTF-8"), includePayloadVerificationHash, storageServerSkew);
 
           final Context context = getContext();
           final SyncConfiguration syncConfig = new SyncConfiguration(token.uid, authHeaderProvider, sharedPrefs, syncKeyBundle);
 
           Collection<String> knownStageNames = SyncConfiguration.validEngineNames();
           syncConfig.stagesToSync = Utils.getStagesToSyncFromBundle(knownStageNames, extras);
           syncConfig.setClusterURL(storageServerURI);
 
--- a/mobile/android/base/sync/net/HawkAuthHeaderProvider.java
+++ b/mobile/android/base/sync/net/HawkAuthHeaderProvider.java
@@ -13,16 +13,17 @@ import java.security.InvalidKeyException
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.util.Locale;
 
 import javax.crypto.Mac;
 import javax.crypto.spec.SecretKeySpec;
 
 import org.mozilla.apache.commons.codec.binary.Base64;
+import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.sync.Utils;
 
 import ch.boye.httpclientandroidlib.Header;
 import ch.boye.httpclientandroidlib.HttpEntity;
 import ch.boye.httpclientandroidlib.HttpEntityEnclosingRequest;
 import ch.boye.httpclientandroidlib.client.methods.HttpRequestBase;
 import ch.boye.httpclientandroidlib.client.methods.HttpUriRequest;
 import ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient;
@@ -50,24 +51,30 @@ public class HawkAuthHeaderProvider impl
   protected final boolean includePayloadHash;
   protected final long skewSeconds;
 
   /**
    * Create a Hawk Authorization header provider.
    * <p>
    * Hawk specifies no mechanism by which a client receives an
    * identifier-and-key pair from the server.
+   * <p>
+   * Hawk requests can include a payload verification hash with requests that
+   * enclose an entity (PATCH, POST, and PUT requests).  <b>You should default
+   * to including the payload verification hash<b> unless you have a good reason
+   * not to -- the server can always ignore payload verification hashes provided
+   * by the client.
    *
    * @param id
    *          to name requests with.
    * @param key
    *          to sign request with.
    *
    * @param includePayloadHash
-   *          true if message integrity hash should be included in signed
+   *          true if payload verification hash should be included in signed
    *          request header. See <a href="https://github.com/hueniverse/hawk#payload-validation">https://github.com/hueniverse/hawk#payload-validation</a>.
    *
    * @param skewSeconds
    *          a number of seconds by which to skew the current time when
    *          computing a header.
    */
   public HawkAuthHeaderProvider(String id, byte[] key, boolean includePayloadHash, long skewSeconds) {
     if (id == null) {
@@ -77,21 +84,16 @@ public class HawkAuthHeaderProvider impl
       throw new IllegalArgumentException("key must not be null");
     }
     this.id = id;
     this.key = key;
     this.includePayloadHash = includePayloadHash;
     this.skewSeconds = skewSeconds;
   }
 
-  public HawkAuthHeaderProvider(String id, byte[] key, boolean includePayloadHash) {
-    this(id, key, includePayloadHash, 0L);
-  }
-
-
   /**
    * @return the current time in milliseconds.
    */
   @SuppressWarnings("static-method")
   protected long now() {
     return System.currentTimeMillis();
   }
 
@@ -136,24 +138,19 @@ public class HawkAuthHeaderProvider impl
       throw new IllegalArgumentException("nonce must not be null.");
     }
     if (nonce.length() == 0) {
       throw new IllegalArgumentException("nonce must not be empty.");
     }
 
     String payloadHash = null;
     if (includePayloadHash) {
-      if (!(request instanceof HttpEntityEnclosingRequest)) {
-        throw new IllegalArgumentException("cannot specify payload for request without an entity");
-      }
-      HttpEntity entity = ((HttpEntityEnclosingRequest) request).getEntity();
-      if (entity == null) {
-        throw new IllegalArgumentException("cannot specify payload for request with a null entity");
-      }
-      payloadHash = Base64.encodeBase64String(getPayloadHash(entity));
+      payloadHash = getPayloadHashString(request);
+    } else {
+      Logger.debug(LOG_TAG, "Configured to not include payload hash for this request.");
     }
 
     String app = null;
     String dlg = null;
     String requestString = getRequestString(request, "header", timestamp, nonce, payloadHash, extra, app, dlg);
     String macString = getSignature(requestString.getBytes("UTF-8"), this.key);
 
     StringBuilder sb = new StringBuilder();
@@ -179,16 +176,48 @@ public class HawkAuthHeaderProvider impl
     sb.append("mac=\"");
     sb.append(macString);
     sb.append("\"");
 
     return new BasicHeader("Authorization", sb.toString());
   }
 
   /**
+   * Get the payload verification hash for the given request, if possible.
+   * <p>
+   * Returns null if the request does not enclose an entity (is not an HTTP
+   * PATCH, POST, or PUT). Throws if the payload verification hash cannot be
+   * computed.
+   *
+   * @param request
+   *          to compute hash for.
+   * @return verification hash, or null if the request does not enclose an entity.
+   * @throws IllegalArgumentException if the request does not enclose a valid non-null entity.
+   * @throws UnsupportedEncodingException
+   * @throws NoSuchAlgorithmException
+   * @throws IOException
+   */
+  protected static String getPayloadHashString(HttpRequestBase request)
+      throws UnsupportedEncodingException, NoSuchAlgorithmException, IOException, IllegalArgumentException {
+    final boolean shouldComputePayloadHash = request instanceof HttpEntityEnclosingRequest;
+    if (!shouldComputePayloadHash) {
+      Logger.debug(LOG_TAG, "Not computing payload verification hash for non-enclosing request.");
+      return null;
+    }
+    if (!(request instanceof HttpEntityEnclosingRequest)) {
+      throw new IllegalArgumentException("Cannot compute payload verification hash for enclosing request without an entity");
+    }
+    final HttpEntity entity = ((HttpEntityEnclosingRequest) request).getEntity();
+    if (entity == null) {
+      throw new IllegalArgumentException("Cannot compute payload verification hash for enclosing request with a null entity");
+    }
+    return Base64.encodeBase64String(getPayloadHash(entity));
+  }
+
+  /**
    * Escape the user-provided extra string for the ext="" header attribute.
    * <p>
    * Hawk escapes the header ext="" attribute differently than it does the extra
    * line in the normalized request string.
    * <p>
    * See <a href="https://github.com/hueniverse/hawk/blob/871cc597973110900467bd3dfb84a3c892f678fb/lib/browser.js#L385">https://github.com/hueniverse/hawk/blob/871cc597973110900467bd3dfb84a3c892f678fb/lib/browser.js#L385</a>.
    *
    * @param extra to escape.