Merge pull request #150 from kbase/develop

Release 0.1.4 (develop -> master)

.github/workflows/test.yml

@@ -18,7 +18,7 @@ env:
   AUTH2_JAR_NAME: kbase-auth2-test-shadow-all-0.7.0.jar
 
 jobs:
-  workspace_deluxe_tests:
+  blobstore_tests:
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
@@ -84,4 +84,5 @@ jobs:
       - name: Upload coverage to Codecov
         uses: codecov/codecov-action@v3
         with:
+          token: ${{ secrets.CODECOV_TOKEN }}
           fail_ci_if_error: true

README.md

@@ -101,8 +101,17 @@ This data structure is identical to Shock's error data structure.
 
 # API
 
-Requests are authenticated by including the header `Authorization: OAuth <kbase token>` in the
-request.
+## Authentication
+
+Requests are authenticated by including the header `Authorization: OAuth <kbase token>` or
+including a cookie with the value of `<kbase token>` in the request.
+
+The names of cookies that the server will check are set in the deployment configuration file.
+
+The header takes precedence, then each cookie in the list in the configuration file in order.
+
+Note that for backwards compatibility, incorrect or invalid authentication headers respond with a
+400 HTTP code. Invalid cookies respond with the appropriate 401 code.
 
 ## Root
 
@@ -141,8 +150,10 @@ curl -H "Authorization: OAuth $KBASE_TOKEN" -T mylittlefile
   "http://<host>/node?filename=mylittlefile&format=text"
 ```
 
-`filename` can be at most 256 characters with no control characters.  
-`format` can be at most 100 characters with no control characters.
+`filename` can be at most 256 characters consisting of only unicode alphanumerics, space, and
+the characters `[ ] ( ) = . - _`.  
+`format` can be at most 100 characters consisting of only unicode alphanumerics and
+the characters `- _`. 
 
 ## Copy a node
 ```
@@ -171,7 +182,7 @@ RETURNS: an ACL.
 ## Download a file from a node
 ```
 AUTHORIZATION OPTIONAL
-GET /node/<id>?download[_raw][&seek=#][&length=#]
+GET /node/<id>?download[_raw][&seek=#][&length=#][&del]
 
 RETURNS: the file content.
 ```
@@ -186,6 +197,10 @@ to the file size is an error. Defaults to 0.
 `length` may be greater than the remaining file length. Defaults to 0, which indicates that the
 remainder of the file should be returned.
 
+`del` causes the node to be deleted once the file contents have been streamed. The user must
+be the node owner or a service administrator. Note this is playing very fast and loose with the
+semantics of an HTTP GET.
+
 ## Set a node to be publicly readable
 ```
 AUTHORIZATION REQUIRED
@@ -252,8 +267,8 @@ The form **may** contain a part called `format` where the part contents are the
 file, equivalent to the `format` query parameter for the standard upload method and with the same
 restrictions. The `format` part **MUST** come before the `upload` part.
 
-Any file name provided in the `Content-Disposition` header can be at most 256 characters with no
-control characters.
+Any file name provided in the `Content-Disposition` header has the same restrictions as the
+filename parameter for the standard upload method.
 
 ### Curl example
 

RELEASE_NOTES.md

@@ -1,17 +1,26 @@
+# 0.1.4
+
+* Added the `del` param when downloading the file from a node.
+* The Blobstore will now look for auth tokens in cookies specified in the deployment configuration.
+* The filename and format strings are now much more restrictive in regard to allowed contents.
+  See the API documenation for details.
+  * Any extant data is not affected and will be returned when requested as normal.
+
 # 0.1.3
 
-- Added GHA workflows and removed Travis CI
-- MongoController is now compatible with Mongo versions 2 through 7
-- Updated test config file to specify the auth2 shadow jar path vs. the jars repo path
+* Added GHA workflows and removed Travis CI
+* MongoController is now compatible with Mongo versions 2 through 7
+* Updated test config file to specify the auth2 shadow jar path vs. the jars repo path
 
 # 0.1.2
 
-- Support for disabling SSL verification of remote S3 certificates (default false) with the s3-disable-ssl-verify option in the configuration file.
+* Support for disabling SSL verification of remote S3 certificates (default false)
+  with the s3-disable-ssl-verify option in the configuration file.
 
 # 0.1.1
 
-- Added seek & length parameters to file download requests
+* Added seek & length parameters to file download requests
 
 # 0.1.0
 
-- Initial release
+* Initial release

app/blobstore.go

@@ -19,7 +19,7 @@ import (
 
 const (
 	name        = "blobstore"
-	version     = "0.1.3"
+	version     = "0.1.4"
 	shockname   = "Shock"
 	shockver    = "0.9.6" // do not increment
 	deprecation = "The id and version fields are deprecated."

config/config.go

@@ -46,6 +46,9 @@ const (
 	// KeyAuthAdminRoles is the configuration key where the value is comma-delimited auth server
 	// roles that denote that a user is a blobstore admin
 	KeyAuthAdminRoles = "kbase-auth-admin-roles"
+	// KeyAuthTokenCookies is the configuartion key where the value is comma-delimited cookie
+	// names where the serivce should look for authentication tokens.
+	KeyAuthTokenCookies = "kbase-auth-token-cookies"
 	// KeyDontTrustXIPHeaders is the configuration key where the value determines whether to
 	// distrust the X-Forwarded-For and X-Real-IP headers (true) or not (anything else).
 	KeyDontTrustXIPHeaders = "dont-trust-x-ip-headers"
@@ -82,6 +85,9 @@ type Config struct {
 	// AuthAdminRoles are the auth server roles that denote that a user is a blobstore admin.
 	// It is never nil but may be empty.
 	AuthAdminRoles *[]string
+	// AuthTokenCookies are the cookie names to check for auth tokens.
+	// It is never nil but may be empty.
+	AuthTokenCookies *[]string
 	// DontTrustXIPHeaders determines whether to distrust the X-Forwarded-For and X-Real-IP
 	// headers.
 	DontTrustXIPHeaders bool
@@ -112,6 +118,7 @@ func New(configFilePath string) (*Config, error) {
 	s3region, err := getString(err, configFilePath, sec, KeyS3Region, true)
 	authurl, err := getURL(err, configFilePath, sec, KeyAuthURL)
 	roles, err := getStringList(err, configFilePath, sec, KeyAuthAdminRoles)
+	cookies, err := getStringList(err, configFilePath, sec, KeyAuthTokenCookies)
 	xip, err := getString(err, configFilePath, sec, KeyDontTrustXIPHeaders, false)
 	if err != nil {
 		return nil, err
@@ -137,6 +144,7 @@ func New(configFilePath string) (*Config, error) {
 			S3Region:            s3region,
 			AuthURL:             authurl,
 			AuthAdminRoles:      roles,
+			AuthTokenCookies:    cookies,
 			DontTrustXIPHeaders: "true" == xip,
 		},
 		nil

config/config_test.go

@@ -93,6 +93,7 @@ func (t *TestSuite) TestMinimalConfig() {
 		S3DisableSSLVerify:  false,
 		AuthURL:             u,
 		AuthAdminRoles:      &[]string{},
+		AuthTokenCookies:    &[]string{},
 		DontTrustXIPHeaders: false,
 	}
 	t.Equal(&expected, cfg, "incorrect config")
@@ -114,6 +115,7 @@ func (t *TestSuite) TestMinimalConfigWhitespaceFields() {
 		"s3-region =       us-west-1    \t   ",
 		"kbase-auth-url = https://kbase.us/authyauth",
 		"kbase-auth-admin-roles =    \t     ",
+		"kbase-auth-token-cookies =    \t     ",
 		"dont-trust-x-ip-headers =      \t     ",
 	)
 	cfg, err := New(filePath)
@@ -132,6 +134,7 @@ func (t *TestSuite) TestMinimalConfigWhitespaceFields() {
 		S3DisableSSLVerify:  false,
 		AuthURL:             u,
 		AuthAdminRoles:      &[]string{},
+		AuthTokenCookies:    &[]string{},
 		DontTrustXIPHeaders: false,
 	}
 	t.Equal(&expected, cfg, "incorrect config")
@@ -153,6 +156,7 @@ func (t *TestSuite) TestMaximalConfig() {
 		"s3-disable-ssl-verify=     true    ",
 		"kbase-auth-url = https://kbase.us/authyauth",
 		"kbase-auth-admin-roles =    \t     ,    foo   , \tbar\t , ,  baz ,,",
+		"kbase-auth-token-cookies =    \t   , chokkiechip   , \toreoinmilk\t , , earwaxNsnot ,,",
 		"dont-trust-x-ip-headers =     true   \t  ",
 	)
 	cfg, err := New(filePath)
@@ -173,6 +177,7 @@ func (t *TestSuite) TestMaximalConfig() {
 		S3Region:            "us-west-1",
 		AuthURL:             u,
 		AuthAdminRoles:      &[]string{"foo", "bar", "baz"},
+		AuthTokenCookies:    &[]string{"chokkiechip", "oreoinmilk", "earwaxNsnot"},
 		DontTrustXIPHeaders: true,
 	}
 	t.Equal(&expected, cfg, "incorrect config")

core/values/values.go

@@ -9,11 +9,23 @@ import (
 )
 
 const (
-	maxFileNameSize = 256
-	maxFormatSize   = 100
+	maxFileNameSize    = 256
+	maxFormatSize      = 100
+	allowedFileChars   = "[]()=-_. "
+	allowedFormatChars = "_-"
 )
 
 var md5regex = regexp.MustCompile("^[a-fA-F0-9]{32}$")
+var fileCharsSet = createAllowedCharsSet(allowedFileChars)
+var formatCharsSet = createAllowedCharsSet(allowedFormatChars)
+
+func createAllowedCharsSet(allowedSpecialChars string) map[rune]struct{} {
+	allowedCharsSet := make(map[rune]struct{})
+	for _, char := range allowedSpecialChars {
+		allowedCharsSet[char] = struct{}{}
+	}
+	return allowedCharsSet
+}
 
 // MD5 contains a valid MD5 string.
 type MD5 struct {
@@ -53,7 +65,7 @@ type FileName struct {
 
 // NewFileName creates a new filename.
 func NewFileName(name string) (*FileName, error) {
-	name, err := checkString(name, "File name", maxFileNameSize)
+	name, err := checkString(name, "File name", maxFileNameSize, fileCharsSet)
 	if err != nil {
 		return nil, err
 	}
@@ -72,7 +84,7 @@ type FileFormat struct {
 
 // NewFileFormat creates a new filename.
 func NewFileFormat(name string) (*FileFormat, error) {
-	name, err := checkString(name, "File format", maxFormatSize)
+	name, err := checkString(name, "File format", maxFormatSize, formatCharsSet)
 	if err != nil {
 		return nil, err
 	}
@@ -84,17 +96,37 @@ func (fn *FileFormat) GetFileFormat() string {
 	return fn.fileFormat
 }
 
-func checkString(s string, name string, maxSize int) (string, error) {
+func checkString(
+	s string, name string, maxSize int, allowedSpcChars map[rune]struct{},
+) (string, error) {
 	s = strings.TrimSpace(s)
 	if len(s) > maxSize {
 		return "", NewIllegalInputError(fmt.Sprintf("%s is > %d bytes", name, maxSize))
 	}
+	// check for control characters first to avoid returning / logging a string with control chars
 	if containsControlChar(s) {
 		return "", NewIllegalInputError(fmt.Sprintf("%s contains control characters", name))
 	}
+	for _, r := range(s) {
+		if !isAllowedChar(r, allowedSpcChars) {
+			return "", NewIllegalInputError(
+				fmt.Sprintf("%s string %s contains an illegal character: %q", name, s, r),
+			)
+		}
+	}
 	return s, nil
 }
 
+func isAllowedChar(r rune, allowedSpcChars map[rune]struct{}) bool {
+	if unicode.IsLetter(r) || unicode.IsDigit(r) {
+		return true
+	}
+	if _, exists := allowedSpcChars[r]; exists {
+		return true
+	}
+	return false
+}
+
 func containsControlChar(s string) bool {
 	for _, c := range s {
 		if unicode.IsControl(c) {

core/values/values_test.go

@@ -51,7 +51,7 @@ func TestFileName(t *testing.T) {
 }
 
 func fileNameString() string {
-	s := "ab9 8&#']K[1*(&)ꢇ]" // 20 bytes
+	s := "ab9_8४.[]( )1=-ꢇ" // 20 bytes
 
 	b := strings.Builder{}
 	for i := 0; i < 12; i++ {
@@ -72,18 +72,34 @@ func TestFileNameFail(t *testing.T) {
 	assert.Nil(t, fn, "expected error")
 	assert.Equal(t, NewIllegalInputError("File name contains control characters"), err,
 		"incorrect error")
+	for _, ch := range []string{"*", ":", "?", "|", "✈", "🐱", "∞", "€", "◊"} {
+		fn, err = NewFileName("abc"+ch+"neg")
+		assert.Nil(t, fn, "expected error")
+		errstr := "File name string abc"+ch+"neg contains an illegal character: '"+ch+"'"
+		assert.Equal(t, NewIllegalInputError(errstr), err, "incorrect error")
+	}
 }
 
 func TestFileFormat(t *testing.T) {
-	fns := fileNameString()[:100]
+	fns := formatString()
 	assert.Equal(t, 100, len(fns), "incorrect length")
 	fn, err := NewFileFormat("    \t       " + fns + "    \t       ")
 	assert.Nil(t, err, "unexpected error")
 	assert.Equal(t, fns, fn.GetFileFormat())
 }
 
+func formatString() string {
+	s := "ab9_8४K3Z23o1n-ꢇ" // 20 bytes
+
+	b := strings.Builder{}
+	for i := 0; i < 5; i++ {
+		b.Write([]byte(s))
+	}
+	return b.String()
+}
+
 func TestFileFormatFail(t *testing.T) {
-	fns := fileNameString()[:100]
+	fns := formatString()
 	assert.Equal(t, 100, len(fns), "incorrect length")
 	fn, err := NewFileFormat(fns + "a")
 	assert.Nil(t, fn, "expected error")
@@ -93,4 +109,11 @@ func TestFileFormatFail(t *testing.T) {
 	assert.Nil(t, fn, "expected error")
 	assert.Equal(t, NewIllegalInputError("File format contains control characters"), err,
 		"incorrect error")
+	badchrs := []string{"*", ":", "?", "|", "✈", "🐱", "∞", "€", "◊", "=", "[", "]", "(", ")", "."}
+	for _, ch := range badchrs {
+		fn, err = NewFileFormat("abc"+ch+"neg")
+		assert.Nil(t, fn, "expected error")
+		errstr := "File format string abc"+ch+"neg contains an illegal character: '"+ch+"'"
+		assert.Equal(t, NewIllegalInputError(errstr), err, "incorrect error")
+	}
 }

deploy.cfg.example

@@ -30,6 +30,9 @@ s3-region = us-west-1
 kbase-auth-url = https://kbase.us/services/auth
 # KBase auth server custom roles that denote the user is a blobstore admin. Comma delimited.
 kbase-auth-admin-roles = KBASE_ADMIN, BLOBSTORE_ADMIN
+# A list of comma separated cookie names to check for authentication tokens.
+# The authentication header is checked first, then each cookie in the list in order.
+# kbase-auth-token-cookies = 
 
 # If "true", make the server ignore the X-Forwarded-For and X-Real-IP headers. Otherwise
 # (the default behavior), the logged IP address for a request, in order of precedence, is