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 179078 4a44ad0248ba002bd1ced06096e1a20c631cb2b4
parent 179077 64bfb458f73901560f1f543bc9a5a7f14f4a490f
child 179079 c2c8796a92126b7a2c8709a730cf8ce2eb222c2a
push id26606
push userryanvm@gmail.com
push dateFri, 18 Apr 2014 02:20:00 +0000
treeherdermozilla-central@ec728bfdbb79 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs986664
milestone31.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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.