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: custom error types. #162

Merged
merged 1 commit into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
anyhow = "1.0"
async-std = "1.9"
async-stream = "0.3"
async-trait = "0.1"
Expand All @@ -36,6 +35,7 @@ sigstore = { version = "0.7.2", default-features = false, features = [
"tuf",
"cached-client",
] }
thiserror = "1.0"
tracing = "0.1"
url = { version = "2.2", features = ["serde"] }
walkdir = "2"
Expand Down
41 changes: 41 additions & 0 deletions src/errors.rs
flavio marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
use thiserror::Error;

pub type FetcherResult<T> = std::result::Result<T, FetcherError>;

#[derive(Error, Debug)]
pub enum FetcherError {
#[error("cannot retrieve path from uri: {0}")]
InvalidFilePathError(String),
#[error("invalid wasm file")]
InvalidWasmFileError,
#[error("wasm module cannot be save to {0:?}: {1}")]
CannotWriteWasmModuleFile(String, #[source] std::io::Error),
#[error(transparent)]
PolicyError(#[from] crate::policy::DigestError),
#[error(transparent)]
VerifyError(#[from] crate::verify::errors::VerifyError),
#[error(transparent)]
RegistryError(#[from] crate::registry::errors::RegistryError),
#[error(transparent)]
UrlParserError(#[from] url::ParseError),
#[error(transparent)]
SourceError(#[from] crate::sources::SourceError),
#[error(transparent)]
StoreError(#[from] crate::store::errors::StoreError),
#[error(transparent)]
InvalidURLError(#[from] InvalidURLError),
#[error(transparent)]
CannotCreateStoragePathError(#[from] CannotCreateStoragePathError),
}

#[derive(thiserror::Error, Debug)]
#[error("{0}")]
pub struct FailedToParseYamlDataError(#[from] pub serde_yaml::Error);
Copy link
Member

Choose a reason for hiding this comment

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

This is not "wrapped" by the FetcherError enum, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not. But this error exists because it used in multiple modules. Those modules errors (SourceError and VerifyError) wraps this error.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we drop the pub attribute, or turn that into a pub(crate) if needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will cause warning while compiling because we will use a more private type (FailedToParseYamlDataError) in more public ones (SourceError and RegistryError). As far as I know, we still want this types public. Therefore, I would like to avoid this warnings. To more context about the warning:

warning: type `FailedToParseYamlDataError` is more private than the item `SourceError::FailedToParseYamlDataError::0`
  --> src/sources.rs:27:40
   |
27 |     FailedToParseYamlDataError(#[from] FailedToParseYamlDataError),
   |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^ field `SourceError::FailedToParseYamlDataError::0` is reachable at visibility `pub`
   |
note: but type `FailedToParseYamlDataError` is only usable at visibility `pub(crate)`
  --> src/errors.rs:33:1
   |
33 | pub(crate) struct FailedToParseYamlDataError(#[from] pub serde_yaml::Error);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: `#[warn(private_interfaces)]` on by default

warning: type `FailedToParseYamlDataError` is more private than the item `VerifyError::FailedToParseYamlDataError::0`
  --> src/verify/errors.rs:35:40
   |
35 |     FailedToParseYamlDataError(#[from] FailedToParseYamlDataError),
   |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^ field `VerifyError::FailedToParseYamlDataError::0` is reachable at visibility `pub`
   |
note: but type `FailedToParseYamlDataError` is only usable at visibility `pub(crate)`
  --> src/errors.rs:33:1
   |
33 | pub(crate) struct FailedToParseYamlDataError(#[from] pub serde_yaml::Error);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: `policy-fetcher` (lib) generated 2 warnings


#[derive(thiserror::Error, Debug)]
#[error("invalid URL: {0}")]
pub struct CannotCreateStoragePathError(#[from] pub std::io::Error);

#[derive(thiserror::Error, Debug)]
#[error("invalid URL: {0}")]
pub struct InvalidURLError(pub String);
Comment on lines +39 to +41
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this error, can't we use the FetcherError::InvalidURLError?

Copy link
Member Author

Choose a reason for hiding this comment

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

This error exists because it is used in multiple modules. Thus, it's not necessary to duplicate it all over the place. And there is a wrapper around it in the FetcherError because the FetcherError is the type used in the Result returned by the lib to the callers.

5 changes: 2 additions & 3 deletions src/fetcher.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use anyhow::Result;
use async_trait::async_trait;
use url::Url;

use crate::sources::Certificate;
use crate::{sources::Certificate, sources::SourceResult};

#[derive(Clone, Debug, PartialEq)]
pub(crate) enum ClientProtocol {
Expand All @@ -22,5 +21,5 @@ pub(crate) enum TlsVerificationMode {
#[async_trait]
pub(crate) trait PolicyFetcher {
// Download and return the bytes of the WASM module
async fn fetch(&self, url: &Url, client_protocol: ClientProtocol) -> Result<Vec<u8>>;
async fn fetch(&self, url: &Url, client_protocol: ClientProtocol) -> SourceResult<Vec<u8>>;
}
27 changes: 19 additions & 8 deletions src/https.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#![allow(clippy::upper_case_acronyms)]

use anyhow::{anyhow, Result};
use async_trait::async_trait;
use std::{
boxed::Box,
Expand All @@ -10,27 +9,39 @@ use url::Url;

use crate::fetcher::{ClientProtocol, PolicyFetcher, TlsVerificationMode};
use crate::sources::Certificate;
use crate::sources::SourceError;
use crate::sources::SourceResult;

// Struct used to reference a WASM module that is hosted on a HTTP(s) server
#[derive(Default)]
pub(crate) struct Https {}

impl TryFrom<&Certificate> for reqwest::Certificate {
type Error = anyhow::Error;
type Error = SourceError;

fn try_from(certificate: &Certificate) -> Result<Self> {
fn try_from(certificate: &Certificate) -> SourceResult<Self> {
match certificate {
Certificate::Der(certificate) => reqwest::Certificate::from_der(certificate)
.map_err(|err| anyhow!("could not load certificate as DER encoded: {}", err)),
Certificate::Pem(certificate) => reqwest::Certificate::from_pem(certificate)
.map_err(|err| anyhow!("could not load certificate as PEM encoded: {}", err)),
Certificate::Der(certificate) => {
reqwest::Certificate::from_der(certificate).map_err(|err| {
SourceError::InvalidCertificateError(format!(
"could not load certificate as DER encoded: {err}"
))
})
}
Certificate::Pem(certificate) => {
reqwest::Certificate::from_pem(certificate).map_err(|err| {
SourceError::InvalidCertificateError(format!(
"could not load certificate as PEM encoded: {err}"
))
})
}
}
}
}

#[async_trait]
impl PolicyFetcher for Https {
async fn fetch(&self, url: &Url, client_protocol: ClientProtocol) -> Result<Vec<u8>> {
async fn fetch(&self, url: &Url, client_protocol: ClientProtocol) -> SourceResult<Vec<u8>> {
let mut client_builder = reqwest::Client::builder();
match client_protocol {
ClientProtocol::Http => {}
Expand Down
73 changes: 49 additions & 24 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ extern crate reqwest;
extern crate rustls;
extern crate walkdir;

use anyhow::{anyhow, Result};
use errors::FetcherResult;
use std::boxed::Box;
use std::fs;
use store::errors::{StoreError, StoreResult};
use url::Url;

pub mod errors;
pub mod fetcher;
mod https;
pub mod policy;
Expand All @@ -16,6 +18,7 @@ pub mod sources;
pub mod store;
pub mod verify;

use crate::errors::{CannotCreateStoragePathError, FetcherError};
use crate::fetcher::{ClientProtocol, PolicyFetcher, TlsVerificationMode};
use crate::https::Https;
use crate::policy::Policy;
Expand Down Expand Up @@ -78,7 +81,7 @@ pub async fn fetch_policy(
url: &str,
destination: PullDestination,
sources: Option<&Sources>,
) -> Result<Policy> {
) -> FetcherResult<Policy> {
let url = parse_url(url)?;
match url.scheme() {
"file" => {
Expand All @@ -87,15 +90,17 @@ pub async fn fetch_policy(
uri: url.to_string(),
local_path: url
.to_file_path()
.map_err(|err| anyhow!("cannot retrieve path from uri {}: {:?}", url, err))?,
.map_err(|_| FetcherError::InvalidFilePathError(url.to_string()))?,
});
}
"registry" | "http" | "https" => Ok(()),
_ => Err(anyhow!("unknown scheme: {}", url.scheme())),
_ => Err(StoreError::UnknownSchemeError(url.scheme().to_owned())),
}?;
let (store, mut destination) = pull_destination(&url, &destination)?;
if let Some(store) = store {
store.ensure(&store.policy_full_path(url.as_str(), store::PolicyPath::PrefixOnly)?)?;
store
.ensure(&store.policy_full_path(url.as_str(), store::PolicyPath::PrefixOnly)?)
.map_err(CannotCreateStoragePathError)?;
}
match url.scheme() {
"registry" => {
Expand Down Expand Up @@ -134,11 +139,7 @@ pub async fn fetch_policy(
{
Err(err) => {
if !sources.is_insecure_source(&host_and_port(&url)?) {
return Err(anyhow!(
"the policy {} could not be downloaded due to error: {}",
url,
err
));
return Err(FetcherError::SourceError(err));
}
}
Ok(bytes) => return create_file_if_valid(&bytes, &destination, url.to_string()),
Expand All @@ -155,11 +156,14 @@ pub async fn fetch_policy(

match policy_fetcher.fetch(&url, ClientProtocol::Http).await {
Ok(bytes) => create_file_if_valid(&bytes, &destination, url.to_string()),
Err(e) => Err(anyhow!("could not pull policy {}: {}", url, e)),
Err(e) => Err(FetcherError::SourceError(e)),
}
}

fn client_protocol(url: &Url, sources: &Sources) -> Result<ClientProtocol> {
fn client_protocol(
url: &Url,
sources: &Sources,
) -> std::result::Result<ClientProtocol, errors::InvalidURLError> {
if let Some(certificates) = sources.source_authority(&host_and_port(url)?) {
return Ok(ClientProtocol::Https(
TlsVerificationMode::CustomCaCertificates(certificates),
Expand All @@ -168,7 +172,10 @@ fn client_protocol(url: &Url, sources: &Sources) -> Result<ClientProtocol> {
Ok(ClientProtocol::Https(TlsVerificationMode::SystemCa))
}

fn pull_destination(url: &Url, destination: &PullDestination) -> Result<(Option<Store>, PathBuf)> {
fn pull_destination(
url: &Url,
destination: &PullDestination,
) -> FetcherResult<(Option<Store>, PathBuf)> {
Ok(match destination {
PullDestination::MainStore => {
let store = Store::default();
Expand Down Expand Up @@ -196,19 +203,19 @@ fn pull_destination(url: &Url, destination: &PullDestination) -> Result<(Option<
// Helper function, takes the URL of the policy and allocates the
// right struct to interact with it
#[allow(clippy::box_default)]
fn url_fetcher(scheme: &str) -> Result<Box<dyn PolicyFetcher>> {
fn url_fetcher(scheme: &str) -> StoreResult<Box<dyn PolicyFetcher>> {
match scheme {
"http" | "https" => Ok(Box::new(Https::default())),
"registry" => Ok(Box::new(Registry::new())),
_ => Err(anyhow!("unknown scheme: {}", scheme)),
_ => Err(StoreError::UnknownSchemeError(scheme.to_owned())),
}
}

pub(crate) fn host_and_port(url: &Url) -> Result<String> {
pub(crate) fn host_and_port(url: &Url) -> std::result::Result<String, errors::InvalidURLError> {
Ok(format!(
"{}{}",
url.host_str()
.ok_or_else(|| anyhow!("invalid URL {}", url))?,
.ok_or_else(|| errors::InvalidURLError(url.to_string()))?,
url.port()
.map(|port| format!(":{}", port))
.unwrap_or_default(),
Expand All @@ -222,12 +229,13 @@ pub(crate) fn host_and_port(url: &Url) -> Result<String> {
// https://webassembly.github.io/spec/core/bikeshed/#binary-magic
const WASM_MAGIC_NUMBER: [u8; 4] = [0x00, 0x61, 0x73, 0x6D];

fn create_file_if_valid(bytes: &[u8], destination: &Path, url: String) -> Result<Policy> {
fn create_file_if_valid(bytes: &[u8], destination: &Path, url: String) -> FetcherResult<Policy> {
if !bytes.starts_with(&WASM_MAGIC_NUMBER) {
return Err(anyhow!("invalid wasm file"));
return Err(FetcherError::InvalidWasmFileError);
};
fs::write(destination, bytes)
.map_err(|e| anyhow!("wasm module cannot be save to {:?}: {}", destination, e))?;
fs::write(destination, bytes).map_err(|e| {
FetcherError::CannotWriteWasmModuleFile(destination.to_string_lossy().to_string(), e)
})?;

Ok(Policy {
uri: url,
Expand Down Expand Up @@ -359,7 +367,7 @@ mod tests {
port: Some(5000),
path: "/kubewarden/policies/test".to_string(),
}))]
fn url_parsing(#[case] url: &str, #[case] expected: anyhow::Result<UrlParseDetails>) {
fn url_parsing(#[case] url: &str, #[case] expected: FetcherResult<UrlParseDetails>) {
let res = parse_url(url);
println!("{} -> {:?}", url, res);
assert_eq!(res.is_ok(), expected.is_ok());
Expand Down Expand Up @@ -525,14 +533,31 @@ mod tests {
);
}

#[test]
fn save_wasm_files_to_invalid_path() {
//simulate a write with a invalid file name for linux and windows
let dest = Path::new(r"\/");
let file_contents = read_fixture(Path::new("simple.wasm"));

let outcome = create_file_if_valid(&file_contents, dest, "not relevant".to_string());
assert!(matches!(
outcome,
Err(FetcherError::CannotWriteWasmModuleFile(..))
));
}

#[rstest]
#[case("simple.wasm", true)]
#[case("auth-present.json", false)]
#[case::valid_wasm_file("simple.wasm", true)]
#[case::invalid_wasm_file("auth-present.json", false)]
fn save_only_wasm_files_to_disk(#[case] fixture_file: &str, #[case] success: bool) {
let dest = NamedTempFile::new().expect("Cannot create tmp file");
let file_contents = read_fixture(Path::new(fixture_file));

let outcome = create_file_if_valid(&file_contents, dest.path(), "not relevant".to_string());
assert_eq!(outcome.is_ok(), success);
assert_eq!(
matches!(outcome, Err(FetcherError::InvalidWasmFileError),),
!success
);
}
}
11 changes: 10 additions & 1 deletion src/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,17 @@ pub struct Policy {
pub local_path: PathBuf,
}

#[derive(thiserror::Error, Debug)]
#[error("cannot retrieve path from uri: {err}")]
pub struct DigestError {
#[from]
err: std::io::Error,
}

type PolicyResult<T> = std::result::Result<T, DigestError>;

impl Policy {
pub fn digest(&self) -> Result<String, std::io::Error> {
pub fn digest(&self) -> PolicyResult<String> {
let d = Sha256::digest(std::fs::read(&self.local_path)?);
Ok(format!("{:x}", d))
}
Expand Down
21 changes: 21 additions & 0 deletions src/registry/errors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use thiserror::Error;

use crate::errors::InvalidURLError;

pub type RegistryResult<T> = std::result::Result<T, RegistryError>;

#[derive(Error, Debug)]
pub enum RegistryError {
#[error("Fail to interact with OCI registry: {0}")]
OCIRegistryError(#[from] oci_distribution::errors::OciDistributionError),
#[error("Invalid OCI image reference: {0}")]
InvalidOCIImageReferenceError(#[from] oci_distribution::ParseError),
#[error("{0}")]
BuildImmutableReferenceError(String),
#[error("Invalid destination format")]
InvalidDestinationError,
#[error(transparent)]
UrlParserError(#[from] url::ParseError),
#[error(transparent)]
InvalidURLError(#[from] InvalidURLError),
}
Loading
Loading