diff --git a/crates/server/src/images/dispatch.rs b/crates/server/src/images/dispatch.rs index 579a496a..d97aa34e 100644 --- a/crates/server/src/images/dispatch.rs +++ b/crates/server/src/images/dispatch.rs @@ -400,14 +400,15 @@ impl Registry { } /// Whether the caller may `action` the target's image: the repository - /// permission and its content selectors, then a registry token's scope - /// (checked against the name the client used). + /// permission and its content selectors ([`Self::rbac_decision`]), then + /// a registry token's scope (checked against the name the client used). pub(crate) async fn decide(&self, caller: &Caller, target: &Target, action: Level) -> Decision { - if !self - .rbac_allows(&caller.principal, &target.repo, &target.image, action) + match self + .rbac_decision(&caller.principal, &target.repo, &target.image, action) .await { - return Decision::Deny; + Decision::Allow => {} + refused => return refused, } match &caller.claims { Some(claims) if !claims_allow(claims, &target.name, action) => Decision::NeedsScope, @@ -415,25 +416,48 @@ impl Registry { } } - /// The RBAC part of [`Self::decide`]. - pub(crate) async fn rbac_allows( + /// The RBAC part of [`Self::decide`]: the repository permission, then + /// its content selectors. An image a selector hides from a caller who + /// may read the repository is [`Decision::Hidden`] to a read, and + /// [`Decision::Deny`] to a push or a delete. + async fn rbac_decision( &self, principal: &Principal, repo: &Repository, image: &str, action: Level, - ) -> bool { + ) -> Decision { let authz = &self.inner.authz; let allowed = match action { Level::Read => authz.can_read(principal, repo).await, Level::Write => authz.can_write(principal, repo).await, Level::Delete => authz.can_delete(principal, repo).await, }; - allowed - && authz - .package_filter_for(principal, repo, action) - .await - .allows(image) + if !allowed { + Decision::Deny + } else if authz + .package_filter_for(principal, repo, action) + .await + .allows(image) + { + Decision::Allow + } else if action == Level::Read { + Decision::Hidden + } else { + Decision::Deny + } + } + + /// Whether RBAC allows the caller `action` on `repo`'s `image` + /// ([`Self::rbac_decision`]). + pub(crate) async fn rbac_allows( + &self, + principal: &Principal, + repo: &Repository, + image: &str, + action: Level, + ) -> bool { + self.rbac_decision(principal, repo, image, action).await == Decision::Allow } /// Where a push or delete addressed to `target` lands (the repository @@ -453,7 +477,12 @@ impl Registry { let landing = match target.repo.kind() { RepoKind::Hosted => target.clone(), RepoKind::Proxy | RepoKind::Group => { - let decision = self.decide(caller, target, Level::Read).await; + // A push of an image a selector hides is refused, not + // answered as a name that does not exist. + let decision = match self.decide(caller, target, Level::Read).await { + Decision::Hidden => Decision::Deny, + decision => decision, + }; if decision != Decision::Allow { return Err(self.refusal( caller, @@ -523,7 +552,9 @@ impl Registry { /// The refusal of `permission` on `name`: a challenge (401) when /// credentials or a token for the scope could help, a denial (403) for - /// an authenticated caller the permission is missing for. + /// an authenticated caller the permission is missing for, and for an + /// image a selector hides the answer to a name that does not exist + /// (404 `NAME_UNKNOWN`). pub(crate) fn refusal( &self, caller: &Caller, @@ -535,6 +566,9 @@ impl Registry { let authenticated = mutation_credential(&caller.principal); let error = caller.claims.as_ref().map(|_| "insufficient_scope"); match decision { + Decision::Hidden => Failure::new(ImageError::NameUnknown { + name: name.to_string(), + }), Decision::NeedsScope => challenge(site, name, permission, error), Decision::Deny if !authenticated => challenge(site, name, permission, error), _ => Failure::new(ImageError::Denied { @@ -1067,6 +1101,11 @@ pub(crate) enum Decision { /// RBAC allows it, but the registry token presented does not cover it: /// the client should fetch a token for this scope. NeedsScope, + /// The caller may read the repository, but a content selector of its + /// grant hides the image: a read is answered as for a name that does + /// not exist (404), as the other formats answer a package a selector + /// hides. + Hidden, Deny, } diff --git a/crates/server/tests/registry.rs b/crates/server/tests/registry.rs index 59e4cac7..7b436d3d 100644 --- a/crates/server/tests/registry.rs +++ b/crates/server/tests/registry.rs @@ -3515,13 +3515,87 @@ async fn group_browse_lists_what_its_pulls_serve() { assert_eq!(listed(reader), [("other/app".to_string(), 1)]); } +/// An image a selector of the caller's grant hides is not found (#703): a +/// read of it answers 404 `NAME_UNKNOWN` on every endpoint, as a name that +/// does not exist, where it answered 403, while a push outside the +/// selector is still refused (403), and a caller who may not read the +/// repository at all is refused as before. +#[tokio::test] +async fn an_image_a_selector_hides_is_not_found() { + let srv = common::start().await; + let repo = repo_name("sel"); + create_repo( + &srv, + json!({"name": repo, "kind": "hosted", "format": "docker"}), + ) + .await; + let admin = admin_oci(&srv).await; + let digest = admin + .push_image(&format!("{repo}/secret/app"), "1", "amd64", b"secret layer") + .await; + admin + .push_image(&format!("{repo}/public/app"), "1", "amd64", b"public layer") + .await; + let selector = srv.create_selector(&["public/*"]).await; + let caller = srv.caller().await; + srv.grant_role(caller.id, &[(&repo, "write", Some(selector))]) + .await; + let oci = Oci::new(&srv, Some((caller.username.clone(), caller.token.clone()))); + + let hidden = format!("{repo}/secret/app"); + for path in [ + "manifests/1".to_string(), + format!("manifests/{digest}"), + format!("blobs/{}", sha(b"secret layer")), + "tags/list".to_string(), + format!("referrers/{digest}"), + ] { + let resp = oci.get(&hidden, &path).await; + assert_eq!(resp.status(), 404, "{path}"); + let body: Value = resp.json().await.unwrap(); + assert_eq!(body["errors"][0]["code"], "NAME_UNKNOWN", "{path}: {body}"); + assert_eq!(oci.head(&hidden, &path).await.status(), 404, "HEAD {path}"); + } + assert_eq!( + oci.get(&format!("{repo}/public/app"), "manifests/1") + .await + .status(), + 200 + ); + // No push outside the selector, and no delete of what it hides. + let resp = oci + .request( + Method::POST, + &format!("/v2/{repo}/secret/new/blobs/uploads/"), + "pull,push", + &format!("{repo}/secret/new"), + ) + .await + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 403); + let resp = oci.delete(&hidden, &format!("manifests/{digest}")).await; + assert!( + resp.status() == 401 || resp.status() == 403, + "{}", + resp.status() + ); + + // Who may not read the repository is refused, not told it is absent. + let stranger = srv.caller().await; + let oci = Oci::new(&srv, Some((stranger.username, stranger.token))); + assert_eq!(oci.get(&hidden, "manifests/1").await.status(), 403); +} + /// A group shows a caller what it could pull from its members directly, no /// more (#648): with a member it may not read, a selector on the group's /// grant or one on a member's, a hidden image's manifests, blobs, tags and /// referrers answer as an image of a name like it that does not exist -/// (404, or 403 for a name outside a selector of the group's own grant, -/// which grants no `pull` on it), and the browse API's image list (its -/// count too), image page and chart page leave it out. +/// (404; a selector of the group's own grant answers so too, #703, and a +/// readable name's referrers of an unknown subject are an empty index), and +/// the browse API's image list (its count too), image page and chart page +/// leave it out. #[tokio::test] async fn groups_show_a_caller_only_the_images_it_may_read() { let srv = common::start().await; @@ -3613,7 +3687,7 @@ async fn groups_show_a_caller_only_the_images_it_may_read() { let (expected, absent_body) = (resp.status(), resp.text().await.unwrap()); // Referrers of an unknown subject are an empty index. assert!( - matches!(expected.as_u16(), 403 | 404) || path.starts_with("referrers/"), + expected == 404 || path.starts_with("referrers/"), "{label}: {absent} {path}: {expected}" ); assert_eq!(status, expected, "{label}: {name} {path}: {body}"); diff --git a/docs/changes/2.2.0/703.fixed.md b/docs/changes/2.2.0/703.fixed.md new file mode 100644 index 00000000..3c4f99d4 --- /dev/null +++ b/docs/changes/2.2.0/703.fixed.md @@ -0,0 +1,7 @@ +A docker pull of an image a content selector of the caller's grant hides +was refused with 403 (`DENIED`, 401 for an anonymous caller) when the +selector was on the repository pulled from, a group's included; through a +group's member it answered 404. Such an image is now not found on every +endpoint (manifests, blobs, tags and referrers answer 404 `NAME_UNKNOWN`), +as a package a selector hides is in the other formats. A push or a delete +outside the selector is still refused with 403. diff --git a/docs/docker.md b/docs/docker.md index 9ab9014b..1ddc28b7 100644 --- a/docs/docker.md +++ b/docs/docker.md @@ -83,7 +83,11 @@ docker push forge.example.com/docker-hosted/team/app:1.3 A registry token grants only what the caller may do when it is issued: `pull` needs `read` on the repository, `push` needs `write`, and `delete` -needs `delete`; content selectors match image names (`team/*`). One token +needs `delete`; content selectors match image names (`team/*`). An image a +selector hides from a caller who may read the repository is not found: a +pull, its tags or its referrers answer 404 `NAME_UNKNOWN`, as for a name +that does not exist (also through a group), while a push or a delete of it +is refused with 403. One token request may name at most 32 repositories (clients ask for one to three); a request naming more is refused with `400 UNSUPPORTED`. Every request is authorized again against the current permissions, and the user