TBA: Port padlock states for .onion services to mobile
The same work made in the #23247 (moved) needs to be ported to mobile.
Activity
Trac:
Parent: N/A to #5709 (moved)Bug 26690 - Port padlock states for .onion services to mobile Prior to this patch, TBA was showing all onion services as SSL/TLS encrypted connections(lock icon). This patch fixes the issue adding several new Onion icons to indicate all the various permutations of the Onions services hosted HTTP or HTTPS pages.Trac:
Status: new to needs_reviewNice, 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.javaare #27657 (moved), 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.
Trac:
Status: needs_review to needs_revisionReplying to sysrqb:
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?
Yeah, you are right. I updated the commit msg.
{{{
- <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?
I renamed them.
{{{ @@ -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
Duh! Updated it.
{{{ @@ -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.
}
Okey dockey.
{{{
-
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.javaare #27657 (moved), so I'm not sure we should implement this yet (although I like it).
I think we should because without this patch for some cases it is showing the string "Null" and it is ugly and misleading.
{{{
-
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)
Yeah, it should't. However I reversed engineered the https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-60.3.0esr-8.5-1&id=c3775a668d0c9fae044aa26ed85994139590c21d
And it always checked for the cert.
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.
Updated patch: https://trac.torproject.org/projects/tor/attachment/ticket/26690/0001-Bug-26690-Port-padlock-states-for-.onion-services-to.2.patch
Trac:
Status: needs_revision to needs_review
Sorry for the delay here. Just some nits I found:
- indentation (in security_mode_icon_nm.xml)
+ android:drawable="@drawable/ic_onion_disabled" + android:maxLevel="9"/>- In the commit message
s/Onion icons/onion icons s/of the Onion services hosted/of onion services hosted/
antonela: I felt the onion icon (not the one for extended validation nor the one showing insecure content) was too big. Compared to the desktop, where the icon stays the same size-wise compared to the lock icon, the onion icon seems to get bigger on mobile. Could we fix that or do you want to leave it as-is?
Oh, and, yes, having the things scheduled for #27657 (moved) already done here for mobile is pretty cool!
Trac:
Cc: sysrqb, gk to sysrqb, gk, antonela
Keywords: TorBrowserTeam201811R deleted, TorBrowserTeam201811 added
Status: needs_review to needs_revisionI updated the commit msg and the indentation:
Trac:
Status: needs_revision to needs_review
Replying to gk:
antonela: I felt the onion icon (not the one for extended validation nor the one showing insecure content) was too big. Compared to the desktop, where the icon stays the same size-wise compared to the lock icon, the onion icon seems to get bigger on mobile. Could we fix that or do you want to leave it as-is?
I just tested the .apk and yes is big. The size should be the same size we have at the lock icon (24px). If we can fix it, would be awesome.
Igt0 let me know if you need any asset.
Replying to antonela:
Replying to gk:
antonela: I felt the onion icon (not the one for extended validation nor the one showing insecure content) was too big. Compared to the desktop, where the icon stays the same size-wise compared to the lock icon, the onion icon seems to get bigger on mobile. Could we fix that or do you want to leave it as-is?
I just tested the .apk and yes is big. The size should be the same size we have at the lock icon (24px). If we can fix it, would be awesome.
Igt0 let me know if you need any asset.
I see the SVG used in #23247 (moved) is 16x16 (by default?), but the PNG used here is showing as 36x36.
Overall, I think the patch looks good.
-
Trac:
assets.zip I updated the assets:
Trac:
Status: needs_revision to needs_review-
Looks good now, thank! Applied to
tor-browser-60.3.0esr-8.5-1(commit e5659b39d8bc51faecac5daa7f2cb9b63d4dccf8).FWIW: I found a fun bug while testing (thinking first it is related to this patch) where the security state is vanishing while a download is happening if one clicks on the lock icon. But that's actually a Fennec issue which is tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1511003.
Trac:
Resolution: N/A to fixed
Status: needs_review to closed mentioned in issue #34377 (moved)
mentioned in issue tpo/applications/tor-browser#34377 (moved)
mentioned in issue tpo/applications/android-components#40015 (closed)
