Story #17464

Logging and restricting downloads in keep-web and keepproxy

Added by Peter Amstutz 3 months ago. Updated about 1 hour ago.

Status:
In Progress
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
06/15/2021
Due date:
% Done:

0%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

  • When user uploads or downloads a file on keep-web, write an entry to the API server logs table
  • User activity report recognizes logs from keep-web
  • Add config options to restrict uploading (PUT) and downloading (GET) (separately) through keep-web and keepproxy (a) for users (b) for admins
  • includes logging user uuid to journalctl logs

Subtasks

Task #17733: Review 17464-download-loggingIn ProgressTom Clegg

History

#1 Updated by Peter Amstutz 3 months ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)

#3 Updated by Peter Amstutz 3 months ago

  • Description updated (diff)
  • Subject changed from Logging downloads in keep-web to Logging downloads in keep-web and keepproxy

#4 Updated by Peter Amstutz 22 days ago

  • Target version set to 2021-05-26 sprint

#5 Updated by Peter Amstutz 22 days ago

  • Target version changed from 2021-05-26 sprint to 2021-06-09 sprint

#6 Updated by Peter Amstutz 20 days ago

  • Assigned To set to Peter Amstutz

#7 Updated by Peter Amstutz 12 days ago

  • Release set to 39

#8 Updated by Peter Amstutz 7 days ago

  • Subject changed from Logging downloads in keep-web and keepproxy to Logging and restricting downloads in keep-web and keepproxy

#9 Updated by Peter Amstutz 6 days ago

  • Target version changed from 2021-06-09 sprint to 2021-06-23 sprint

#10 Updated by Peter Amstutz about 1 hour ago

I'm getting what seems to be a low-probability race condition in testing:

fatal error: concurrent map iteration and map write

goroutine 3143 [running]:
runtime.throw(0xe6d335, 0x26)
    /var/lib/arvados/go/src/runtime/panic.go:1117 +0x72 fp=0xc002eadd98 sp=0xc002eadd68 pc=0x43a5f2
runtime.mapiternext(0xc002eade58)
    /var/lib/arvados/go/src/runtime/map.go:858 +0x54c fp=0xc002eade18 sp=0xc002eadd98 pc=0x412d8c
runtime.mapiterinit(0xd655a0, 0xc004f2a030, 0xc002eade58)
    /var/lib/arvados/go/src/runtime/map.go:848 +0x1c5 fp=0xc002eade38 sp=0xc002eade18 pc=0x412745
git.arvados.org/arvados.git/sdk/go/arvados.(*treenode).MemorySize(0xc004f2c000, 0x0)
    /usr/src/arvados/sdk/go/arvados/fs_base.go:336 +0xd2 fp=0xc002eaded8 sp=0xc002eade38 pc=0x8cec72
git.arvados.org/arvados.git/sdk/go/arvados.(*fileSystem).MemorySize(0xc004f2e000, 0x0)
    /usr/src/arvados/sdk/go/arvados/fs_base.go:631 +0x33 fp=0xc002eadef8 sp=0xc002eaded8 pc=0x8d1193
git.arvados.org/arvados.git/services/keep-web.(*cache).pruneSessions(0xc0044af288)
    /usr/src/arvados/services/keep-web/cache.go:277 +0x415 fp=0xc002eadfd8 sp=0xc002eadef8 pc=0xc80cf5
runtime.goexit()
    /var/lib/arvados/go/src/runtime/asm_amd64.s:1371 +0x1 fp=0xc002eadfe0 sp=0xc002eadfd8 pc=0x474241
created by git.arvados.org/arvados.git/services/keep-web.(*cache).GetSession
    /usr/src/arvados/services/keep-web/cache.go:249 +0x1b0

The MemorySize routine is really simple:

func (n *treenode) MemorySize() (size int64) {
    n.RLock()
    defer n.RUnlock()
    for _, inode := range n.inodes {
        size += inode.MemorySize()
    }
    return
}

So there must be something else stealthily modifying the map.

I noticed the Child() method modifies the inodes map but doesn't have any locking.

func (n *treenode) Child(name string, replace func(inode) (inode, error)) (child inode, err error) {

There's a note about "caller must have lock".

#11 Updated by Peter Amstutz about 1 hour ago

The above-mentiond race condition aside, the branch is ready for review

17464-download-logging @ commit:0da30aa9a699faa5e735c3aca2f2d6cd96429944

  • Adds "File download" and "File upload" log messages with the user uuid, user full name, and collection uuid
  • Posts an file_download or file_upload event to the logs table. The object_uuid is the user, the properties are the collection uuid.
  • Add seperate user / admin permission bits for file upload and download.
  • Adds documentation page explaining how to use the permissions feature
  • Adds tests for all the above functionality.

https://ci.arvados.org/view/Developer/job/developer-run-tests/2528/

Also available in: Atom PDF