Skip to content

Commit

Permalink
oidc: reject storage scopes without path
Browse files Browse the repository at this point in the history
Fixes commit 094c171

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
(cherry picked from commit 83d1e77)
Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
  • Loading branch information
kofemann authored and mksahakyan committed Oct 27, 2023
1 parent b862c12 commit 1288355
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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<Activity> 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));
Expand All @@ -120,6 +123,10 @@ public String getLabel() {
public EnumSet<Activity> allowedActivities() {
return allowedActivities;
}

public boolean isPathRequired() {
return requirePath;
}
}

private static final Map<String, Operation> OPERATIONS_BY_LABEL;
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down

0 comments on commit 1288355

Please sign in to comment.