* s3api: fix authentication bypass and potential SIGSEGV * s3api: improve security tests with positive cases and nil identity guards * s3api: fix secondary authentication bypass in AuthSignatureOnly * s3api: refactor account loading and refine security tests based on review feedback * s3api: refine security tests with realistic signature failures * Update weed/s3api/auth_security_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
Chris Lu
@@ -271,42 +271,36 @@ func (iam *IdentityAccessManagement) loadS3ApiConfiguration(config *iam_pb.S3Api
|
||||
|
||||
for _, account := range config.Accounts {
|
||||
glog.V(3).Infof("loading account name=%s, id=%s", account.DisplayName, account.Id)
|
||||
accounts[account.Id] = &Account{
|
||||
Id: account.Id,
|
||||
DisplayName: account.DisplayName,
|
||||
EmailAddress: account.EmailAddress,
|
||||
}
|
||||
switch account.Id {
|
||||
case AccountAdmin.Id:
|
||||
AccountAdmin = Account{
|
||||
Id: account.Id,
|
||||
DisplayName: account.DisplayName,
|
||||
EmailAddress: account.EmailAddress,
|
||||
}
|
||||
accounts[account.Id] = &AccountAdmin
|
||||
foundAccountAdmin = true
|
||||
case AccountAnonymous.Id:
|
||||
AccountAnonymous = Account{
|
||||
Id: account.Id,
|
||||
DisplayName: account.DisplayName,
|
||||
EmailAddress: account.EmailAddress,
|
||||
}
|
||||
accounts[account.Id] = &AccountAnonymous
|
||||
foundAccountAnonymous = true
|
||||
default:
|
||||
t := Account{
|
||||
Id: account.Id,
|
||||
DisplayName: account.DisplayName,
|
||||
EmailAddress: account.EmailAddress,
|
||||
}
|
||||
accounts[account.Id] = &t
|
||||
}
|
||||
if account.EmailAddress != "" {
|
||||
emailAccount[account.EmailAddress] = accounts[account.Id]
|
||||
}
|
||||
}
|
||||
if !foundAccountAdmin {
|
||||
accounts[AccountAdmin.Id] = &AccountAdmin
|
||||
emailAccount[AccountAdmin.EmailAddress] = &AccountAdmin
|
||||
accounts[AccountAdmin.Id] = &Account{
|
||||
DisplayName: AccountAdmin.DisplayName,
|
||||
EmailAddress: AccountAdmin.EmailAddress,
|
||||
Id: AccountAdmin.Id,
|
||||
}
|
||||
emailAccount[AccountAdmin.EmailAddress] = accounts[AccountAdmin.Id]
|
||||
}
|
||||
if !foundAccountAnonymous {
|
||||
accounts[AccountAnonymous.Id] = &AccountAnonymous
|
||||
emailAccount[AccountAnonymous.EmailAddress] = &AccountAnonymous
|
||||
accounts[AccountAnonymous.Id] = &Account{
|
||||
DisplayName: AccountAnonymous.DisplayName,
|
||||
EmailAddress: AccountAnonymous.EmailAddress,
|
||||
Id: AccountAnonymous.Id,
|
||||
}
|
||||
emailAccount[AccountAnonymous.EmailAddress] = accounts[AccountAnonymous.Id]
|
||||
}
|
||||
for _, ident := range config.Identities {
|
||||
glog.V(3).Infof("loading identity %s (disabled=%v)", ident.Name, ident.Disabled)
|
||||
@@ -615,9 +609,8 @@ func (iam *IdentityAccessManagement) authRequestWithAuthType(r *http.Request, ac
|
||||
amzAuthType = "SigV4"
|
||||
case authTypeStreamingUnsigned:
|
||||
glog.V(3).Infof("unsigned streaming upload")
|
||||
// no amzAuthType set for this case in original code?
|
||||
// Actually original explicitly returned ErrNone without setting identity
|
||||
return identity, s3err.ErrNone, reqAuthType
|
||||
identity, s3Err = iam.reqSignatureV4Verify(r)
|
||||
amzAuthType = "SigV4"
|
||||
case authTypeJWT:
|
||||
glog.V(3).Infof("jwt auth type detected, iamIntegration != nil? %t", iam.iamIntegration != nil)
|
||||
r.Header.Set(s3_constants.AmzAuthType, "Jwt")
|
||||
@@ -749,7 +742,8 @@ func (iam *IdentityAccessManagement) AuthSignatureOnly(r *http.Request) (*Identi
|
||||
|
||||
case authTypeStreamingUnsigned:
|
||||
glog.V(3).Infof("unsigned streaming upload")
|
||||
return identity, s3err.ErrNone
|
||||
identity, s3Err = iam.reqSignatureV4Verify(r)
|
||||
authType = "SigV4"
|
||||
case authTypeJWT:
|
||||
glog.V(3).Infof("jwt auth type detected, iamIntegration != nil? %t", iam.iamIntegration != nil)
|
||||
r.Header.Set(s3_constants.AmzAuthType, "Jwt")
|
||||
@@ -783,6 +777,9 @@ func (iam *IdentityAccessManagement) AuthSignatureOnly(r *http.Request) (*Identi
|
||||
}
|
||||
|
||||
func (identity *Identity) canDo(action Action, bucket string, objectKey string) bool {
|
||||
if identity == nil {
|
||||
return false
|
||||
}
|
||||
if identity.isAdmin() {
|
||||
return true
|
||||
}
|
||||
@@ -829,6 +826,9 @@ func (identity *Identity) canDo(action Action, bucket string, objectKey string)
|
||||
}
|
||||
|
||||
func (identity *Identity) isAdmin() bool {
|
||||
if identity == nil {
|
||||
return false
|
||||
}
|
||||
return slices.Contains(identity.Actions, s3_constants.ACTION_ADMIN)
|
||||
}
|
||||
|
||||
|
||||
181
weed/s3api/auth_security_test.go
Normal file
181
weed/s3api/auth_security_test.go
Normal file
@@ -0,0 +1,181 @@
|
||||
package s3api
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/sha256"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/aws/aws-sdk-go-v2/aws"
|
||||
v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func signRawHTTPRequest(ctx context.Context, req *http.Request, accessKey, secretKey, region string) error {
|
||||
creds := aws.Credentials{
|
||||
AccessKeyID: accessKey,
|
||||
SecretAccessKey: secretKey,
|
||||
}
|
||||
signer := v4.NewSigner()
|
||||
payloadHash := fmt.Sprintf("%x", sha256.Sum256([]byte{}))
|
||||
return signer.SignHTTP(ctx, creds, req, payloadHash, "s3", region, time.Now())
|
||||
}
|
||||
|
||||
func TestReproIssue7912(t *testing.T) {
|
||||
// Create a temporary s3.json
|
||||
configContent := `{
|
||||
"identities": [
|
||||
{
|
||||
"name": "xx",
|
||||
"credentials": [
|
||||
{
|
||||
"accessKey": "xx_access_key",
|
||||
"secretKey": "xx_secret_key"
|
||||
}
|
||||
],
|
||||
"actions": ["Admin", "Read", "Write", "List", "Tagging"]
|
||||
},
|
||||
{
|
||||
"name": "read_only_user",
|
||||
"credentials": [
|
||||
{
|
||||
"accessKey": "readonly_access_key",
|
||||
"secretKey": "readonly_secret_key"
|
||||
}
|
||||
],
|
||||
"actions": ["Read", "List"]
|
||||
}
|
||||
]
|
||||
}`
|
||||
tmpFile, err := os.CreateTemp("", "s3-config-*.json")
|
||||
assert.NoError(t, err)
|
||||
defer os.Remove(tmpFile.Name())
|
||||
|
||||
_, err = tmpFile.Write([]byte(configContent))
|
||||
assert.NoError(t, err)
|
||||
tmpFile.Close()
|
||||
|
||||
// Initialize Identities Access Management
|
||||
option := &S3ApiServerOption{
|
||||
Config: tmpFile.Name(),
|
||||
}
|
||||
iam := NewIdentityAccessManagementWithStore(option, "memory")
|
||||
|
||||
assert.True(t, iam.isEnabled(), "Auth should be enabled")
|
||||
|
||||
// Test case 1: Unknown access key should be rejected
|
||||
t.Run("Unknown access key", func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, "http://localhost:8333/", nil)
|
||||
r.Host = "localhost:8333"
|
||||
err := signRawHTTPRequest(context.Background(), r, "unknown_key", "any_secret", "us-east-1")
|
||||
require.NoError(t, err)
|
||||
|
||||
identity, errCode := iam.authRequest(r, s3_constants.ACTION_LIST)
|
||||
assert.Equal(t, s3err.ErrInvalidAccessKeyID, errCode, "Should be denied with unknown access key")
|
||||
assert.Nil(t, identity)
|
||||
})
|
||||
|
||||
t.Run("Positive test case: properly signed credentials", func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, "http://localhost:8333/", nil)
|
||||
r.Host = "localhost:8333"
|
||||
err := signRawHTTPRequest(context.Background(), r, "readonly_access_key", "readonly_secret_key", "us-east-1")
|
||||
require.NoError(t, err)
|
||||
|
||||
identity, errCode := iam.authRequest(r, s3_constants.ACTION_LIST)
|
||||
assert.Equal(t, s3err.ErrNone, errCode)
|
||||
require.NotNil(t, identity)
|
||||
assert.Equal(t, "read_only_user", identity.Name)
|
||||
})
|
||||
|
||||
t.Run("Nil identity tests for guards", func(t *testing.T) {
|
||||
var nilIdentity *Identity
|
||||
// Test isAdmin guard
|
||||
assert.False(t, nilIdentity.isAdmin())
|
||||
// Test canDo guard
|
||||
assert.False(t, nilIdentity.canDo(s3_constants.ACTION_LIST, "bucket", "object"))
|
||||
})
|
||||
|
||||
t.Run("AuthSignatureOnly path", func(t *testing.T) {
|
||||
// Valid request
|
||||
r := httptest.NewRequest(http.MethodGet, "http://localhost:8333/", nil)
|
||||
r.Host = "localhost:8333"
|
||||
err := signRawHTTPRequest(context.Background(), r, "xx_access_key", "xx_secret_key", "us-east-1")
|
||||
require.NoError(t, err)
|
||||
|
||||
identity, errCode := iam.AuthSignatureOnly(r)
|
||||
assert.Equal(t, s3err.ErrNone, errCode)
|
||||
require.NotNil(t, identity)
|
||||
assert.Equal(t, "xx", identity.Name)
|
||||
|
||||
// Invalid request (wrong signature)
|
||||
r2 := httptest.NewRequest(http.MethodGet, "http://localhost:8333/", nil)
|
||||
r2.Host = "localhost:8333"
|
||||
err = signRawHTTPRequest(context.Background(), r2, "xx_access_key", "this_is_a_wrong_secret", "us-east-1")
|
||||
require.NoError(t, err)
|
||||
|
||||
_, errCode2 := iam.AuthSignatureOnly(r2)
|
||||
assert.Equal(t, s3err.ErrSignatureDoesNotMatch, errCode2)
|
||||
|
||||
// Verify fix: Streaming unsigned payload should be denied without auth header in AuthSignatureOnly
|
||||
r3 := httptest.NewRequest(http.MethodPut, "http://localhost:8333/somebucket/someobject", nil)
|
||||
r3.Header.Set("x-amz-content-sha256", "STREAMING-UNSIGNED-PAYLOAD-TRAILER")
|
||||
// No Authorization header
|
||||
_, errCode3 := iam.AuthSignatureOnly(r3)
|
||||
assert.Equal(t, s3err.ErrAccessDenied, errCode3, "AuthSignatureOnly should be denied with unsigned streaming if no auth header")
|
||||
})
|
||||
|
||||
t.Run("Wrong secret key", func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, "http://localhost:8333/", nil)
|
||||
r.Host = "localhost:8333"
|
||||
err := signRawHTTPRequest(context.Background(), r, "readonly_access_key", "this_is_a_wrong_secret", "us-east-1")
|
||||
require.NoError(t, err)
|
||||
|
||||
identity, errCode := iam.authRequest(r, s3_constants.ACTION_LIST)
|
||||
assert.Equal(t, s3err.ErrSignatureDoesNotMatch, errCode, "Should NOT be allowed with wrong signature")
|
||||
assert.Nil(t, identity)
|
||||
})
|
||||
|
||||
t.Run("Anonymous request to protected bucket", func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, "http://localhost:8333/somebucket/", nil)
|
||||
// No Authorization header
|
||||
|
||||
identity, errCode := iam.authRequest(r, s3_constants.ACTION_LIST)
|
||||
assert.Equal(t, s3err.ErrAccessDenied, errCode, "Should be denied for anonymous")
|
||||
assert.Nil(t, identity)
|
||||
})
|
||||
|
||||
t.Run("Non-S3 request should be denied", func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, "http://localhost:8333/", nil)
|
||||
// No headers at all
|
||||
|
||||
identity, errCode := iam.authRequest(r, s3_constants.ACTION_LIST)
|
||||
assert.Equal(t, s3err.ErrAccessDenied, errCode)
|
||||
assert.Nil(t, identity)
|
||||
})
|
||||
t.Run("Any other credentials", func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodGet, "http://localhost:8333/", nil)
|
||||
r.Host = "localhost:8333"
|
||||
err := signRawHTTPRequest(context.Background(), r, "some_other_key", "some_secret", "us-east-1")
|
||||
require.NoError(t, err)
|
||||
|
||||
identity, errCode := iam.authRequest(r, s3_constants.ACTION_LIST)
|
||||
assert.Equal(t, s3err.ErrInvalidAccessKeyID, errCode, "Should NOT be allowed with ANY other credentials")
|
||||
assert.Nil(t, identity)
|
||||
})
|
||||
t.Run("Streaming unsigned payload bypass attempt", func(t *testing.T) {
|
||||
r := httptest.NewRequest(http.MethodPut, "http://localhost:8333/somebucket/someobject", nil)
|
||||
r.Header.Set("x-amz-content-sha256", "STREAMING-UNSIGNED-PAYLOAD-TRAILER")
|
||||
// No Authorization header
|
||||
|
||||
identity, errCode := iam.authRequest(r, s3_constants.ACTION_WRITE)
|
||||
assert.Equal(t, s3err.ErrAccessDenied, errCode, "Should be denied with unsigned streaming if no auth header")
|
||||
assert.Nil(t, identity)
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user