-
Notifications
You must be signed in to change notification settings - Fork 3
Add integrity checks during package upload #71
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
Changes from all commits
e87bae4
e458e73
7f7f21a
68064c7
334134a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,7 @@ use config::Towerfile; | |||||||||
| use tokio_tar::{Archive, Builder}; | ||||||||||
| use glob::glob; | ||||||||||
| use tmpdir::TmpDir; | ||||||||||
| use sha2::{Sha256, Digest}; | ||||||||||
|
|
||||||||||
| use async_compression::tokio::write::GzipEncoder; | ||||||||||
| use async_compression::tokio::bufread::GzipDecoder; | ||||||||||
|
|
@@ -54,6 +55,10 @@ pub struct Manifest { | |||||||||
| // modules_dir_name is the name of the modules directory within the package. | ||||||||||
| #[serde(default)] | ||||||||||
| pub modules_dir_name: String, | ||||||||||
|
|
||||||||||
| // checksum contains a hash of all the content in the package. | ||||||||||
| #[serde(default)] | ||||||||||
| pub checksum: String, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| impl Manifest { | ||||||||||
|
|
@@ -165,6 +170,7 @@ impl Package { | |||||||||
| import_paths: vec![], | ||||||||||
| app_dir_name: "app".to_string(), | ||||||||||
| modules_dir_name: "modules".to_string(), | ||||||||||
| checksum: "".to_string(), | ||||||||||
| }, | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -202,6 +208,11 @@ impl Package { | |||||||||
| let gzip = GzipEncoder::new(file); | ||||||||||
| let mut builder = Builder::new(gzip); | ||||||||||
|
|
||||||||||
| // These help us compute the integrity of the package contents overall. For each path, we'll | ||||||||||
| // store a hash of the contents written to the file. Then we'll hash the final content to | ||||||||||
| // create a fingerprint of the data. | ||||||||||
| let mut path_hashes = HashMap::new(); | ||||||||||
|
|
||||||||||
| // If the user didn't specify anything here we'll package everything under this directory | ||||||||||
| // and ship it to Tower. | ||||||||||
| let mut file_globs = spec.file_globs.clone(); | ||||||||||
|
|
@@ -228,6 +239,10 @@ impl Package { | |||||||||
| for (physical_path, logical_path) in file_paths { | ||||||||||
| // All of the app code goes into the "app" directory. | ||||||||||
| let logical_path = app_dir.join(logical_path); | ||||||||||
|
|
||||||||||
| let hash = compute_sha256_file(&physical_path).await?; | ||||||||||
| path_hashes.insert(logical_path.clone(), hash); | ||||||||||
|
|
||||||||||
| builder.append_path_with_name(physical_path, logical_path).await?; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -253,6 +268,10 @@ impl Package { | |||||||||
| // Now we write all of these paths to the modules directory. | ||||||||||
| for (physical_path, logical_path) in file_paths { | ||||||||||
| let logical_path = module_dir.join(logical_path); | ||||||||||
|
|
||||||||||
| let hash = compute_sha256_file(&physical_path).await?; | ||||||||||
| path_hashes.insert(logical_path.clone(), hash); | ||||||||||
|
|
||||||||||
| debug!("adding file {}", logical_path.display()); | ||||||||||
| builder.append_path_with_name(physical_path, logical_path).await?; | ||||||||||
| } | ||||||||||
|
|
@@ -266,6 +285,7 @@ impl Package { | |||||||||
| schedule: spec.schedule, | ||||||||||
| app_dir_name: app_dir.to_string_lossy().to_string(), | ||||||||||
| modules_dir_name: module_dir.to_string_lossy().to_string(), | ||||||||||
| checksum: compute_sha256_package(&path_hashes)?, | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| // the whole manifest needs to be written to a file as a convenient way to avoid having to | ||||||||||
|
|
@@ -282,11 +302,10 @@ impl Package { | |||||||||
| "Towerfile", | ||||||||||
| ).await?; | ||||||||||
|
|
||||||||||
| // We'll need to delete the lines above here. | ||||||||||
| let mut gzip = builder.into_inner().await?; | ||||||||||
| gzip.shutdown().await?; | ||||||||||
|
|
||||||||||
| //// probably not explicitly required; however, makes the test suite pass so... | ||||||||||
| // probably not explicitly required; however, makes the test suite pass so... | ||||||||||
| let mut file = gzip.into_inner(); | ||||||||||
| file.shutdown().await?; | ||||||||||
|
|
||||||||||
|
|
@@ -490,3 +509,50 @@ fn should_ignore_file(p: &PathBuf) -> bool { | |||||||||
|
|
||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| fn compute_sha256_package(path_hashes: &HashMap<PathBuf, String>) -> Result<String, Error> { | ||||||||||
| let mut sorted_keys: Vec<&PathBuf> = path_hashes.keys().collect(); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe dumb question, but it seems we're passing in a full (relative?) path, whereas in the go code we're hashing the filename — are these the same?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think their cooincidentally the same. I need to do more research. This is especially interesting in the case of e.g. Windows. Let's ship this, and I'll follow up with a bug fix accordingly. |
||||||||||
| sorted_keys.sort(); | ||||||||||
|
|
||||||||||
| // hasher that we'll use for computing the overall SHA256 hash. | ||||||||||
| let mut hasher = Sha256::new(); | ||||||||||
|
|
||||||||||
| for key in sorted_keys { | ||||||||||
| // We need to sort the keys so that we can compute a consistent hash. | ||||||||||
| let value = path_hashes.get(key).unwrap(); | ||||||||||
|
||||||||||
| let value = path_hashes.get(key).unwrap(); | |
| let value = path_hashes.get(key).expect("Key not found in path_hashes during SHA256 computation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using unwrap() here is unsafe since we're iterating over keys from the same HashMap
what?
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using unwrap() is unsafe here. Since we're iterating over keys from the same HashMap, this should never fail, but it's better to handle the error case properly or use a more explicit approach like path_hashes[key].
| let value = path_hashes.get(key).unwrap(); | |
| let value = path_hashes.get(key).expect("Key not found in path_hashes"); |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The function is marked as public but appears to be an internal implementation detail. Consider making it private or moving it to a separate module if it needs to be shared.
| pub async fn compute_sha256_file(file_path: &PathBuf) -> Result<String, Error> { | |
| async fn compute_sha256_file(file_path: &PathBuf) -> Result<String, Error> { |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This function is marked as pub but appears to be an internal utility. Consider making it private or moving it to a separate module if it needs to be shared between crates.
| pub async fn compute_sha256_file(file_path: &PathBuf) -> Result<String, Error> { | |
| async fn compute_sha256_file(file_path: &PathBuf) -> Result<String, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is generic and doesn't mention the specific failure (checksum computation). Consider making it more specific: 'Tower CLI failed to compute package checksum during deployment preparation.'