Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linux-client): generate firezone-id (device ID) automatically if it's not provided at launch #3920

Merged
merged 29 commits into from
Mar 8, 2024

Conversation

ReactorScram
Copy link
Collaborator

@ReactorScram ReactorScram commented Mar 4, 2024

Closes #3815

Changes that are breaking (but these aren't in production so it should be okay)

  • Windows, renaming device_id.json to firezone-id.json to match the rest of the code
  • Linux GUI, storing the firezone-id under /var/lib instead of under $HOME
  • Linux GUI, bails out if not run with sudo --preserve-env by detecting $HOME == root or $USER != root

@ReactorScram ReactorScram added kind/feature New feature or request area/linux_client Linux client labels Mar 4, 2024
@ReactorScram ReactorScram self-assigned this Mar 4, 2024
Copy link

vercel bot commented Mar 4, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
firezone ⬜️ Ignored (Inspect) Visit Preview Mar 7, 2024 3:39pm

Copy link

github-actions bot commented Mar 4, 2024

Terraform Cloud Plan Output

Plan: 8 to add, 7 to change, 2 to destroy.

Terraform Cloud Plan

Copy link

github-actions bot commented Mar 4, 2024

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 65.8 MiB (+0%) 66.3 MiB (+0%) 39 (-22%)
direct-tcp-server2client 53.1 MiB (-3%) 53.4 MiB (-3%) 38 (+27%)
relayed-tcp-client2server 29.2 MiB (+3%) 32.3 MiB (+4%) 0 (-100%)
relayed-tcp-server2client 28.2 MiB (-1%) 30.3 MiB (+1%) 149 (-11%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 50.0 MiB (-0%) 0.27ms (+41%) 0.00% (NaN%)
direct-udp-server2client 50.0 MiB (+0%) 0.24ms (+17%) 0.00% (NaN%)
relayed-udp-client2server 50.0 MiB (-0%) 0.35ms (+58%) 19.99% (-20%)
relayed-udp-server2client 50.0 MiB (+0%) 0.71ms (+45%) 19.76% (+7%)

@@ -113,7 +113,6 @@ services:
FIREZONE_TOKEN: "n.SFMyNTY.g2gDaANtAAAAJGM4OWJjYzhjLTkzOTItNGRhZS1hNDBkLTg4OGFlZjZkMjhlMG0AAAAkN2RhN2QxY2QtMTExYy00NGE3LWI1YWMtNDAyN2I5ZDIzMGU1bQAAACtBaUl5XzZwQmstV0xlUkFQenprQ0ZYTnFJWktXQnMyRGR3XzJ2Z0lRdkZnbgYAGUmu74wBYgABUYA.UN3vSLLcAMkHeEh5VHumPOutkuue8JA6wlxM9JxJEPE"
RUST_LOG: firezone_linux_client=trace,wire=trace,connlib_client_shared=trace,firezone_tunnel=trace,connlib_shared=trace,boringtun=debug,snownet=debug,str0m=debug,info
FIREZONE_API_URL: ws://api:8081
FIREZONE_ID: D0455FDE-8F65-4960-A778-B934E4E85A5F
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove FIREZONE_ID from docker-compose to prove that the change works.

@@ -1,7 +1,7 @@
use tokio::fs;
use std::fs;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave up on tokio::fs for this since the Linux CLI client doesn't have a Tokio runtime. I personally like async FS operations but it's not a big deal, especially since it's only used at startup.

@@ -9,6 +9,7 @@ pub mod control;
pub mod error;
pub mod messages;

pub mod device_id;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module moved upstream to connlib_shared so that the Linux CLI, Linux GUI, and Windows GUI can all share it.

@@ -19,10 +19,16 @@ fn main() -> Result<()> {
handle,
};

// AKA "Device ID", not the Firezone slug
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case I forget again

@@ -54,6 +54,7 @@ pub(crate) fn setup(log_filter: &str) -> Result<Handles, Error> {
);
}
LogTracer::init()?;
tracing::debug!(?log_path, "Log path");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used to debug CI. Switching between sudo and non-sudo makes the logs show up in different places, and it's easier just to double-check in stderr on a dev system with a terminal.

@@ -110,7 +110,8 @@ pub(crate) async fn save(settings: &AdvancedSettings) -> Result<()> {
.parent()
.context("settings path should have a parent")?;
tokio::fs::create_dir_all(dir).await?;
tokio::fs::write(path, serde_json::to_string(settings)?).await?;
tokio::fs::write(&path, serde_json::to_string(settings)?).await?;
tracing::debug!(?path, "Saved settings");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used for CI debugging

@ReactorScram ReactorScram marked this pull request as ready for review March 5, 2024 18:41
@jamilbk
Copy link
Member

jamilbk commented Mar 5, 2024

Linux GUI, storing the firezone-id under /var/lib instead of under $HOME

Makes sense. This will be needed in #3713

Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! I'll leave the approval to @conectado for the Rust bits.

/// Only properly implemented on Linux and Windows (platforms with Tauri and headless client)
pub mod device_id;

// Must be compiled on Mac so the Mac runner can do `version-check` on `linux-client`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I can refactor that if it continues to be a nuissance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember what I needed it for, but I did just stub out the Tauri client for macOS in #3977 so I can do some of the checks locally without waiting on CI. So that might go in soon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could rename linux-client to cli-client too so it's not weird to say that a Mac might compile the "Linux" client even if headless Mac clients are not supported for production.

github-merge-queue bot pushed a commit that referenced this pull request Mar 5, 2024
This would allow me to fix compile / check / fmt errors from macOS CI
runners locally, e.g.
#3920 (comment)

It can't sign in or start connlib, but it shows the GUI


![image](https://github.com/firezone/firezone/assets/13400041/5307b66e-9874-4fe5-a6a6-43082dd6e385)

---------

Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Co-authored-by: User <user@Users-MacBook-Pro.local>
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@ReactorScram ReactorScram marked this pull request as draft March 5, 2024 22:42
@ReactorScram
Copy link
Collaborator Author

Taking this back to drafts since Jamil reminded me that sudo --preserve-env would make this all much simpler.

@ReactorScram ReactorScram marked this pull request as ready for review March 5, 2024 23:14
@jamilbk jamilbk removed kind/feature New feature or request area/linux_client Linux client labels Mar 6, 2024
Copy link
Collaborator

@conectado conectado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just left a couple of comments

rust/connlib/shared/src/device_id.rs Outdated Show resolved Hide resolved
// Try to read it from the disk
if let Some(j) = fs::read_to_string(&path)
.ok()
.and_then(|s| serde_json::from_str::<DeviceIdJson>(&s).ok())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any point on using DeviceIdJson it seems that we only read and write the id we could just use a plain string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking in case we ever need to add other fields or version it for any reason. I can change it to a string but I'll have to delete the ID file on my dev systems and ask any beta customers who have used it to do the same, or the whole JSON object will be the ID. (Which the portal can probably handle, it'll just look odd)

Or I could stick a schema version in advanced_settings and then it can do the migration automatically (If there's no version stored, rewrite firezone-id, if there is a version, leave it alone) but then it's not atomic since it's two files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON is probably fine, we use it already for log storage.

Not sure how the other settings are stored, but you could even stick it in there theoretically if we want to persist settings across installs

@@ -28,7 +29,7 @@ resolv-conf = "0.7.0"
serde = { version = "1.0", default-features = false, features = ["derive", "std"] }
serde_json = { version = "1.0", default-features = false, features = ["std"] }
thiserror = { version = "1.0", default-features = false }
tokio = { version = "1.36", default-features = false, features = ["rt", "rt-multi-thread"]}
tokio = { version = "1.36", default-features = false, features = ["fs", "rt", "rt-multi-thread"]}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't part of the change but I noticed it when running Cargo clippy, I guess the dependency solver had been adding fs to Tokio but this crate wasn't asking for it directly.

Is there a way to find these problems automatically?

Comment on lines +8 to +25
// Must use `eprintln` here because `tracing` won't be initialized yet.

let user = std::env::var("USER").context("USER env var should be set")?;
if user != "root" {
eprintln!("Firezone must run with root permissions to set up DNS. Re-run it with `sudo --preserve-env`");
return Ok(false);
}
let home = std::env::var("HOME").context("HOME env var should be set")?;
if home == "/root" {
eprintln!("If Firezone is run with `$HOME == /root`, deep links will not work. Re-run it with `sudo --preserve-env`");
// If we don't bail out here, this message will probably never be read.
return Ok(false);
}
Ok(true)
}

pub(crate) fn elevate() -> Result<()> {
todo!()
anyhow::bail!("Firezone does not self-elevate on Linux.");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added all this stuff as part of debugging, when trying to get sudo working just right in CI. It's not directly related to firezone-id but it should go into main eventually.

@ReactorScram ReactorScram added this pull request to the merge queue Mar 8, 2024
Merged via the queue into main with commit 7211e88 Mar 8, 2024
65 checks passed
@ReactorScram ReactorScram deleted the feat/generate-firezone-id branch March 8, 2024 16:26
github-merge-queue bot pushed a commit that referenced this pull request Mar 19, 2024
Closes #3961 

No tests yet, might be tricky to test since it's all I/O. 
I cued it off the device ID being generated, so it will have a minor
merge conflict with #3920

```[tasklist]
### Before merging
- [ ] UI polish, or disable the welcome screen temporarily
```

<img width="664" alt="image"
src="https://github.com/firezone/firezone/assets/13400041/d5def59c-b075-4135-91e5-85f9f9212fa5">

---------

Co-authored-by: Jamil Bou Kheir <jamilbk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make firezone-id optional for linux
3 participants