[tor-bugs] #26690 [Applications/Tor Browser]: TBA: Port padlock states for .onion services to mobile

[Tor Bug Tracker & Wiki]

#26690: TBA: Port padlock states for .onion services to mobile
-------------------------------------------------+-------------------------
 Reporter:  igt0                                 |          Owner:  tbb-
                                                 |  team
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-mobile, TBA-a2,                  |  Actual Points:
  TorBrowserTeam201810R                          |
Parent ID:  #5709                                |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by sysrqb):

 * status:  needs_review => needs_revision


Comment:

 Nice, looks good! Only some minor comments.

 From the commit message:
 {{{
  Prior to this patch, TBA was showing all onion services as SSL/TLS
     encrypted connections(lock icon).
 }}}
 Nit: That isn't true, is it? Maybe I'm misreading it. Currently, onion
 sites served without TLS do not show a lock icon, right?

 ----

 {{{
 +    <item
 +        android:drawable="@drawable/ic_lock_onion_active"
 +        android:maxLevel="8"/>
 }}}

 I feel like "onion_active" doesn't describe the state well. Can we call it
 onion_lock (or something similar)? When I see IconType.ONION_ACTIVATE in
 the code, I don't immediately remember what that means. I think
 IconType.ONION_LOCK be a little better. And maybe the icons that don't
 contain locks shouldn't have "lock" in their name? Thoughts?

 ----

 {{{
 @@ -100,6 +103,8 @@ public class SecurityModeUtil {
          final MixedMode displayMixedMode =
 identity.getMixedModeDisplay();
          final TrackingMode trackingMode = identity.getTrackingMode();
          final boolean securityException = identity.isSecurityException();
 +       final boolean isOnionHost = identity.isOnionHost();
 +       final boolean hasCert = identity.hasCert();
 }}}
 Nit: Please correct the indentation

 ----

 {{{
 @@ -119,9 +124,15 @@ public class SecurityModeUtil {
              return IconType.DEFAULT;
          }

 -        return securityModeMap.containsKey(securityMode)
 -                ? securityModeMap.get(securityMode)
 -                : IconType.UNKNOWN;
 +        if (securityMode == SecurityMode.UNKNOWN) {
 +            return isOnionHost ? IconType.ONION : IconType.UNKNOWN;
 +       } else if (securityMode == SecurityMode.IDENTIFIED) {
 +            return isOnionHost ? (hasCert ? IconType.ONION_ACTIVATE :
 IconType.ONION) : IconType.LOCK_SECURE;
 +       } else if (securityMode == SecurityMode.VERIFIED) {
 +            return isOnionHost ? IconType.ONION_ACTIVATE :
 IconType.LOCK_SECURE;
 +       } else {
 +            return IconType.UNKNOWN;
 +        }
      }
 }}}
 Nit. Here, too.

 ----

 {{{
 -                mIcon.setImageResource(R.drawable.ic_lock_disabled);
 +                int resId = siteIdentity.isOnionHost() ?
 R.drawable.ic_lock_onion_disabled : R.drawable.ic_lock_disabled;
 +                mIcon.setImageResource(resId);
 }}}
 These changes in
 `mobile/android/base/java/org/mozilla/gecko/toolbar/SiteIdentityPopup.java`
 are #27657, so I'm not sure we should implement this yet (although I like
 it).

 ----

 {{{
 -        final boolean isIdentityKnown = (siteIdentity.getSecurityMode()
 == SecurityMode.IDENTIFIED ||
 -                                         siteIdentity.getSecurityMode()
 == SecurityMode.VERIFIED);
 +        final boolean isIdentityKnown = ((siteIdentity.getSecurityMode()
 == SecurityMode.IDENTIFIED ||
 +                                         siteIdentity.getSecurityMode()
 == SecurityMode.VERIFIED) &&
 +                                         siteIdentity.hasCert());

 }}}

 Is this needed? Can the SecurityMode be Verified or Identified without a
 cert? (I don't see a code path that allows this, but it's possible I
 missed it)

 ----

 I created some (simple) test pages for this:
 https://people.torproject.org/~sysrqb/mixed_content/
 http://sbe5fi5cka5l3fqe.onion/~sysrqb/mixed_content/

 The fact Mozilla re-implemented all of this logic for mobile instead of
 reusing some of the existing browser functionality is really annoying.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26690#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online