Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Research: Secure View - Time Boxed - 3pd #8855

Closed
13 tasks
micbar opened this issue Apr 15, 2024 · 14 comments
Closed
13 tasks

Research: Secure View - Time Boxed - 3pd #8855

micbar opened this issue Apr 15, 2024 · 14 comments
Labels
Type:Story User Story

Comments

@micbar
Copy link
Contributor

micbar commented Apr 15, 2024

Description

User Stories

  • As a dev team, we want to have a concept before implementing a new feature

Value

  • Certainty
  • Avoid escalation of efforts

Acceptance Criteria

  • Written down technical approach regarding
    • How to implement permissions (Download or secure-view??)
    • How to give access to the content via the collaboration service
    • How to show the state in WebDAV and LibreGraph
    • Document in an ADR

Definition of ready

  • Everybody needs to understand the value written in the user story
  • Acceptance criteria have to be defined
  • All dependencies of the user story need to be identified
  • Feature should be seen from an end user perspective
  • Story has to be estimated
  • Story points need to be less than 20

Definition of done

  • Functional requirements
    • Functionality described in the user story works
    • Acceptance criteria are fulfilled
  • Quality
    • Code review happened
    • CI is green (that includes new and existing automated tests)
    • Critical code received unit tests by the developer
  • Non-functional requirements
    • No sonar cloud issues
  • Configuration changes
    • The next branch of the ocis charts is compatible
@micbar micbar added the Type:Story User Story label Apr 15, 2024
@micbar micbar changed the title Research: Secure View Research: Secure View - Time Boxed - 3pd Apr 15, 2024
@dragonchaser
Copy link
Member

Why is there already a merged PR? As I read it there should be an ADR first....

@kobergj
Copy link
Collaborator

kobergj commented Apr 29, 2024

That just added the role - this ticket is about figuring out how to use it.

@dragonchaser
Copy link
Member

dragonchaser commented Apr 30, 2024

We need to set the Attributes in

fileInfo := FileInfo{
// OwnerId must use only alphanumeric chars (https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/rest/files/checkfileinfo/checkfileinfo-response#requirements-for-user-identity-properties)
OwnerId: hex.EncodeToString([]byte(statRes.GetInfo().GetOwner().GetOpaqueId() + "@" + statRes.GetInfo().GetOwner().GetIdp())),
Size: int64(statRes.GetInfo().GetSize()),
Version: strconv.FormatUint(statRes.GetInfo().GetMtime().GetSeconds(), 10) + "." + strconv.FormatUint(uint64(statRes.GetInfo().GetMtime().GetNanos()), 10),
BaseFileName: path.Base(statRes.GetInfo().GetPath()),
BreadcrumbDocName: path.Base(statRes.GetInfo().GetPath()),
// to get the folder we actually need to do a GetPath() request
//BreadcrumbFolderName: path.Dir(statRes.Info.Path),
UserCanNotWriteRelative: true,
HostViewUrl: wopiContext.ViewAppUrl,
HostEditUrl: wopiContext.EditAppUrl,
//EnableOwnerTermination: true, // enable only for collabora? wopivalidator is complaining
EnableOwnerTermination: false,
SupportsExtendedLockLength: true,
SupportsGetLock: true,
SupportsLocks: true,
}
in the same way they are set in the richdocuments app in oc10 (see: https://github.com/owncloud/richdocuments/blob/d9d2c2c624649fe6106617f2f416b40a40b98351/lib/Controller/WopiController.php#L244-L268 )

@dragonchaser
Copy link
Member

dragonchaser commented Apr 30, 2024

func (c *ContentConnector) GetFile(ctx context.Context, writer io.Writer) error {
needs to be adapted accordingly to respect the permissions set.
TODO: We need to figure out how to create a permission scope that is limited to the specific item in the share.

@butonic
Copy link
Member

butonic commented Apr 30, 2024

The storageprovider has to be able to decide if a InitiateFileDownload request is allowed. It can only look at the context, which currently only contains the user info. If the user does not have the InitiateDownload permission he currently cannot download the file. We cannot add a view-only permission to the scope, that would blow up the scope and it would have to hapen during login.

The storageprovider could check a Permission, but then any client making a CS3 RPC could make the same request and download the file.

Instead of a permission check we could add a ViewOnly Permission to the Permission set, but then we still need to check if the request was made by a trusted client, like a WOPI server.

AFAICT we need to carry the application in the scope. In this case WOPI (or to be specific only 'Collabora') supports 'secure view'. That trusted application could be put into the scope when the WOPI server makes the RPC request.

Collabora documented their WOPI extensions in https://sdk.collaboraonline.com/docs/advanced_integration.html#checkfileinfo-extended-response-properties

🤔

@butonic
Copy link
Member

butonic commented Apr 30, 2024

The CS3 API already has view modes for the open in app rpc:

enum ViewMode {
VIEW_MODE_INVALID = 0;
// The resource can be opened but not downloaded.
VIEW_MODE_VIEW_ONLY = 1;
// The resource can be downloaded.
VIEW_MODE_READ_ONLY = 2;
// The resource can be downloaded and updated. The underlying application
// MUST be a fully capable editor to support this mode.
VIEW_MODE_READ_WRITE = 3;
// The resource can be downloaded and updated, but must be shown in
// preview mode. If the underlying application does not support a preview mode,
// or if in a view-only mode users are not allowed to switch to edit mode,
// then this mode MUST fall back to READ_WRITE.
VIEW_MODE_PREVIEW = 4;
}

@butonic
Copy link
Member

butonic commented Apr 30, 2024

In the libregraph API we have mapped the CS3 permissions to actions:

  • initiate_file_download -> libre.graph/driveItem/content/read
  • viewOnly -> libre.graph/driveItem/content/viewOnly would be an option

I don't like to put all permissions into the permissions set. It forces all storage drivers to implement that permission.

🤔

The naming of the action should be a camelCased CRUD verb: https://learn.microsoft.com/en-us/graph/api/resources/unifiedrolepermission?view=graph-rest-1.0#allowedresourceactions-property

But I don't see a good way of mapping viewOnly to CRUD verbs.

@butonic
Copy link
Member

butonic commented Apr 30, 2024

Two approaches:

1. let WOPI use a service account with a limited scope

Let WOPI mint a token with scope to limit the request to that specific resource.

2. attach app password to share in file metadata

Creates a specific token that allows downloading the file, similar to a presigned url

3. Introduce ViewOnly permission

Create a new CS3 permission that allows the InitiateFileDownload call for trusted clients, like the wopi server. The trust relationship can be established using a shared secret, a new scope, client secrets ... tokens ... whatever

4. Use CS3 Permission Check call

Instead of a ViewOnly permission we could make a call to the permissions service. I'm not sure if that request can also carry the client information. 👀

5. Create two shares

  • one for the user with a stat permission
  • one for the wopi service account with an initiate file download permission
    The the wopi server could "impersonate" the user (both users are in the context) and the storage provider can check both of them?

Or the wopi server is an application in the context, additionally to the user in the context.

The crux is that the application has more permissions than the user. Which I assume is the reason why MS Graph has application permissions.

When sharing a file the application does - in this case - not act like a constraint. Both conditions have to be true:

  1. The user must have stat permissions
  2. The application must have initiate file download permission
    For this we would need 2 ACEs per share ... 🤔

6. Let the gateway mint an additional viewOnlyToken for the approvider

The gatway can mint an additional viewOnlyToken when a user makes an OpenInApp call having view only permissions.
see #8855 (comment)

@butonic
Copy link
Member

butonic commented Apr 30, 2024

Permission Checks:

message CheckPermissionRequest {
	// REQUIRED.
	// The permission to check.
	string permission = 1;
	// REQUIRED.
	// The subject holding the permission.
	SubjectReference subject_ref = 2;
	// OPTIONAL.
	// The target resource of the permission.
	cs3.storage.provider.v1beta1.Reference ref = 3;
}

message SubjectReference {
	oneof spec {
		cs3.identity.user.v1beta1.UserId user_id = 1;
		cs3.identity.group.v1beta1.GroupId group_id = 2;
	}
}

So currently the request cannot carry any client/application information.

@butonic
Copy link
Member

butonic commented Apr 30, 2024

The storageprovider has to determine access basedon the user, the application and the sharing permissions if a download can be initiated.

@micbar
Copy link
Contributor Author

micbar commented Apr 30, 2024

Permission Checks:

message CheckPermissionRequest {
	// REQUIRED.
	// The permission to check.
	string permission = 1;
	// REQUIRED.
	// The subject holding the permission.
	SubjectReference subject_ref = 2;
	// OPTIONAL.
	// The target resource of the permission.
	cs3.storage.provider.v1beta1.Reference ref = 3;
}

message SubjectReference {
	oneof spec {
		cs3.identity.user.v1beta1.UserId user_id = 1;
		cs3.identity.group.v1beta1.GroupId group_id = 2;
	}
}

So currently the request cannot carry any client/application information.

Please do not mix the user permissions with file permissions.

@butonic
Copy link
Member

butonic commented Apr 30, 2024

We could mint an additional viewOnlyToken in the gateway and send it as part of the OpenInApp call. For now we can use Opaque ... should be come a distinct property.
The collaboration service aka CS3 appprovider can then use that token to make the InitiateFileDownload request.

The gateway already does a stat request. To decide if we should mint a viewOnlyToken (which effectively allows downloading the file content) We can either make a call to the share manager, or we introduce a ViewOnly Permission. The former would rely on the share manager that captures the share intent. The latter would need a code change ... and it needs special handling in every storage provider implementation.

diff --git a/internal/grpc/services/gateway/appprovider.go b/internal/grpc/services/gateway/appprovider.go
index 3a91e12a0..442e5896a 100644
--- a/internal/grpc/services/gateway/appprovider.go
+++ b/internal/grpc/services/gateway/appprovider.go
@@ -173,7 +173,8 @@ func (s *svc) openLocalResources(ctx context.Context, ri *storageprovider.Resour
                ResourceInfo: ri,
                ViewMode:     providerpb.ViewMode(vm),
                AccessToken:  accessToken,
-               Opaque:       opaque,
+               // ViewOnlyToken: viewOnlyToken // scoped to the shared resource if the stat response hase a ViewOnly permission
+               Opaque: opaque,
        }
 
        res, err := appProviderClient.OpenInApp(ctx, appProviderReq)

Always minting a ViewOnlyToken when a user has the stat permission is far too permissive IMO.

But adding a new ViewOnly permission would require code in every storage provider implementation.

And since the posix world has no use for that permission I am leaning towards the additional request to the share manager.

The gateway will need a service account that it can use to mint a ViewOnlyToken for the OpenInApp Request.

@butonic
Copy link
Member

butonic commented May 2, 2024

the current ocis collaboration service also needs a few changes

  • it may have to learn the wopi proxy trick unless we use the wopi proxy secret to sign the wopi JWT
  • it currently uses one JWT secret for three things: 1. reva access token signature verification, 2. wopi JWT creation and 3. access token encryption

@micbar
Copy link
Contributor Author

micbar commented May 3, 2024

Follow ups created. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Story User Story
Projects
Status: Done
Development

No branches or pull requests

4 participants