From 094c1714c196c07665b9835e6759d73208f021a9 Mon Sep 17 00:00:00 2001 From: Paul Millar Date: Wed, 4 Oct 2023 22:26:19 +0200 Subject: [PATCH] gplazma: oidc update explicit AuthZ parsing Motivation: The WLCG JWT Profile describes how JWTs may include explicit authorisation statements in the 'scope' claim (e.g., `storage.read:/foo`). When processing a token, the oidc gPlazma plugin decides whether the token carries explicit AuthZ statements. If it does, then the resulting login will suspend namespace-based authorisation and fully honour the authorisation from the token. There are a few issues with how dCache currently parses these explicit AuthZ statements. 1. The profile describes how the resource identifier is optional (e.g., `storage.read` is valid), but dCache scope parsing currently rejects explicit AuthZ if the resource identifier is missing. 2. The profile describes several explicit AuthZ statements (e.g., `compute.read`). Currently, dCache completely ignores these statements. However, this is not complete correct because such compute explicit AuthZ statements indicate the token (in general) is carrying explicit AuthZ. Therefore, the presence of compute explicit AuthZ statements and a lack of any storage explicit AuthZ statements should result in dCache rejecting requests by that token. Modification: Update scope parsing components to relax the requirement on having a resource identifier. An explicit AuthZ statement without a resource identifier is equivalent to adding `:/` to the statement (e.g., `storage.read` interpreted as `storage.read:/`). Add extra valid values for the "compute.\*" explicit AuthZ statements. This are ignored by the rest of dCache, but their presence will result in dCache considering the token as one with explicit AuthZ. Result: dCache will now accept non-targeted explicit AuthZ statements in the scope claim (e.g., `storage.read`). dCache will consider tokens containing compute explicit AuthZ statements but without any storage explicit AuthZ statements as tokens with explicit authorisation; the lack of any storage explicit authorisation statements will result in all requests with that token being denied. Target: master Requires-notes: yes Requires-book: no Request: 9.2 Request: 9.1 Request: 9.0 Request: 8.2 Patch: https://rb.dcache.org/r/13996/ Acked-by: Dmitry Litvintsev --- .../oidc/profiles/WlcgProfileScope.java | 52 +++++++-- .../oidc/profiles/WlcgProfileScopeTest.java | 100 +++++++++++++++++- .../oidc/profiles/WlcgProfileTest.java | 44 ++++++++ 3 files changed, 182 insertions(+), 14 deletions(-) diff --git a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScope.java b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScope.java index 9ed38bc9f24..60e50eb3835 100644 --- a/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScope.java +++ b/modules/gplazma2-oidc/src/main/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScope.java @@ -1,6 +1,6 @@ /* dCache - http://www.dcache.org/ * - * Copyright (C) 2020-2022 Deutsches Elektronen-Synchrotron + * Copyright (C) 2020-2023 Deutsches Elektronen-Synchrotron * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as @@ -80,14 +80,37 @@ public enum Operation { * Read the data, potentially causing data to be staged from a nearline resource to an * online resource. This is a superset of {@literal storage.read}. */ - STAGE("storage.stage", LIST, READ_METADATA, DOWNLOAD); // FIXME need to allow staging. + STAGE("storage.stage", LIST, READ_METADATA, DOWNLOAD), // FIXME need to allow staging. + + /** + * "Read" or query information about job status and attributes. + */ + COMPUTE_READ("compute.read"), + + /** + * Modify or change the attributes of an existing job. + */ + COMPUTE_MODIFY("compute.modify"), + + /** + * Create or submit a new job at the computing resource. + */ + COMPUTE_CREATE("compute.create"), + + /** + * Delete a job from the computing resource, potentially terminating + * a running job. + */ + COMPUTE_CANCEL("compute.cancel"); private final String label; private final EnumSet allowedActivities; private Operation(String label, Activity... allowedActivities) { this.label = label; - this.allowedActivities = EnumSet.copyOf(asList(allowedActivities)); + this.allowedActivities = allowedActivities.length == 0 + ? EnumSet.noneOf(Activity.class) + : EnumSet.copyOf(asList(allowedActivities)); } public String getLabel() { @@ -116,16 +139,20 @@ public EnumSet allowedActivities() { public WlcgProfileScope(String scope) { int colon = scope.indexOf(':'); - checkScopeValid(colon != -1, "Missing ':' in scope"); - String operationLabel = scope.substring(0, colon); + String operationLabel = colon == -1 ? scope : scope.substring(0, colon); + operation = OPERATIONS_BY_LABEL.get(operationLabel); checkScopeValid(operation != null, "Unknown operation %s", operationLabel); - String scopePath = scope.substring(colon + 1); - checkScopeValid(scopePath.startsWith("/"), "Path does not start with /"); + if (colon == -1) { + path = "/"; + } else { + String scopePath = scope.substring(colon + 1); + checkScopeValid(scopePath.startsWith("/"), "Path does not start with /"); - path = URI.create(scopePath).getPath(); + path = URI.create(scopePath).getPath(); + } LOGGER.debug("WlcgProfileScope created from scope \"{}\": op={} path={}", scope, operation, path); @@ -133,12 +160,17 @@ public WlcgProfileScope(String scope) { public static boolean isWlcgProfileScope(String scope) { int colon = scope.indexOf(':'); - return colon >= MINIMUM_LABEL_SIZE - && OPERATIONS_BY_LABEL.keySet().contains(scope.substring(0, colon)); + String authz = colon == -1 ? scope : scope.substring(0, colon); + return authz.length() >= MINIMUM_LABEL_SIZE + && OPERATIONS_BY_LABEL.keySet().contains(authz); } @Override public Optional authorisation(FsPath prefix) { + if (operation.allowedActivities.isEmpty()) { + return Optional.empty(); + } + FsPath absPath = prefix.resolve(path.substring(1)); LOGGER.debug("WlcgProfileScope authorising {} with prefix \"{}\" to path {}", prefix, operation.allowedActivities, absPath); diff --git a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScopeTest.java b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScopeTest.java index 252249ee9f2..7dd5331931e 100644 --- a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScopeTest.java +++ b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileScopeTest.java @@ -1,6 +1,6 @@ /* dCache - http://www.dcache.org/ * - * Copyright (C) 2020-2022 Deutsches Elektronen-Synchrotron + * Copyright (C) 2020-2023 Deutsches Elektronen-Synchrotron * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU Affero General Public License as @@ -17,8 +17,6 @@ */ package org.dcache.gplazma.oidc.profiles; -import org.dcache.gplazma.oidc.profiles.WlcgProfileScope; - import static org.dcache.auth.attributes.Activity.DOWNLOAD; import static org.dcache.auth.attributes.Activity.LIST; import static org.dcache.auth.attributes.Activity.READ_METADATA; @@ -35,6 +33,11 @@ public class WlcgProfileScopeTest { + @Test(expected=InvalidScopeException.class) + public void shouldRejectUnknownScope() { + new WlcgProfileScope("unknown-value"); + } + @Test public void shouldIdentifyStorageReadScope() { assertTrue(WlcgProfileScope.isWlcgProfileScope("storage.read:/")); @@ -56,7 +59,41 @@ public void shouldNotIdentifyStorageWriteScope() { } @Test - public void shouldParseReadScope() { + public void shouldIdentifyComputeReadScope() { + assertTrue(WlcgProfileScope.isWlcgProfileScope("compute.read")); + } + + @Test + public void shouldIdentifyComputeModifyScope() { + assertTrue(WlcgProfileScope.isWlcgProfileScope("compute.modify")); + } + + @Test + public void shouldIdentifyComputeCreateScope() { + assertTrue(WlcgProfileScope.isWlcgProfileScope("compute.create")); + } + + @Test + public void shouldIdentifyComputeCancelScope() { + assertTrue(WlcgProfileScope.isWlcgProfileScope("compute.cancel")); + } + + @Test + public void shouldParseReadScopeWithoutResourcePath() { + WlcgProfileScope scope = new WlcgProfileScope("storage.read"); + + Optional maybeAuth = scope.authorisation(FsPath.create("/VOs/wlcg")); + + assertTrue(maybeAuth.isPresent()); + + Authorisation auth = maybeAuth.get(); + + assertThat(auth.getPath(), equalTo(FsPath.create("/VOs/wlcg"))); + assertThat(auth.getActivity(), containsInAnyOrder(LIST, READ_METADATA, DOWNLOAD)); + } + + @Test + public void shouldParseReadScopeWithRootResourcePath() { WlcgProfileScope scope = new WlcgProfileScope("storage.read:/"); Optional maybeAuth = scope.authorisation(FsPath.create("/VOs/wlcg")); @@ -68,4 +105,59 @@ public void shouldParseReadScope() { assertThat(auth.getPath(), equalTo(FsPath.create("/VOs/wlcg"))); assertThat(auth.getActivity(), containsInAnyOrder(LIST, READ_METADATA, DOWNLOAD)); } + + @Test + public void shouldParseReadScopeWithNonRootResourcePath() { + WlcgProfileScope scope = new WlcgProfileScope("storage.read:/foo"); + + Optional maybeAuth = scope.authorisation(FsPath.create("/VOs/wlcg")); + + assertTrue(maybeAuth.isPresent()); + + Authorisation auth = maybeAuth.get(); + + assertThat(auth.getPath(), equalTo(FsPath.create("/VOs/wlcg/foo"))); + assertThat(auth.getActivity(), containsInAnyOrder(LIST, READ_METADATA, DOWNLOAD)); + } + + @Test(expected=InvalidScopeException.class) + public void shouldRejectReadScopeWithRelativeResourcePath() { + new WlcgProfileScope("storage.read:foo"); + } + + @Test + public void shouldParseComputeReadScope() { + WlcgProfileScope scope = new WlcgProfileScope("compute.read"); + + Optional maybeAuth = scope.authorisation(FsPath.create("/VOs/wlcg")); + + assertTrue(maybeAuth.isEmpty()); + } + + @Test + public void shouldParseComputeModifyScope() { + WlcgProfileScope scope = new WlcgProfileScope("compute.modify"); + + Optional maybeAuth = scope.authorisation(FsPath.create("/VOs/wlcg")); + + assertTrue(maybeAuth.isEmpty()); + } + + @Test + public void shouldParseComputeCreateScope() { + WlcgProfileScope scope = new WlcgProfileScope("compute.create"); + + Optional maybeAuth = scope.authorisation(FsPath.create("/VOs/wlcg")); + + assertTrue(maybeAuth.isEmpty()); + } + + @Test + public void shouldParseComputeCancelScope() { + WlcgProfileScope scope = new WlcgProfileScope("compute.cancel"); + + Optional maybeAuth = scope.authorisation(FsPath.create("/VOs/wlcg")); + + assertTrue(maybeAuth.isEmpty()); + } } diff --git a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileTest.java b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileTest.java index c3782dc056c..f63cfe7ec5e 100644 --- a/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileTest.java +++ b/modules/gplazma2-oidc/src/test/java/org/dcache/gplazma/oidc/profiles/WlcgProfileTest.java @@ -483,6 +483,36 @@ public void shouldAcceptRootReadAndNonRootModifyWlcgScope() throws Exception { assertFalse(r.isRestricted(Activity.UPDATE_METADATA, FsPath.create("/prefix/write-target/my-file"))); } + @Test + public void shouldAcceptComputeReadScopeWithDenyAllAuthz() throws Exception { + given(aWlcgProfile().withPrefix("/prefix")); + + when(invoked().withIdP(anIp("MY-OP")) + .withStringClaim("wlcg.ver", "1.0") + .withStringClaim("scope", "openid compute.read")); + + assertThat(principals, hasItem(any(ExemptFromNamespaceChecks.class))); + assertThat(restriction, isPresent()); + Restriction r = restriction.get(); + assertTrue(r.isRestricted(Activity.UPLOAD, FsPath.create("/my-file"))); + assertTrue(r.isRestricted(Activity.MANAGE, FsPath.create("/my-file"))); + assertTrue(r.isRestricted(Activity.DELETE, FsPath.create("/my-file"))); + assertTrue(r.isRestricted(Activity.UPDATE_METADATA, FsPath.create("/my-file"))); + assertTrue(r.isRestricted(Activity.DOWNLOAD, FsPath.create("/my-file"))); + + assertTrue(r.isRestricted(Activity.UPLOAD, FsPath.create("/other/my-file"))); + assertTrue(r.isRestricted(Activity.MANAGE, FsPath.create("/other/my-file"))); + assertTrue(r.isRestricted(Activity.DELETE, FsPath.create("/other/my-file"))); + assertTrue(r.isRestricted(Activity.UPDATE_METADATA, FsPath.create("/other/my-file"))); + assertTrue(r.isRestricted(Activity.DOWNLOAD, FsPath.create("/other/my-file"))); + + assertTrue(r.isRestricted(Activity.UPLOAD, FsPath.create("/prefix/my-file"))); + assertTrue(r.isRestricted(Activity.MANAGE, FsPath.create("/prefix/my-file"))); + assertTrue(r.isRestricted(Activity.DELETE, FsPath.create("/prefix/my-file"))); + assertTrue(r.isRestricted(Activity.UPDATE_METADATA, FsPath.create("/prefix/my-file"))); + assertTrue(r.isRestricted(Activity.DOWNLOAD, FsPath.create("/prefix/my-file"))); + } + @Test public void shouldIncludeAuthzIdentity() throws Exception { given(aWlcgProfile().withPrefix("/prefix") @@ -497,6 +527,20 @@ public void shouldIncludeAuthzIdentity() throws Exception { assertThat(principals, not(hasItem(new GroupNamePrincipal("non-authz-group")))); } + @Test + public void shouldIncludeAuthzIdentityWhenAcceptingComputeReadScope() throws Exception { + given(aWlcgProfile().withPrefix("/prefix") + .withAuthzIdentity(aSetOfPrincipals().withGroupname("authz-group")) + .withNonAuthzIdentity(aSetOfPrincipals().withGroupname("non-authz-group"))); + + when(invoked().withIdP(anIp("MY-OP")) + .withStringClaim("wlcg.ver", "1.0") + .withStringClaim("scope", "openid compute.read")); + + assertThat(principals, hasItem(new GroupNamePrincipal("authz-group"))); + assertThat(principals, not(hasItem(new GroupNamePrincipal("non-authz-group")))); + } + private void given(WlcgProfileBuilder builder) { profile = builder.build();