From 83d1e77eed2de6b92055088c1aec1544da05decd Mon Sep 17 00:00:00 2001 From: Tigran Mkrtchyan Date: Tue, 24 Oct 2023 10:10:01 +0200 Subject: [PATCH] oidc: reject storage scopes without path Fixes commit 094c1714c Motivation: According WLCG Common JWT Profiles document: For all storage.* scopes, $PATH MUST be specified (but may be /to authorize the entire resource associated with the issuer); if not specified for these scopes, the token MUST be rejected. However, dCache doesn't follow this rule, thus, such scopes are accepted and path is set to '/'. Modification: Update WlcgProfileScope.Operation enum to indicate whatever path is required or optional for a given scope. Require path for all 'storage.*' scopes. Result: spec compatibility and potential security fix. Ticket: #10523 Acked-by: Paul Millar Acked-by: Lea Morschel Acked-by: Dmitry Litvintsev Target: master, 9.2, 9.1, 9.0, 8.2 Require-book: no Require-notes: yes --- .../oidc/profiles/WlcgProfileScope.java | 26 ++++++++++++------- .../oidc/profiles/WlcgProfileScopeTest.java | 16 ++++++++++++ 2 files changed, 33 insertions(+), 9 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 60e50eb3835..82b3e3e8ec5 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 @@ -56,7 +56,7 @@ public enum Operation { * Read data. Only applies to “online” resources such as disk (as opposed to “nearline” such * as tape where the {@literal stage} authorization should be used in addition). */ - READ("storage.read", LIST, READ_METADATA, DOWNLOAD), + READ("storage.read", true, LIST, READ_METADATA, DOWNLOAD), /** * Upload data. This includes renaming files if the destination file does not already exist. @@ -67,47 +67,50 @@ public enum Operation { * use case for a separate {@literal storage.create} scope is to enable stage-out of data * from jobs on a worker node. */ - CREATE("storage.create", READ_METADATA, UPLOAD, MANAGE, UPDATE_METADATA), + CREATE("storage.create", true, READ_METADATA, UPLOAD, MANAGE, UPDATE_METADATA), /** * Change data. This includes renaming files, creating new files, and writing data. This * permission includes overwriting or replacing stored data in addition to deleting or * truncating data. This is a strict superset of {@literal storage.create}. */ - MODIFY("storage.modify", LIST, READ_METADATA, UPLOAD, MANAGE, DELETE, UPDATE_METADATA), + MODIFY("storage.modify", true, LIST, READ_METADATA, UPLOAD, MANAGE, DELETE, UPDATE_METADATA), /** * 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", true, LIST, READ_METADATA, DOWNLOAD), // FIXME need to allow staging. /** * "Read" or query information about job status and attributes. */ - COMPUTE_READ("compute.read"), + COMPUTE_READ("compute.read", false), /** * Modify or change the attributes of an existing job. */ - COMPUTE_MODIFY("compute.modify"), + COMPUTE_MODIFY("compute.modify", false), /** * Create or submit a new job at the computing resource. */ - COMPUTE_CREATE("compute.create"), + COMPUTE_CREATE("compute.create", false), /** * Delete a job from the computing resource, potentially terminating * a running job. */ - COMPUTE_CANCEL("compute.cancel"); + COMPUTE_CANCEL("compute.cancel", false); private final String label; private final EnumSet allowedActivities; - private Operation(String label, Activity... allowedActivities) { + private final boolean requirePath; + + private Operation(String label, boolean requirePath, Activity... allowedActivities) { this.label = label; + this.requirePath = requirePath; this.allowedActivities = allowedActivities.length == 0 ? EnumSet.noneOf(Activity.class) : EnumSet.copyOf(asList(allowedActivities)); @@ -120,6 +123,10 @@ public String getLabel() { public EnumSet allowedActivities() { return allowedActivities; } + + public boolean isPathRequired() { + return requirePath; + } } private static final Map OPERATIONS_BY_LABEL; @@ -146,6 +153,7 @@ public WlcgProfileScope(String scope) { checkScopeValid(operation != null, "Unknown operation %s", operationLabel); if (colon == -1) { + checkScopeValid(!operation.isPathRequired(), "Path must be specified"); path = "/"; } else { String scopePath = scope.substring(colon + 1); 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 7dd5331931e..c565b437d0b 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 @@ -58,6 +58,22 @@ public void shouldNotIdentifyStorageWriteScope() { assertFalse(WlcgProfileScope.isWlcgProfileScope("storage.write:/")); } + + @Test(expected = InvalidScopeException.class) + public void shouldRejectStorageCreateScopeWithoutPath() { + new WlcgProfileScope("storage.create"); + } + + @Test(expected = InvalidScopeException.class) + public void shouldRejectStorageReadScopeWithoutPath() { + new WlcgProfileScope("storage.read"); + } + + @Test(expected = InvalidScopeException.class) + public void shouldRejectStorageModifyScopeWithoutPath() { + new WlcgProfileScope("storage.modify"); + } + @Test public void shouldIdentifyComputeReadScope() { assertTrue(WlcgProfileScope.isWlcgProfileScope("compute.read"));