feat(clients): use hardware id for device verification by conectado · Pull Request #6857 · firezone/firezone

conectado · 2024-09-27T21:38:20Z

We want to associate additional device information for the device verification, these are all parameters that tries to uniquely identify the hardware.

For that reason we read system information and send it as part of the query params to the portal, that way the portal can store this when device is verified and match against that later on.

These are the parameters according to each platform:

Platform	Query Field	Field Meaning
MacOS	`device_serial`	Hardware's Serial
MacOS	`device_uuid`	Hardware's UUID
iOS	`identifier_for_vendor`	Identifier for vendor, resets only on uninstall/install
Android	`firebase_installation_id`	Firebase installation ID, resets only on uninstall/install
Windows	`device_serial`	Motherboard's Serial
Linux	`device_serial`	Motherboard's Serial

Fixes #6837

vercel · 2024-09-27T21:38:23Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
firezone	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 2, 2024 2:45am

conectado · 2024-09-30T15:16:14Z

@jamilbk this is practically ready but we should decide if we keep the fallback or maybe skip smbios completely?

conectado · 2024-09-30T15:17:55Z

Before merging I also still need to test on iOS

conectado · 2024-09-30T15:21:42Z

Also to note here, after merging this PR even when we fallback, due to no longer hashing the id, this will un-verify all new clients, so this might be worth a minor version change?

jamilbk · 2024-09-30T15:29:07Z

Also to note here, after merging this PR even when we fallback, due to no longer hashing the id, this will un-verify all new clients, so this might be worth a minor version change?

I don't think many are using this feature yet, so I'll just let the customer know directly.

thomaseizinger

Overall looks good. From the issue it sounds like there are more changes coming though :)

website/src/components/Changelog/Android.tsx

website/src/components/Changelog/Apple.tsx

website/src/components/Changelog/GUI.tsx

website/src/components/Changelog/Headless.tsx

kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt

rust/phoenix-channel/src/login_url.rs

conectado · 2024-10-02T02:45:21Z

@thomaseizinger Ready for another round of reviews

thomaseizinger

LGTM

I think we should clean up the error handling but that can happen in a follow-up!

thomaseizinger · 2024-10-02T08:39:35Z

kotlin/android/app/src/main/java/dev/firezone/android/tunnel/TunnelService.kt

+            val executor = Executors.newSingleThreadExecutor()
+
+            executor.execute {
+                val deviceInfo = DeviceInfo()
+
+                runCatching {
+                    Tasks.await(FirebaseInstallations.getInstance().id)
+                }.onSuccess { firebaseInstallationId ->
+                    deviceInfo.firebaseInstallationId = firebaseInstallationId
+                }.onFailure { exception ->
+                    Log.d(TAG, "Failed to obtain firebase installation id: $exception")
+                }
+
+                val gson: Gson =
+                    GsonBuilder()
+                        .setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES)
+                        .create()
+
+                connlibSessionPtr =
+                    ConnlibSession.connect(
+                        apiUrl = config.apiUrl,
+                        token = token,
+                        deviceId = deviceId(),
+                        deviceName = getDeviceName(),
+                        osVersion = Build.VERSION.RELEASE,
+                        logDir = getLogDir(),
+                        logFilter = config.logFilter,
+                        callback = callback,
+                        deviceInfo = gson.toJson(deviceInfo),
+                    )
+
+                startNetworkMonitoring()
+                startDisconnectMonitoring()
+            }


This has gotten a bit verbose which was the main reason I suggested to do it elsewhere. I don't know how long it takes to fetch the Firebase ID, I guess once it is cached locally it shouldn't delay the connect much?

thomaseizinger · 2024-10-02T08:40:57Z

rust/connlib/clients/apple/src/lib.rs

        install_rustls_crypto_provider();

        let secret = SecretString::from(token);
+        let device_info = serde_json::from_str(&device_info).unwrap();


Suggested change

let device_info = serde_json::from_str(&device_info).unwrap();

let device_info = serde_json::from_str(&device_info)?;

thomaseizinger · 2024-10-02T08:41:12Z

rust/connlib/clients/android/src/lib.rs

    let log_filter = string_from_jstring!(env, log_filter);
+    let device_info = string_from_jstring!(env, device_info);
+
+    let device_info = serde_json::from_str(&device_info).unwrap();


Suggested change

let device_info = serde_json::from_str(&device_info).unwrap();

let device_info = serde_json::from_str(&device_info)?;

jamilbk · 2024-10-02T17:27:44Z

@conectado Hm I thought we'd cut clients before this got merged. Are the portal changes already deployed to prod? Otherwise we need to revert this, cut clients, and merge this when we can deploy them to prod right?

conectado · 2024-10-02T17:32:44Z

@conectado Hm I thought we'd cut clients before this got merged. Are the portal changes already deployed to prod? Otherwise we need to revert this, cut clients, and merge this when we can deploy them to prod right?

@jamilbk We just need to revert the changes to hashing the device id, which we should anyways, that was a left over from the first implementation. Everything else should be harmless as we only add fields not remove it.

jamilbk · 2024-10-02T17:34:40Z

@conectado Got it, thanks for the clarification. If the only breakage here is that it resets verified clients, I'll check prod to see who's using that feature and then just notify them directly.

conectado · 2024-10-02T17:39:21Z

@conectado Got it, thanks for the clarification. If the only breakage here is that it resets verified clients, I'll check prod to see who's using that feature and then just notify them directly.

@jamilbk yes, that should be it. Though shouldn't we just revert that change? I'm not sure why we hashed the id in the first place but there's no point to making this change anymore

thomaseizinger · 2024-10-02T21:07:12Z

@conectado Got it, thanks for the clarification. If the only breakage here is that it resets verified clients, I'll check prod to see who's using that feature and then just notify them directly.

@jamilbk yes, that should be it. Though shouldn't we just revert that change? I'm not sure why we hashed the id in the first place but there's no point to making this change anymore

I'll send a PR for that so we are unblocked from releasing.

conectado · 2024-10-02T21:11:35Z

@conectado Got it, thanks for the clarification. If the only breakage here is that it resets verified clients, I'll check prod to see who's using that feature and then just notify them directly.

@jamilbk yes, that should be it. Though shouldn't we just revert that change? I'm not sure why we hashed the id in the first place but there's no point to making this change anymore

I'll send a PR for that so we are unblocked from releasing.

Thank you @thomaseizinger ! I didn't find the time to sit down and do it myself

thomaseizinger · 2024-10-02T23:13:28Z

PR here: #6918.

As part of iterating on the correct approach in #6857, we at some point removed the hashing of the Firezone-generated device ID. This will break customers because all of the device IDs as seen by the portal are changing. We've since settled on a different approach for device verification. To not break anyone, we are re-introducing hashing of the device ID. Related: #6857.

jamilbk · 2024-10-03T03:46:15Z

@conectado Was there a reason why we are not also pulling the SMBIOS UUID for PC systems? That way we have field parity for all desktop systems, similar to what the WARP client pulls.

I left a comment regarding this here: #6837 (comment)

jamilbk · 2024-10-03T04:10:30Z

Fixed here: #6921

Addresses the comment here: #6857 (comment)

vercel bot deployed to Preview September 27, 2024 21:39 View deployment

conectado changed the title ~~Feat/connlib/use hardware id for device verification~~ feat(clients): use hardware id for device verification Sep 28, 2024

vercel bot deployed to Preview September 28, 2024 22:42 View deployment

vercel bot deployed to Preview September 30, 2024 14:51 View deployment

conectado force-pushed the feat/connlib/use-hardware-id-for-device-verification branch from 5a81233 to fbe3e57 Compare September 30, 2024 14:53

vercel bot deployed to Preview September 30, 2024 14:55 View deployment

vercel bot deployed to Preview September 30, 2024 15:06 View deployment

conectado marked this pull request as ready for review September 30, 2024 15:15

conectado requested review from ReactorScram, jamilbk and thomaseizinger and removed request for ReactorScram September 30, 2024 15:16

vercel bot deployed to Preview September 30, 2024 15:17 View deployment

vercel bot deployed to Preview September 30, 2024 15:20 View deployment

vercel bot deployed to Preview September 30, 2024 15:24 View deployment

vercel bot deployed to Preview September 30, 2024 15:34 View deployment

vercel bot deployed to Preview September 30, 2024 15:46 View deployment

thomaseizinger reviewed Sep 30, 2024

View reviewed changes

vercel bot deployed to Preview October 1, 2024 02:08 View deployment

conectado marked this pull request as draft October 1, 2024 02:24

thomaseizinger reviewed Oct 1, 2024

View reviewed changes

rust/phoenix-channel/src/login_url.rs Outdated Show resolved Hide resolved

vercel bot deployed to Preview October 1, 2024 19:04 View deployment

vercel bot deployed to Preview October 2, 2024 01:32 View deployment

vercel bot deployed to Preview October 2, 2024 02:07 View deployment

conectado added 3 commits October 1, 2024 23:35

chore(connlib): use struct instead of dict for device info

e39a9b3

chore(headless-client): fix for structured device info

e04019c

update changelogs

01b9ff2

conectado force-pushed the feat/connlib/use-hardware-id-for-device-verification branch from 2d1fb47 to 01b9ff2 Compare October 2, 2024 02:36

vercel bot deployed to Preview October 2, 2024 02:38 View deployment

fix kotlin lint

add7f40

conectado marked this pull request as ready for review October 2, 2024 02:45

conectado requested a review from thomaseizinger October 2, 2024 02:45

vercel bot deployed to Preview October 2, 2024 02:45 View deployment

conectado enabled auto-merge October 2, 2024 02:54

thomaseizinger approved these changes Oct 2, 2024

View reviewed changes

conectado added this pull request to the merge queue Oct 2, 2024

Merged via the queue into main with commit 3501d5b Oct 2, 2024

conectado deleted the feat/connlib/use-hardware-id-for-device-verification branch October 2, 2024 08:58

thomaseizinger mentioned this pull request Oct 2, 2024

fix(connlib): reintroduce device-ID hashing #6918

Merged

jamilbk mentioned this pull request Oct 3, 2024

fix(connlib): also read device_uuid for PC systems #6921

Merged

github-merge-queue bot pushed a commit that referenced this pull request Oct 3, 2024

fix(connlib): also read device_uuid for PC systems (#6921)

a95a2a2

Addresses the comment here: #6857 (comment)

thomaseizinger mentioned this pull request Oct 7, 2024

chore(rust/telemetry): add firezone ID (formerly device ID) to sentry as a tag #6946

Merged

	let device_info = serde_json::from_str(&device_info).unwrap();
	let device_info = serde_json::from_str(&device_info)?;

Conversation

conectado commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conectado commented Sep 30, 2024

Uh oh!

conectado commented Sep 30, 2024

Uh oh!

conectado commented Sep 30, 2024

Uh oh!

jamilbk commented Sep 30, 2024

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

conectado commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

jamilbk commented Oct 2, 2024

Uh oh!

conectado commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamilbk commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conectado commented Oct 2, 2024

Uh oh!

thomaseizinger commented Oct 2, 2024

Uh oh!

conectado commented Oct 2, 2024

Uh oh!

thomaseizinger commented Oct 2, 2024

Uh oh!

jamilbk commented Oct 3, 2024

Uh oh!

jamilbk commented Oct 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

conectado commented Sep 27, 2024 •

edited

Loading

vercel bot commented Sep 27, 2024 •

edited

Loading

conectado commented Oct 2, 2024 •

edited

Loading

conectado commented Oct 2, 2024 •

edited

Loading

jamilbk commented Oct 2, 2024 •

edited

Loading