Skip to content

Commit

Permalink
gplazma/pnfsmanager: update namespace so scitoken 'scope' takes priority
Browse files Browse the repository at this point in the history
Motivation:

The SciToken and WLCG-AuthZ specifications allow a token to contain
authorisation statements.  These are recorded in the `scope` claim.  In
dCache, these authorisation statements are translated into restrictions
which limit the user, preventing that user from doing anything the token
says the bearer is not entitled to do.

However, the namespace permissions are still enforced.  If the token
says the user IS allowed to do something and the namespace states that
this user IS NOT allowed to do that thing then the operation will fail:
the namespace "wins".

This is doesn't really make sense, as one of the main goals of such
tokens is for services (such as dCache) to delegate authorisation
decisions from the service to some central service.  In other words, if
there is a conflict the token should "win".

Modification:

Add a new principal, `ExemptFromNamespaceChecks`, that shows the user
should be exempt from the normal namespace-based authorisation policies.
Any restrictions accompanying a namespace request should still be
enforced.

Update the `Subjects` utility class to include the
`isExemptFromNamespaceChecks` static method.  This new method checks
whether the user is exempt from namespace authorisation checks.  It
returns `true` for the root user or if the user has the
`ExemptFromNamespaceChecks` principal.  The new method is meant as a
drop-in replacement for `isRoot`, making updating dCache easier.

Update the `ChimeraNameSpaceProvider` class to use the new `Subjects`
method instead of `isRoot`.  The `PnfsManagerV3` class is also updated,
but only in those places where `isRoot` is currently by-passing a
namespace check (the mask) --- those places that enforce restrictions
are deliberately left as `isRoot`.

The `scitoken` plugin is updated to set the `ExemptFromNamespaceChecks`
principal.  Note that the plugin is only successful if the token
contains a `scope` claim that authorises access.

Unit tests are updated to verify correct behaviour of
`isExemptFromNamespaceChecks`.

Result:

In the SciToken plugin, the 'scope' based authorisation is now honoured,
irrespective of the authorisation decisions in the namespace.  The
mapped user it used to determine the ownership (uid & gid) of new
content (directories, files, etc) but otherwise has no other purpose.

Target: master
Requires-notes: yes
Requres-book: likely
Patch: https://rb.dcache.org/r/13138/
Acked-by: Tigran Mkrtchyan
  • Loading branch information
paulmillar authored and mksahakyan committed Oct 12, 2021
1 parent 9115df4 commit 14f2019
Show file tree
Hide file tree
Showing 7 changed files with 143 additions and 25 deletions.
@@ -0,0 +1,46 @@
/*
* dCache - http://www.dcache.org/
*
* Copyright (C) 2021 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
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
package org.dcache.auth;

import java.io.Serializable;
import java.security.Principal;

/**
* The presence of this principal indicates that the user is exempt from the
* normal namespace permission rules. The
* {@link org.dcache.auth.attributes.Restriction} accompanying a namespace
* request is still enforced. Code that inserts this principal should (very
* likely) also add restrictions, otherwise the user will have root-like
* authority.
*/
@AuthenticationOutput
public class ExemptFromNamespaceChecks implements Principal, Serializable
{
@Override
public String getName()
{
return "full"; // all namespace checks are by-passed.
}

@Override
public String toString()
{
return "ExemptFromNamespaceChecks";
}
}
31 changes: 30 additions & 1 deletion modules/common/src/main/java/org/dcache/auth/Subjects.java
Expand Up @@ -22,6 +22,8 @@
import java.util.Set;
import java.util.stream.Collectors;

import org.dcache.util.PrincipalSetMaker;

import static com.google.common.base.Preconditions.checkArgument;

public class Subjects
Expand Down Expand Up @@ -67,6 +69,21 @@ public static boolean isRoot(Subject subject)
return hasUid(subject, 0);
}

/**
* Return true if the subject is root or has the special
* ExemptFromNamespaceChecks principal.
* @param subject The identity of the user.
* @return if the user is except from namespace checks.
* @see #isRoot(javax.security.auth.Subject)
*/
public static boolean isExemptFromNamespaceChecks(Subject subject)
{
return subject.getPrincipals().stream()
.anyMatch(p -> p instanceof UidPrincipal && ((UidPrincipal)p).getUid() == 0
||
p instanceof ExemptFromNamespaceChecks);
}

/**
* Returns true if and only if the subject is nobody, i.e., does
* not have a UID.
Expand Down Expand Up @@ -678,7 +695,7 @@ private static StringBuilder appendOptionallyInQuotes(StringBuilder sb, String a
// Returned Subject must NOT be readOnly.
public static Subject of(int uid, int gid, int[] gids)
{
Builder builder = of().uid(uid).gid(gid);
Builder builder = Subjects.of().uid(uid).gid(gid);
for (int g : gids) {
builder.gid(g);
}
Expand All @@ -690,6 +707,18 @@ public static Builder of()
return new Builder();
}

public static Subject ofPrincipals(Set<Principal> principals)
{
Subject subject = new Subject();
subject.getPrincipals().addAll(principals);
return subject;
}

public static Subject of(PrincipalSetMaker maker)
{
return ofPrincipals(maker.build());
}

public static class Builder
{
private final Subject _subject = new Subject();
Expand Down
Expand Up @@ -11,6 +11,7 @@

import org.dcache.auth.DesiredRole;
import org.dcache.auth.EmailAddressPrincipal;
import org.dcache.auth.ExemptFromNamespaceChecks;
import org.dcache.auth.FQANPrincipal;
import org.dcache.auth.GidPrincipal;
import org.dcache.auth.GroupNamePrincipal;
Expand Down Expand Up @@ -181,6 +182,12 @@ public PrincipalSetMaker withKerberos(String kerberos)
return this;
}

public PrincipalSetMaker withExemptFromNamespaceChecks()
{
_principals.add(new ExemptFromNamespaceChecks());
return this;
}

/**
* Provide a unmodifiable view of the set of principals.
*/
Expand Down
34 changes: 34 additions & 0 deletions modules/common/src/test/java/org/dcache/auth/SubjectsTest.java
Expand Up @@ -12,7 +12,10 @@
import java.util.NoSuchElementException;
import java.util.Set;

import org.dcache.util.PrincipalSetMaker;

import static java.util.Arrays.asList;
import static org.dcache.util.PrincipalSetMaker.aSetOfPrincipals;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasItem;
import static org.junit.Assert.*;
Expand Down Expand Up @@ -285,4 +288,35 @@ public void shouldPrincipalsFromArgsForOidc()

assertThat(principals, hasItem(new OidcSubjectPrincipal("sub-claim", "OP")));
}

@Test
public void normalUserShouldNotBeExceptFromNamespaceChecks()
{
var subject = Subjects.of(aSetOfPrincipals().withOidc("sub-claim", "OP"));

assertFalse(Subjects.isExemptFromNamespaceChecks(_subject1));
assertFalse(Subjects.isExemptFromNamespaceChecks(_subject2));
assertFalse(Subjects.isExemptFromNamespaceChecks(_subject3));
assertFalse(Subjects.isExemptFromNamespaceChecks(_subject4));
assertFalse(Subjects.isExemptFromNamespaceChecks(subject));
}

@Test
public void rootShouldBeExceptFromNamespaceChecks()
{
var root = Subjects.of(aSetOfPrincipals().withUid(0).withPrimaryGid(0));

assertTrue(Subjects.isExemptFromNamespaceChecks(root));
}

@Test
public void exemptUserShouldBeExceptFromNamespaceChecks()
{
var root = Subjects.of(aSetOfPrincipals()
.withUid(1000)
.withPrimaryGid(1000)
.withExemptFromNamespaceChecks());

assertTrue(Subjects.isExemptFromNamespaceChecks(root));
}
}
Expand Up @@ -228,7 +228,7 @@ public void setUploadSubDirectory(String path)
private ExtendedInode pathToInode(Subject subject, String path)
throws ChimeraFsException, CacheException
{
if (Subjects.isRoot(subject)) {
if (Subjects.isExemptFromNamespaceChecks(subject)) {
return new ExtendedInode(_fs, _fs.path2inode(path));
}

Expand Down Expand Up @@ -304,7 +304,7 @@ public FileAttributes createFile(Subject subject, String path,
}
ExtendedInode parent = pathToInode(subject, parentPath);

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes
= getFileAttributesForPermissionHandler(parent);
if (_permissionHandler.canCreateFile(subject, attributes) != ACCESS_ALLOWED) {
Expand Down Expand Up @@ -409,7 +409,7 @@ public PnfsId createSymLink(Subject subject, String path, String dest, FileAttri
}
ExtendedInode parent = pathToInode(subject, parentPath);

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes
= getFileAttributesForPermissionHandler(parent);
if (_permissionHandler.canCreateFile(subject, attributes) != ACCESS_ALLOWED) {
Expand Down Expand Up @@ -505,7 +505,7 @@ public FileAttributes deleteEntry(Subject subject, Set<FileType> allowed, PnfsId

checkAllowed(allowed, inode);

if (!Subjects.isRoot(subject) && !canDelete(subject, inode.getParent(), inode)) {
if (!Subjects.isExemptFromNamespaceChecks(subject) && !canDelete(subject, inode.getParent(), inode)) {
throw new PermissionDeniedCacheException("Access denied: " + pnfsId);
}

Expand Down Expand Up @@ -540,7 +540,7 @@ public FileAttributes deleteEntry(Subject subject, Set<FileType> allowed,
ExtendedInode inode = parent.inodeOf(name, STAT);
checkAllowed(allowed, inode);

if (!Subjects.isRoot(subject) && !canDelete(subject, parent, inode)) {
if (!Subjects.isExemptFromNamespaceChecks(subject) && !canDelete(subject, parent, inode)) {
throw new PermissionDeniedCacheException("Access denied: " + path);
}

Expand Down Expand Up @@ -579,7 +579,7 @@ public FileAttributes deleteEntry(Subject subject, Set<FileType> allowed,

checkAllowed(allowed, inode);

if (!Subjects.isRoot(subject) && !canDelete(subject, parent, inode)) {
if (!Subjects.isExemptFromNamespaceChecks(subject) && !canDelete(subject, parent, inode)) {
throw new PermissionDeniedCacheException("Access denied: " + path);
}

Expand Down Expand Up @@ -617,7 +617,7 @@ public void rename(Subject subject, @Nullable PnfsId pnfsId,
if (pnfsId != null) {
inode = new ExtendedInode(_fs, pnfsId, STAT);
} else {
if (!Subjects.isRoot(subject) &&
if (!Subjects.isExemptFromNamespaceChecks(subject) &&
_permissionHandler.canLookup(subject, sourceDirAttributes) != ACCESS_ALLOWED) {
throw new PermissionDeniedCacheException("Access denied: " + sourcePath);
}
Expand Down Expand Up @@ -659,8 +659,8 @@ public void rename(Subject subject, @Nullable PnfsId pnfsId,

/* Permission checks.
*/
if (!Subjects.isRoot(subject) || !overwrite) {
if (!Subjects.isRoot(subject) &&
if (!Subjects.isExemptFromNamespaceChecks(subject) || !overwrite) {
if (!Subjects.isExemptFromNamespaceChecks(subject) &&
_permissionHandler.canRename(subject,
sourceDirAttributes,
destDirAttributes,
Expand Down Expand Up @@ -1023,7 +1023,7 @@ public FileAttributes getFileAttributes(Subject subject, PnfsId pnfsId,
try {
ExtendedInode inode = new ExtendedInode(_fs, pnfsId, STAT);

if (Subjects.isRoot(subject)) {
if (Subjects.isExemptFromNamespaceChecks(subject)) {
return getFileAttributes(inode, attr);
}

Expand Down Expand Up @@ -1060,9 +1060,9 @@ public FileAttributes setFileAttributes(Subject subject, PnfsId pnfsId,
_log.debug("File attributes update: {}", attr.getDefinedAttributes());

try {
ExtendedInode inode = new ExtendedInode(_fs, pnfsId, Subjects.isRoot(subject) ? NO_STAT : STAT);
ExtendedInode inode = new ExtendedInode(_fs, pnfsId, Subjects.isExemptFromNamespaceChecks(subject) ? NO_STAT : STAT);

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes =
getFileAttributesForPermissionHandler(inode);

Expand Down Expand Up @@ -1210,7 +1210,7 @@ public void list(Subject subject, String path, Glob glob, Range<Integer> range,
throw new NotDirCacheException("Not a directory: " + path);
}

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes =
getFileAttributesForPermissionHandler(dir);
if (!dir.isDirectory()) {
Expand Down Expand Up @@ -1258,7 +1258,7 @@ public void list(Subject subject, String path, Glob glob, Range<Integer> range,
private ExtendedInode mkdir(Subject subject, ExtendedInode parent, String name, int uid, int gid, int mode)
throws ChimeraFsException, CacheException
{
if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributesOfParent
= getFileAttributesForPermissionHandler(parent);
if (_permissionHandler.canCreateSubDir(subject, attributesOfParent) != ACCESS_ALLOWED) {
Expand Down Expand Up @@ -1338,7 +1338,7 @@ public FsPath createUploadPath(Subject subject, FsPath path, FsPath rootPath,
: lookupDirectory(subject, path.parent());

FileAttributes attributesOfParent =
!Subjects.isRoot(subject)
!Subjects.isExemptFromNamespaceChecks(subject)
? getFileAttributesForPermissionHandler(parentOfPath)
: null;

Expand All @@ -1352,7 +1352,7 @@ public FsPath createUploadPath(Subject subject, FsPath path, FsPath rootPath,
}
/* User must be authorized to delete existing file.
*/
if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributesOfPath =
getFileAttributesForPermissionHandler(inodeOfPath);
if (_permissionHandler.canDeleteFile(subject,
Expand All @@ -1366,7 +1366,7 @@ public FsPath createUploadPath(Subject subject, FsPath path, FsPath rootPath,

/* User must be authorized to create file.
*/
if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
if (_permissionHandler.canCreateFile(subject, attributesOfParent) != ACCESS_ALLOWED) {
throw new PermissionDeniedCacheException("Access denied: " + path);
}
Expand Down Expand Up @@ -1536,7 +1536,7 @@ public FileAttributes commitUpload(Subject subject, FsPath temporaryPath, FsPath
}
/* User must be authorized to delete existing file.
*/
if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributesOfParent =
getFileAttributesForPermissionHandler(finalDirInode);
FileAttributes attributesOfFile =
Expand Down Expand Up @@ -1650,7 +1650,7 @@ public byte[] readExtendedAttribute(Subject subject, FsPath path, String name)
try {
ExtendedInode target = pathToInode(subject, path.toString());

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes = getFileAttributesForPermissionHandler(target);
if (target.isDirectory()) {
if (_permissionHandler.canListDir(subject, attributes) != ACCESS_ALLOWED) {
Expand Down Expand Up @@ -1678,7 +1678,7 @@ public void writeExtendedAttribute(Subject subject, FsPath path, String name,
try {
ExtendedInode target = pathToInode(subject, path.toString());

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes = getFileAttributesForPermissionHandler(target);
if (target.isDirectory()) {
if (_permissionHandler.canCreateFile(subject, attributes) != ACCESS_ALLOWED) {
Expand Down Expand Up @@ -1725,7 +1725,7 @@ public Set<String> listExtendedAttributes(Subject subject, FsPath path)
try {
ExtendedInode target = pathToInode(subject, path.toString());

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes = getFileAttributesForPermissionHandler(target);
if (target.isDirectory()) {
if (_permissionHandler.canListDir(subject, attributes) != ACCESS_ALLOWED) {
Expand Down Expand Up @@ -1753,7 +1753,7 @@ public void removeExtendedAttribute(Subject subject, FsPath path, String name)
try {
ExtendedInode target = pathToInode(subject, path.toString());

if (!Subjects.isRoot(subject)) {
if (!Subjects.isExemptFromNamespaceChecks(subject)) {
FileAttributes attributes = getFileAttributesForPermissionHandler(target);
if (target.isDirectory()) {
if (_permissionHandler.canCreateFile(subject, attributes) != ACCESS_ALLOWED) {
Expand Down
Expand Up @@ -2474,7 +2474,7 @@ private void checkMask(PnfsMessage message)
private void checkMask(Subject subject, PnfsId pnfsId, Set<AccessMask> mask)
throws CacheException
{
if (!Subjects.isRoot(subject) && !mask.isEmpty()) {
if (!Subjects.isExemptFromNamespaceChecks(subject) && !mask.isEmpty()) {
Set<FileAttribute> required =
_permissionHandler.getRequiredAttributes();
FileAttributes attributes =
Expand All @@ -2491,7 +2491,7 @@ private void checkMask(Subject subject, PnfsId pnfsId, Set<AccessMask> mask)
private void checkMask(Subject subject, String path, Set<AccessMask> mask)
throws CacheException
{
if (!Subjects.isRoot(subject) && !mask.isEmpty()) {
if (!Subjects.isExemptFromNamespaceChecks(subject) && !mask.isEmpty()) {
Set<FileAttribute> required =
_permissionHandler.getRequiredAttributes();
PnfsId pnfsId = _nameSpaceProvider.pathToPnfsid(ROOT, path, false);
Expand Down
Expand Up @@ -41,6 +41,7 @@
import diskCacheV111.util.FsPath;

import org.dcache.auth.BearerTokenCredential;
import org.dcache.auth.ExemptFromNamespaceChecks;
import org.dcache.auth.JwtJtiPrincipal;
import org.dcache.auth.JwtSubPrincipal;
import org.dcache.auth.Subjects;
Expand Down Expand Up @@ -157,6 +158,7 @@ public void authenticate(Set<Object> publicCredentials, Set<Object> privateCrede
Restriction r = buildRestriction(issuer.getPrefix(), scopes);
LOGGER.debug("Authenticated user with restriction: {}", r);
restrictions.add(r);
identifiedPrincipals.add(new ExemptFromNamespaceChecks());
} catch (IOException e) {
throw new AuthenticationException(e.getMessage());
}
Expand Down

0 comments on commit 14f2019

Please sign in to comment.