From 17754823866bb4538845d18c9d2fe26756d56437 Mon Sep 17 00:00:00 2001 From: Mikail Bagishov Date: Sun, 14 Jun 2020 15:57:15 +0300 Subject: [PATCH] Use thiserror for error handling --- Cargo.toml | 3 +- examples/image-labels.rs | 5 +- src/errors.rs | 79 ++++++++++++++++---- src/lib.rs | 7 +- src/mediatypes.rs | 10 +-- src/reference.rs | 86 ++++++++++++++------- src/render.rs | 13 +++- src/v2/auth.rs | 89 +++++++++------------- src/v2/blobs.rs | 30 ++------ src/v2/catalog.rs | 10 +-- src/v2/content_digest.rs | 55 ++++++++------ src/v2/manifest/manifest_schema1.rs | 2 +- src/v2/manifest/manifest_schema2.rs | 12 +-- src/v2/manifest/mod.rs | 112 ++++++++++------------------ src/v2/mod.rs | 5 +- src/v2/tags.rs | 12 +-- tests/mock/base_client.rs | 2 +- tests/mod.rs | 4 - tests/net/quay_io/mod.rs | 4 +- 19 files changed, 276 insertions(+), 264 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8b1a81b1..ec736d5a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,6 @@ tag-prefix = "" [dependencies] base64 = "0.12" -error-chain = { version = "0.12", default-features = false } futures = "0.3" http = "0.2" libflate = "1.0" @@ -41,6 +40,8 @@ tokio = "0.2" reqwest = { version = "0.10", default-features = false, features = ["json"] } sha2 = "^0.9.0" async-stream = "0.3" +thiserror = "1.0.19" +url = "2.1.1" [dev-dependencies] dirs = "3.0" diff --git a/examples/image-labels.rs b/examples/image-labels.rs index a305eba1..e21aa3f9 100644 --- a/examples/image-labels.rs +++ b/examples/image-labels.rs @@ -70,10 +70,7 @@ async fn run( let version = dkr_ref.version(); let dclient = client.authenticate(&[&login_scope]).await?; - let manifest = match dclient.get_manifest(&image, &version).await { - Ok(manifest) => Ok(manifest), - Err(e) => Err(format!("Got error {}", e)), - }?; + let manifest = dclient.get_manifest(&image, &version).await?; if let Manifest::S1Signed(s1s) = manifest { let labels = s1s.get_labels(0); diff --git a/src/errors.rs b/src/errors.rs index 08999b41..4a3603cf 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -1,17 +1,68 @@ -//! Error chains, types and traits. +//! Defines root error type -error_chain! { - foreign_links { - Base64Decode(base64::DecodeError); - HeaderInvalid(http::header::InvalidHeaderValue); - HeaderParse(http::header::ToStrError); - Hyper(http::Error); - Io(std::io::Error); - Json(serde_json::Error); - Regex(regex::Error); - Reqwest(reqwest::Error); - UriParse(http::uri::InvalidUri); - Utf8Parse(std::string::FromUtf8Error); - StrumParse(strum::ParseError); +#[non_exhaustive] +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error("base64 decode error")] + Base64Decode(#[from] base64::DecodeError), + #[error("header parse error")] + HeaderParse(#[from] http::header::ToStrError), + #[error("json error")] + Json(#[from] serde_json::Error), + #[error("http transport error")] + Reqwest(#[from] reqwest::Error), + #[error("URI parse error")] + Uri(#[from] url::ParseError), + #[error("input is not UTF-8")] + Ut8Parse(#[from] std::string::FromUtf8Error), + #[error("strum error")] + StrumParse(#[from] strum::ParseError), + #[error("authentication information missing for index {0}")] + AuthInfoMissing(String), + #[error("unknown media type {0:?}")] + UnknownMimeType(mime::Mime), + #[error("unknown media type {0:?}")] + UnsupportedMediaType(crate::mediatypes::MediaTypes), + #[error("mime parse error")] + MimeParse(#[from] mime::FromStrError), + #[error("missing authentication header {0}")] + MissingAuthHeader(&'static str), + #[error("unexpected HTTP status {0}")] + UnexpectedHttpStatus(http::StatusCode), + #[error("invalid auth token '{0}'")] + InvalidAuthToken(String), + #[error("API V2 not supported")] + V2NotSupported, + #[error("obtained token is invalid")] + LoginReturnedBadToken, + #[error("www-authenticate header parse error")] + Www(#[from] crate::v2::WwwHeaderParseError), + #[error("request failed with status {status} and body of size {len}: {}", String::from_utf8_lossy(&body))] + Client { + status: http::StatusCode, + len: usize, + body: Vec, + }, + #[error("content digest error")] + ContentDigestParse(#[from] crate::v2::ContentDigestError), + #[error("no header Content-Type given and no workaround to apply")] + MediaTypeSniff, + #[error("manifest error")] + Manifest(#[from] crate::v2::manifest::ManifestError), + #[error("reference is invalid")] + ReferenceParse(#[from] crate::reference::ReferenceParseError), + #[error("requested operation requires that credentials are available")] + NoCredentials +} + +pub type Result = std::result::Result; + +#[cfg(test)] +mod tests { + use super::*; + #[test] + fn test_error_bounds() { + fn check_bounds() {} + check_bounds::(); } } diff --git a/src/lib.rs b/src/lib.rs index 2fdfc97b..35ed62b9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -36,8 +36,6 @@ #[macro_use] extern crate serde; #[macro_use] -extern crate error_chain; -#[macro_use] extern crate log; #[macro_use] extern crate strum_macros; @@ -48,10 +46,11 @@ pub mod reference; pub mod render; pub mod v2; -use errors::Result; +use errors::{Result, Error}; use std::collections::HashMap; use std::io::Read; + /// Default User-Agent client identity. pub static USER_AGENT: &str = "camallo-dkregistry/0.0"; @@ -71,7 +70,7 @@ pub fn get_credentials( }; let auth = match map.auths.get(real_index) { Some(x) => base64::decode(x.auth.as_str())?, - None => bail!("no auth for index {}", real_index), + None => return Err(Error::AuthInfoMissing(real_index.to_string())), }; let s = String::from_utf8(auth)?; let creds: Vec<&str> = s.splitn(2, ':').collect(); diff --git a/src/mediatypes.rs b/src/mediatypes.rs index 22d71653..2d2686fb 100644 --- a/src/mediatypes.rs +++ b/src/mediatypes.rs @@ -1,6 +1,6 @@ //! Media-types for API objects. -use crate::errors::{Error, Result}; +use crate::errors::{Result}; use mime; use strum::EnumProperty; @@ -62,13 +62,13 @@ impl MediaTypes { } ("vnd.docker.image.rootfs.diff.tar.gzip", _) => Ok(MediaTypes::ImageLayerTgz), ("vnd.docker.container.image.v1", "json") => Ok(MediaTypes::ContainerConfigV1), - _ => bail!("unknown mediatype {:?}", mtype), + _ => return Err(crate::Error::UnknownMimeType(mtype.clone())), } } - _ => bail!("unknown mediatype {:?}", mtype), + _ => return Err(crate::Error::UnknownMimeType(mtype.clone())), } } - pub fn to_mime(&self) -> Result { + pub fn to_mime(&self) -> mime::Mime { match self { &MediaTypes::ApplicationJson => Ok(mime::APPLICATION_JSON), ref m => { @@ -79,6 +79,6 @@ impl MediaTypes { } } } - .map_err(|e| Error::from(e.to_string())) + .expect("to_mime should be always successful") } } diff --git a/src/reference.rs b/src/reference.rs index 5298be30..b05806ff 100644 --- a/src/reference.rs +++ b/src/reference.rs @@ -29,7 +29,6 @@ // The `docker://` schema is not officially documented, but has a reference implementation: // https://github.com/docker/distribution/blob/v2.6.1/reference/reference.go -use crate::errors::Error; use regex; use std::collections::VecDeque; use std::str::FromStr; @@ -46,20 +45,30 @@ pub enum Version { Digest(String, String), } +#[derive(thiserror::Error, Debug)] +pub enum VersionParseError { + #[error("wrong digest format: checksum missing")] + WrongDigestFormat, + #[error("unknown prefix: digest must start from : or @")] + UnknownPrefix, + #[error("empty string is invalid digest")] + Empty, +} + impl str::FromStr for Version { - type Err = Error; + type Err = VersionParseError; fn from_str(s: &str) -> Result { let v = match s.chars().nth(0) { Some(':') => Version::Tag(s.trim_start_matches(':').to_string()), Some('@') => { let r: Vec<&str> = s.trim_start_matches('@').splitn(2, ':').collect(); if r.len() != 2 { - bail!("wrong digest format"); + return Err(VersionParseError::WrongDigestFormat); }; Version::Digest(r[0].to_string(), r[1].to_string()) } - Some(_) => bail!("unknown prefix"), - None => bail!("too short"), + Some(_) => return Err(VersionParseError::UnknownPrefix), + None => return Err(VersionParseError::Empty), }; Ok(v) } @@ -146,13 +155,32 @@ impl fmt::Display for Reference { } impl str::FromStr for Reference { - type Err = Error; + type Err = ReferenceParseError; fn from_str(s: &str) -> Result { parse_url(s) } } -fn parse_url(input: &str) -> Result { +#[derive(thiserror::Error, Debug)] +pub enum ReferenceParseError { + #[error("missing image name")] + MissingImageName, + #[error("version parse error")] + VersionParse(#[from] VersionParseError), + #[error("empty image name")] + EmptyImageName, + #[error("component '{component}' does not conform to regex '{regex}'")] + RegexViolation { + regex: &'static str, + component: String, + }, + #[error("empty repository name")] + EmptyRepositoryName, + #[error("repository name too long")] + RepositoryNameTooLong, +} + +fn parse_url(input: &str) -> Result { // TODO(lucab): investigate using a grammar-based parser. let mut rest = input; @@ -173,7 +201,7 @@ fn parse_url(input: &str) -> Result { // default registry if it's not. let first = components .pop_front() - .ok_or(Error::from("missing image name"))?; + .ok_or(ReferenceParseError::MissingImageName)?; let registry = if regex::Regex::new(r"(?x) ^ @@ -183,7 +211,7 @@ fn parse_url(input: &str) -> Result { # optional port ([:][0-9]{1,6})? $ - ")?.is_match(&first) { + ").expect("hardcoded regex is invalid").is_match(&first) { first } else { components.push_front(first); @@ -193,7 +221,7 @@ fn parse_url(input: &str) -> Result { // Take image name and extract tag or digest-ref, if any. let last = components .pop_back() - .ok_or_else(|| Error::from("missing image name"))?; + .ok_or(ReferenceParseError::MissingImageName)?; let (image_name, version) = match (last.rfind('@'), last.rfind(':')) { (Some(i), _) | (None, Some(i)) => { let s = last.split_at(i); @@ -201,7 +229,9 @@ fn parse_url(input: &str) -> Result { } (None, None) => (last, Version::default()), }; - ensure!(!image_name.is_empty(), "empty image name"); + if image_name.is_empty() { + return Err(ReferenceParseError::EmptyImageName); + } // Handle images in default library namespace, that is: // `ubuntu` -> `library/ubuntu` @@ -212,25 +242,27 @@ fn parse_url(input: &str) -> Result { // Check if all path components conform to the regex at // https://docs.docker.com/registry/spec/api/#overview. - let path_re = regex::Regex::new("^[a-z0-9]+(?:[._-][a-z0-9]+)*$")?; - components - .iter() - .try_for_each(|component| -> Result<(), Error> { - if !path_re.is_match(component) { - bail!( - "component '{}' doesn't conform to the regex '{}'", - component, - path_re.as_str() - ) - }; - - Ok(()) - })?; + const REGEX: &'static str = "^[a-z0-9]+(?:[._-][a-z0-9]+)*$"; + let path_re = regex::Regex::new(REGEX).expect("hardcoded regex is invalid"); + components.iter().try_for_each(|component| { + if !path_re.is_match(component) { + return Err(ReferenceParseError::RegexViolation { + component: component.clone(), + regex: REGEX, + }); + }; + + Ok(()) + })?; // Re-assemble repository name. let repository = components.into_iter().collect::>().join("/"); - ensure!(!repository.is_empty(), "empty repository name"); - ensure!(repository.len() <= 127, "repository name too long"); + if repository.is_empty() { + return Err(ReferenceParseError::EmptyRepositoryName); + } + if repository.len() > 127 { + return Err(ReferenceParseError::RepositoryNameTooLong); + } Ok(Reference { has_schema, diff --git a/src/render.rs b/src/render.rs index d1936e19..6b151aea 100644 --- a/src/render.rs +++ b/src/render.rs @@ -3,18 +3,25 @@ // Docker image format is specified at // https://github.com/moby/moby/blob/v17.05.0-ce/image/spec/v1.md -use crate::errors::*; use libflate::gzip; use std::{fs, path}; use tar; +#[derive(Debug, thiserror::Error)] +pub enum RenderError { + #[error("wrong target path {}: must be absolute path to existing directory", _0.display())] + WrongTargetPath(path::PathBuf), + #[error("io error")] + Io(#[from] std::io::Error) +} + /// Unpack an ordered list of layers to a target directory. /// /// Layers must be provided as gzip-compressed tar archives, with lower layers /// coming first. Target directory must be an existing absolute path. -pub fn unpack(layers: &[Vec], target_dir: &path::Path) -> Result<()> { +pub fn unpack(layers: &[Vec], target_dir: &path::Path) -> Result<(), RenderError> { if !target_dir.is_absolute() || !target_dir.exists() || !target_dir.is_dir() { - bail!("wrong target path"); + return Err(RenderError::WrongTargetPath(target_dir.to_path_buf())); } for l in layers { // Unpack layers diff --git a/src/v2/auth.rs b/src/v2/auth.rs index 973e8c73..1452f42a 100644 --- a/src/v2/auth.rs +++ b/src/v2/auth.rs @@ -1,7 +1,6 @@ use crate::errors::{Error, Result}; use crate::v2::*; use reqwest::{header::HeaderValue, RequestBuilder, StatusCode, Url}; -use std::iter::FromIterator; /// Represents all supported authentication schemes and is stored by `Client`. #[derive(Debug, Clone)] @@ -41,12 +40,7 @@ impl BearerAuth { let auth_ep = bearer_header_content.auth_ep(scopes); trace!("authenticate: token endpoint: {}", auth_ep); - let url = reqwest::Url::parse(&auth_ep).map_err(|e| { - Error::from(format!( - "failed to parse url from string '{}': {}", - auth_ep, e - )) - })?; + let url = reqwest::Url::parse(&auth_ep)?; let auth_req = { Client { @@ -65,14 +59,13 @@ impl BearerAuth { let status = r.status(); trace!("authenticate: got status {}", status); if status != StatusCode::OK { - bail!("authenticate: wrong HTTP status '{}'", status); + return Err(Error::UnexpectedHttpStatus(status)); } let bearer_auth = r.json::().await?; match bearer_auth.token.as_str() { - "unauthenticated" => bail!("token is unauthenticated"), - "" => bail!("received an empty token"), + "unauthenticated" | "" => return Err(Error::InvalidAuthToken(bearer_auth.token)), _ => {} }; @@ -103,6 +96,27 @@ pub(crate) enum WwwAuthenticateHeaderContent { Basic(WwwAuthenticateHeaderContentBasic), } +const REGEX: &str = r#"(?x)\s* +((?P[A-Z][a-z]+)\s*)? +( + \s* + (?P[a-z]+) + \s* + = + \s* + "(?P[^"]+)" + \s* +) +"#; + +#[derive(Debug, thiserror::Error)] +pub enum WwwHeaderParseError { + #[error("header value must conform to {}", REGEX)] + InvalidValue, + #[error("'method' field missing")] + FieldMethodMissing, +} + impl WwwAuthenticateHeaderContent { /// Create a `WwwAuthenticateHeaderContent` by parsing a `HeaderValue` instance. pub(crate) fn from_www_authentication_header(header_value: HeaderValue) -> Result { @@ -110,29 +124,14 @@ impl WwwAuthenticateHeaderContent { // This regex will result in multiple captures which will contain one key-value pair each. // The first capture will be the only one with the "method" group set. - let re = regex::Regex::new( - r#"(?x)\s* - ((?P[A-Z][a-z]+)\s*)? - ( - \s* - (?P[a-z]+) - \s* - = - \s* - "(?P[^"]+)" - \s* - ) - "#, - )?; + let re = regex::Regex::new(REGEX).expect("this static regex is valid"); let captures = re.captures_iter(&header).collect::>(); let method = captures .get(0) - .ok_or_else(|| { - Error::from(format!("regex '{}' didn't match '{}'", re.as_str(), header)) - })? + .ok_or(WwwHeaderParseError::InvalidValue)? .name("method") - .ok_or_else(|| Error::from(format!("method not found in {}", header)))? + .ok_or(WwwHeaderParseError::FieldMethodMissing)? .as_str() .to_string(); @@ -229,25 +228,15 @@ impl Client { async fn get_www_authentication_header(&self) -> Result { let url = { let ep = format!("{}/v2/", self.base_url.clone(),); - reqwest::Url::parse(&ep) - .map_err(|e| format!("failed to parse url from string '{}': {}", ep, e))? + reqwest::Url::parse(&ep)? }; - let r = self - .build_reqwest(Method::GET, url.clone()) - .send() - .map_err(|e| Error::from(format!("{}", e))) - .await?; + let r = self.build_reqwest(Method::GET, url.clone()).send().await?; trace!("GET '{}' status: {:?}", r.url(), r.status()); r.headers() .get(reqwest::header::WWW_AUTHENTICATE) - .ok_or_else(|| { - Error::from(format!( - "missing {:?} header", - reqwest::header::WWW_AUTHENTICATE - )) - }) + .ok_or(Error::MissingAuthHeader("WWW-Authenticate")) .map(ToOwned::to_owned) } @@ -272,7 +261,7 @@ impl Client { user, password: Some(password), }) - .ok_or("cannot authenticate without credentials")?; + .ok_or(Error::NoCredentials)?; Auth::Basic(basic_auth) } @@ -301,15 +290,7 @@ impl Client { pub async fn is_auth(&self) -> Result { let url = { let ep = format!("{}/v2/", self.base_url.clone(),); - match Url::parse(&ep) { - Ok(url) => url, - Err(e) => { - return Err(Error::from(format!( - "failed to parse url from string '{}': {}", - ep, e - ))); - } - } + Url::parse(&ep)? }; let req = self.build_reqwest(Method::GET, url.clone()); @@ -322,7 +303,7 @@ impl Client { match status { reqwest::StatusCode::OK => Ok(true), reqwest::StatusCode::UNAUTHORIZED => Ok(false), - _ => Err(format!("is_auth: wrong HTTP status '{}'", status).into()), + _ => Err(Error::UnexpectedHttpStatus(status)), } } } @@ -340,7 +321,7 @@ mod tests { let header_value = HeaderValue::from_str(&format!( r#"Bearer realm="{}",service="{}",scope="{}""#, realm, service, scope - ))?; + )).expect("this statically known header value only contains ASCII chars so it is correct header value"); let content = WwwAuthenticateHeaderContent::from_www_authentication_header(header_value)?; @@ -368,7 +349,7 @@ mod tests { fn basic_realm_parses_correctly() -> Result<()> { let realm = "Registry realm"; - let header_value = HeaderValue::from_str(&format!(r#"Basic realm="{}""#, realm))?; + let header_value = HeaderValue::from_str(&format!(r#"Basic realm="{}""#, realm)).unwrap(); let content = WwwAuthenticateHeaderContent::from_www_authentication_header(header_value)?; diff --git a/src/v2/blobs.rs b/src/v2/blobs.rs index ec14b4d5..0f35d0f2 100644 --- a/src/v2/blobs.rs +++ b/src/v2/blobs.rs @@ -8,15 +8,7 @@ impl Client { pub async fn has_blob(&self, name: &str, digest: &str) -> Result { let url = { let ep = format!("{}/v2/{}/blobs/{}", self.base_url, name, digest); - match reqwest::Url::parse(&ep) { - Ok(url) => url, - Err(e) => { - return Err(Error::from(format!( - "failed to parse url from string: {}", - e - ))); - } - } + reqwest::Url::parse(&ep)? }; let res = self.build_reqwest(Method::HEAD, url.clone()).send().await?; @@ -35,8 +27,7 @@ impl Client { let blob = { let ep = format!("{}/v2/{}/blobs/{}", self.base_url, name, digest); - let url = reqwest::Url::parse(&ep) - .map_err(|e| Error::from(format!("failed to parse url from string: {}", e)))?; + let url = reqwest::Url::parse(&ep)?; let res = self.build_reqwest(Method::GET, url.clone()).send().await?; @@ -47,10 +38,7 @@ impl Client { // Let client errors through to populate them with the body || status.is_client_error()) { - return Err(Error::from(format!( - "GET request failed with status '{}'", - status - ))); + return Err(Error::UnexpectedHttpStatus(status)); } let status = res.status(); @@ -61,22 +49,18 @@ impl Client { trace!("Successfully received blob with {} bytes ", len); Ok(body_vec) } else if status.is_client_error() { - Err(Error::from(format!( - "GET request failed with status '{}' and body of size {}: {:#?}", + Err(Error::Client { status, len, - String::from_utf8_lossy(&body_vec) - ))) + body: body_vec, + }) } else { // We only want to handle success and client errors here error!( "Received unexpected HTTP status '{}' after fetching the body. Please submit a bug report.", status ); - Err(Error::from(format!( - "GET request failed with status '{}'", - status - ))) + Err(Error::UnexpectedHttpStatus(status)) } }?; diff --git a/src/v2/catalog.rs b/src/v2/catalog.rs index 695083ad..8367f764 100644 --- a/src/v2/catalog.rs +++ b/src/v2/catalog.rs @@ -1,4 +1,4 @@ -use crate::errors::{Result, ResultExt}; +use crate::errors::Result; use crate::v2; use async_stream::try_stream; use futures::stream::Stream; @@ -23,8 +23,7 @@ impl v2::Client { }; let ep = format!("{}/v2/_catalog{}", self.base_url.clone(), suffix); - reqwest::Url::parse(&ep) - .chain_err(|| format!("failed to parse url from string '{}'", ep)) + reqwest::Url::parse(&ep).map_err(|err| crate::Error::from(err)) }; try_stream! { @@ -46,8 +45,7 @@ async fn fetch_catalog(req: RequestBuilder) -> Result { match status { StatusCode::OK => r .json::() - .await - .chain_err(|| "get_catalog: failed to fetch the whole body"), - _ => bail!("get_catalog: wrong HTTP status '{}'", status), + .await.map_err(Into::into), + _ => Err(crate::Error::UnexpectedHttpStatus(status)), } } diff --git a/src/v2/content_digest.rs b/src/v2/content_digest.rs index 0daa1556..165b5149 100644 --- a/src/v2/content_digest.rs +++ b/src/v2/content_digest.rs @@ -1,36 +1,47 @@ -use crate::errors::Result; /// Implements types and methods for content verification use sha2::{self, Digest}; /// ContentDigest stores a digest and its DigestAlgorithm #[derive(Clone, Debug, PartialEq)] -pub(crate) struct ContentDigest { +pub struct ContentDigest { digest: String, algorithm: DigestAlgorithm, } /// DigestAlgorithm declares the supported algorithms #[derive(Display, Clone, Debug, PartialEq, EnumString)] -enum DigestAlgorithm { +pub enum DigestAlgorithm { #[strum(to_string = "sha256")] Sha256, } +#[derive(Debug, thiserror::Error)] +pub enum ContentDigestError { + #[error("digest {0} does not have algorithm prefix")] + BadDigest(String), + #[error("unknown algorithm")] + AlgorithmUnknown(#[from] ::Err), + #[error("verification failed: expected '{expected}', got '{got}'")] + Verify { + expected: ContentDigest, + got: ContentDigest, + }, +} + impl ContentDigest { /// try_new attempts to parse the digest string and create a ContentDigest instance from it /// /// Success depends on /// - the string having a "algorithm:" prefix /// - the algorithm being supported by DigestAlgorithm - pub fn try_new(digest: String) -> Result { + pub fn try_new(digest: String) -> std::result::Result { let digest_split = digest.split(':').collect::>(); if digest_split.len() != 2 { - return Err(format!("digest '{}' does not have an algorithm prefix", digest).into()); + return Err(ContentDigestError::BadDigest(digest)); } - let algorithm = - std::str::FromStr::from_str(digest_split[0]).map_err(|e| format!("{}", e))?; + let algorithm = std::str::FromStr::from_str(digest_split[0])?; Ok(ContentDigest { digest: digest_split[1].to_string(), algorithm, @@ -40,17 +51,15 @@ impl ContentDigest { /// try_verify hashes the input slice and compares it with the digest stored in this instance /// /// Success depends on the result of the comparison - pub fn try_verify(&self, input: &[u8]) -> Result<()> { + pub fn try_verify(&self, input: &[u8]) -> std::result::Result<(), ContentDigestError> { let hash = self.algorithm.hash(input); let layer_digest = Self::try_new(hash)?; if self != &layer_digest { - return Err(format!( - "content verification failed. expected '{}', got '{}'", - self.to_owned(), - layer_digest.to_owned() - ) - .into()); + return Err(ContentDigestError::Verify { + expected: self.clone(), + got: layer_digest.clone(), + }); } trace!("content verification succeeded for '{}'", &layer_digest); @@ -78,7 +87,7 @@ impl DigestAlgorithm { #[cfg(test)] mod tests { use super::*; - type Fallible = Result; + type Fallible = Result; #[test] fn try_new_succeeds_with_correct_digest() -> Fallible<()> { @@ -92,22 +101,19 @@ mod tests { } #[test] - fn try_new_succeeds_with_incorrect_digest() -> Fallible<()> { + fn try_new_succeeds_with_incorrect_digest() { for incorrect_digest in &[ "invalid", "invalid:", "invalid:0000000000000000000000000000000000000000000000000000000000000000", ] { if ContentDigest::try_new(incorrect_digest.to_string()).is_ok() { - return Err(format!( + panic!( "expected try_new to fail for incorrect digest {}", incorrect_digest - ) - .into()); + ); } } - - Ok(()) } #[test] @@ -115,7 +121,9 @@ mod tests { let blob: &[u8] = b"somecontent"; let digest = DigestAlgorithm::Sha256.hash(&blob); - ContentDigest::try_new(digest)?.try_verify(&blob) + ContentDigest::try_new(digest)? + .try_verify(&blob) + .map_err(Into::into) } #[test] @@ -128,9 +136,8 @@ mod tests { .try_verify(&different_blob) .is_ok() { - return Err("expected try_verify to fail for a different blob".into()); + panic!("expected try_verify to fail for a different blob"); } - Ok(()) } } diff --git a/src/v2/manifest/manifest_schema1.rs b/src/v2/manifest/manifest_schema1.rs index 645b0428..611600ed 100644 --- a/src/v2/manifest/manifest_schema1.rs +++ b/src/v2/manifest/manifest_schema1.rs @@ -53,7 +53,7 @@ impl ManifestSchema1Signed { /// Get a collection of all image labels stored in the history array of this manifest. /// /// Note that for this manifest type any `layer` beyond 0 probably returns None. - pub fn get_labels(&self, layer: usize) -> Option<(HashMap)> { + pub fn get_labels(&self, layer: usize) -> Option> { Some( serde_json::from_str::(&self.history.get(layer)?.v1_compat) .ok()? diff --git a/src/v2/manifest/manifest_schema2.rs b/src/v2/manifest/manifest_schema2.rs index 19170ee7..9a069a9f 100644 --- a/src/v2/manifest/manifest_schema2.rs +++ b/src/v2/manifest/manifest_schema2.rs @@ -100,15 +100,7 @@ impl ManifestSchema2Spec { repo, self.config.digest ); - match reqwest::Url::parse(&ep) { - Ok(url) => url, - Err(e) => { - return Err(Error::from(format!( - "failed to parse url from string '{}': {}", - ep, e - ))); - } - } + reqwest::Url::parse(&ep)? }; let r = client @@ -120,7 +112,7 @@ impl ManifestSchema2Spec { trace!("GET {:?}: {}", url, &status); if !status.is_success() { - return Err(format!("wrong HTTP status '{}'", status).into()); + return Err(Error::UnexpectedHttpStatus(status)); } let config_blob = r.json::().await?; diff --git a/src/v2/manifest/mod.rs b/src/v2/manifest/mod.rs index e250a927..f5bc6abe 100644 --- a/src/v2/manifest/mod.rs +++ b/src/v2/manifest/mod.rs @@ -49,17 +49,12 @@ impl Client { match status { StatusCode::OK => {} - _ => return Err(format!("GET {}: wrong HTTP status '{}'", res.url(), status).into()), + _ => return Err(Error::UnexpectedHttpStatus(status)), } let headers = res.headers(); let content_digest = match headers.get("docker-content-digest") { - Some(content_digest_value) => Some( - content_digest_value - .to_str() - .map_err(|e| Error::from(format!("{}", e)))? - .to_string(), - ), + Some(content_digest_value) => Some(content_digest_value.to_str()?.to_string()), None => { debug!("cannot find manifestref in headers"); None @@ -95,10 +90,7 @@ impl Client { res.json::().await.map(Manifest::ML)?, content_digest, )), - unsupported => Err(Error::from(format!( - "unsupported mediatype '{:?}'", - unsupported - ))), + unsupported => Err(Error::UnsupportedMediaType(unsupported)), } } @@ -109,8 +101,7 @@ impl Client { name, reference ); - reqwest::Url::parse(&ep) - .map_err(|e| format!("failed to parse url from string '{}': {}", ep, e).into()) + reqwest::Url::parse(&ep).map_err(|e| Error::from(e)) } /// Fetch content digest for a particular tag. @@ -130,17 +121,12 @@ impl Client { match status { StatusCode::OK => {} - _ => return Err(format!("HEAD {}: wrong HTTP status '{}'", res.url(), status).into()), + _ => return Err(Error::UnexpectedHttpStatus(status)), } let headers = res.headers(); let content_digest = match headers.get("docker-content-digest") { - Some(content_digest_value) => Some( - content_digest_value - .to_str() - .map_err(|e| Error::from(format!("{}", e)))? - .to_string(), - ), + Some(content_digest_value) => Some(content_digest_value.to_str()?.to_string()), None => { debug!("cannot find manifestref in headers"); None @@ -160,35 +146,19 @@ impl Client { mediatypes: Option<&[&str]>, ) -> Result> { let url = self.build_url(name, reference)?; - let accept_types = match { - match mediatypes { - None => { - if let Ok(m) = mediatypes::MediaTypes::ManifestV2S2.to_mime() { - Ok(vec![m]) - } else { - Err(Error::from("to_mime failed")) - } - } - Some(ref v) => to_mimes(v), - } - } { - Ok(x) => x, - Err(e) => { - return Err(Error::from(format!("failed to match mediatypes: {}", e))); + let accept_types = match mediatypes { + None => { + let m = mediatypes::MediaTypes::ManifestV2S2.to_mime(); + vec![m] } + Some(ref v) => to_mimes(v), }; let mut accept_headers = header::HeaderMap::with_capacity(accept_types.len()); for accept_type in accept_types { - match header::HeaderValue::from_str(&accept_type.to_string()) { - Ok(header_value) => accept_headers.insert(header::ACCEPT, header_value), - Err(e) => { - return Err(Error::from(format!( - "failed to parse mime '{}' as accept_header: {}", - accept_type, e - ))); - } - }; + let header_value = header::HeaderValue::from_str(&accept_type.to_string()) + .expect("mime type is always valid header value"); + accept_headers.insert(header::ACCEPT, header_value); } trace!("HEAD {:?}", url); @@ -216,30 +186,24 @@ impl Client { | StatusCode::FOUND | StatusCode::OK => Some(media_type), StatusCode::NOT_FOUND => None, - _ => bail!("has_manifest: wrong HTTP status '{}'", &status), + _ => return Err(Error::UnexpectedHttpStatus(status)), }; Ok(res) } } -fn to_mimes(v: &[&str]) -> Result> { +fn to_mimes(v: &[&str]) -> Vec { let res = v .iter() .filter_map(|x| { let mtype = mediatypes::MediaTypes::from_str(x); match mtype { - Ok(m) => Some(match m.to_mime() { - Ok(mime) => mime, - Err(e) => { - error!("to_mime failed: {}", e); - return None; - } - }), + Ok(m) => Some(m.to_mime()), _ => None, } }) .collect(); - Ok(res) + res } // Evaluate the `MediaTypes` from the the request header. @@ -257,9 +221,7 @@ fn evaluate_media_type( (Some(header_value), false) => { mediatypes::MediaTypes::from_str(header_value).map_err(Into::into) } - (None, false) => Err(Error::from( - "no header_content_type given and no workaround to apply".to_string(), - )), + (None, false) => Err(Error::MediaTypeSniff), (Some(header_value), true) => { // TODO: remove this workaround once Satellite returns a proper content-type here match header_value { @@ -333,6 +295,18 @@ pub enum Manifest { ML(manifest_schema2::ManifestList), } +#[derive(Debug, thiserror::Error)] +pub enum ManifestError { + #[error("no architecture in manifest")] + NoArchitecture, + #[error("architecture mismatch")] + ArchitectureMismatch, + #[error("manifest {0} does not support the 'layer_digests' method")] + LayerDigestsUnsupported(String), + #[error("manifest {0} does not support the 'architecture' method")] + ArchitectureNotSupported(String), +} + impl Manifest { /// List digests of all layers referenced by this manifest, if available. /// @@ -344,23 +318,23 @@ impl Manifest { (Manifest::S1Signed(m), Ok(ref self_architectures), Some(ref a)) => { let self_a = self_architectures .first() - .ok_or("no architecture in manifest")?; - ensure!(self_a == a, "architecture mismatch"); + .ok_or(ManifestError::NoArchitecture)?; + if self_a != a { + return Err(ManifestError::ArchitectureMismatch.into()); + } Ok(m.get_layers()) } (Manifest::S2(m), Ok(ref self_architectures), Some(ref a)) => { let self_a = self_architectures .first() - .ok_or("no architecture in manifest")?; - ensure!(self_a == a, "architecture mismatch"); + .ok_or(ManifestError::NoArchitecture)?; + if self_a != a { + return Err(ManifestError::ArchitectureMismatch.into()); + } Ok(m.get_layers()) } // Manifest::ML(_) => TODO(steveeJ), - _ => Err(format!( - "Manifest {:?} doesn't support the 'layer_digests' method", - self - ) - .into()), + _ => Err(ManifestError::LayerDigestsUnsupported(format!("{:?}", self)).into()), } } @@ -370,11 +344,7 @@ impl Manifest { Manifest::S1Signed(m) => Ok([m.architecture.clone()].to_vec()), Manifest::S2(m) => Ok([m.architecture()].to_vec()), // Manifest::ML(_) => TODO(steveeJ), - _ => Err(format!( - "Manifest {:?} doesn't support the 'architecture' method", - self - ) - .into()), + _ => Err(ManifestError::ArchitectureNotSupported(format!("{:?}", self)).into()), } } } diff --git a/src/v2/mod.rs b/src/v2/mod.rs index 0fdf6553..4168c154 100644 --- a/src/v2/mod.rs +++ b/src/v2/mod.rs @@ -37,6 +37,7 @@ pub use self::config::Config; mod catalog; mod auth; +pub use auth::WwwHeaderParseError; pub mod manifest; @@ -46,6 +47,7 @@ mod blobs; mod content_digest; pub(crate) use self::content_digest::ContentDigest; +pub use self::content_digest::ContentDigestError; /// A Client to make outgoing API requests to a registry. #[derive(Clone, Debug)] @@ -66,7 +68,7 @@ impl Client { /// Ensure remote registry supports v2 API. pub async fn ensure_v2_registry(self) -> Result { if !self.is_v2_supported().await? { - bail!("remote server does not support docker-registry v2 API") + return Err(Error::V2NotSupported) } else { Ok(self) } @@ -80,7 +82,6 @@ impl Client { // GET request to bare v2 endpoint. let v2_endpoint = format!("{}/v2/", self.base_url); let request = reqwest::Url::parse(&v2_endpoint) - .chain_err(|| format!("failed to parse url string '{}'", &v2_endpoint)) .map(|url| { trace!("GET {:?}", url); self.build_reqwest(Method::GET, url) diff --git a/src/v2/tags.rs b/src/v2/tags.rs index 1d4f9306..021078dc 100644 --- a/src/v2/tags.rs +++ b/src/v2/tags.rs @@ -1,4 +1,4 @@ -use crate::errors::{Error, Result}; +use crate::errors::Result; use crate::v2::*; use async_stream::try_stream; use reqwest::{self, header, Url}; @@ -54,15 +54,14 @@ impl Client { (Some(p), Some(l)) => format!("{}?n={}&next_page={}", base_url, p, l), _ => base_url.to_string(), }; - let url = Url::parse(&url_paginated).map_err(|e| Error::from(format!("{}", e)))?; + let url = Url::parse(&url_paginated)?; let resp = self .build_reqwest(Method::GET, url.clone()) .header(header::ACCEPT, "application/json") .send() .await? - .error_for_status() - .map_err(|e| Error::from(format!("{}", e)))?; + .error_for_status()?; // ensure the CONTENT_TYPE header is application/json let ct_hdr = resp.headers().get(header::CONTENT_TYPE).cloned(); @@ -71,10 +70,7 @@ impl Client { let ok = match ct_hdr { None => false, - Some(ref ct) => ct - .to_str() - .map_err(|e| Error::from(format!("{}", e)))? - .starts_with("application/json"), + Some(ref ct) => ct.to_str()?.starts_with("application/json"), }; if !ok { // TODO:(steveeJ): Make this an error once Satellite diff --git a/tests/mock/base_client.rs b/tests/mock/base_client.rs index 11c0b621..82b32690 100644 --- a/tests/mock/base_client.rs +++ b/tests/mock/base_client.rs @@ -30,7 +30,7 @@ fn test_base_no_insecure() { // This relies on the fact that mockito is HTTP-only and // trying to speak TLS to it results in garbage/errors. - runtime.block_on(futcheck).is_err(); + runtime.block_on(futcheck).unwrap_err(); mockito::reset(); } diff --git a/tests/mod.rs b/tests/mod.rs index d0105376..e2dbafeb 100644 --- a/tests/mod.rs +++ b/tests/mod.rs @@ -1,9 +1,5 @@ #[cfg(feature = "test-net")] mod net; -#[cfg(feature = "test-net")] -#[macro_use] -extern crate error_chain; - #[cfg(feature = "test-mock")] mod mock; diff --git a/tests/net/quay_io/mod.rs b/tests/net/quay_io/mod.rs index 2e703b78..bcfceb42 100644 --- a/tests/net/quay_io/mod.rs +++ b/tests/net/quay_io/mod.rs @@ -286,9 +286,9 @@ fn test_quayio_auth_layer_blob() { .and_then(|manifest| { let layers: Vec = manifest.layers_digests(None)?; let num_layers = layers.len(); - ensure!(num_layers == 1, "layers length: {}", num_layers); + assert!(num_layers == 1, "layers length: {}", num_layers); let digest = layers[0].clone(); - ensure!(digest == layer0_sha, "layer0 digest: {}", digest); + assert!(digest == layer0_sha, "layer0 digest: {}", digest); Ok(digest) }) .unwrap();