Skip to content

[persistent collections] based on PR-866 #1261

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

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
cb307db
first persistence draft
jptosso Aug 14, 2023
27fd972
implement interfaces
jptosso Aug 14, 2023
2e0507b
full implementation
jptosso Aug 14, 2023
12af002
force init in tests
jptosso Aug 14, 2023
08a1fc5
minor tweaks
jptosso Aug 14, 2023
846cdc3
ci fixes
jptosso Aug 14, 2023
22c596d
regex implementation and tests
jptosso Aug 14, 2023
4eb78c3
fix race condition
jptosso Aug 14, 2023
d4e3d46
fix test and cycle dependencies issue
Dec 25, 2024
3fa1eaf
remove nil defererence in test
Dec 25, 2024
73d7efe
remove nil pointer dereference in test
Dec 25, 2024
34be2c4
add WithPersistenceEngine config option
Dec 26, 2024
82b276b
rename example
Dec 26, 2024
aab978c
add logic for default and custom persistence engine
Dec 27, 2024
7a5729e
remove nolint comments
Dec 27, 2024
240674a
polishing customttl
Dec 27, 2024
f0c6472
simplify default engine ttl set and expirevar
Dec 30, 2024
a68bc7a
polishing examples
Dec 30, 2024
aa67d52
checks in default pe
Dec 30, 2024
992f416
Merge branch 'main' into pr-866
tty2 Dec 30, 2024
09d9c66
readme
Dec 30, 2024
4f55210
examples tests
Dec 30, 2024
09261ba
expirevar tests
Dec 30, 2024
c7b3a59
default engine SetTTL test
tty2 Dec 31, 2024
cf8554a
add directiveSecPersistenceEngine tests
tty2 Dec 31, 2024
f5ed9bc
Merge branch 'main' into pr-866
tty2 Dec 31, 2024
baf203e
implement waf.Close method, based on the PR #1200
tty2 Dec 31, 2024
a3866e4
change user id comment
tty2 Jan 3, 2025
16e2144
resolve conflicts
tty2 Jan 3, 2025
40fffb0
spellcheck
tty2 Jan 3, 2025
c400f8a
Merge branch 'main' into pr-866
tty2 Jan 7, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions collection/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,24 +43,45 @@ type Keyed interface {
FindString(key string) []types.MatchData
}

type Editable interface {
Keyed

// Remove deletes the key from the CollectionMap
Remove(key string)

// Set will replace the key's value with this slice
Set(key string, values []string)

// TODO: in v4 this should contain setters for Map and Persistence
}

// Map are used to store VARIABLE data
// for transactions, this data structured is designed
// to store slices of data for keys
// Important: CollectionMaps ARE NOT concurrent safe
type Map interface {
Keyed
Editable

// Add a value to some key
Add(key string, value string)

// Set will replace the key's value with this slice
Set(key string, values []string)

// SetIndex will place the value under the index
// If the index is higher than the current size of the CollectionMap
// it will be appended
SetIndex(key string, index int, value string)
}

// Remove deletes the key from the CollectionMap
Remove(key string)
// Persistent collections won't use arrays as values
// They are designed for collections that will be stored
type Persistent interface {
Editable

// Initializes the input as the collection key
Init(key string)

// Sum will add the value to the key
Sum(key string, sum int)

// SetOne will replace the key's value with this string
SetOne(key string, value string)
}
2 changes: 1 addition & 1 deletion examples/http-server/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ require (
github.com/tidwall/gjson v1.18.0 // indirect
github.com/tidwall/match v1.1.1 // indirect
github.com/tidwall/pretty v1.2.1 // indirect
golang.org/x/net v0.32.0 // indirect
golang.org/x/net v0.33.0 // indirect
golang.org/x/sync v0.10.0 // indirect
golang.org/x/tools v0.22.0 // indirect
rsc.io/binaryregexp v0.2.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions examples/http-server/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ github.com/tidwall/pretty v1.2.1 h1:qjsOFOWWQl+N3RsoF5/ssm1pHmJJwhjlSbZ51I6wMl4=
github.com/tidwall/pretty v1.2.1/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
golang.org/x/mod v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
golang.org/x/mod v0.18.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
golang.org/x/net v0.32.0 h1:ZqPmj8Kzc+Y6e0+skZsuACbx+wzMgo5MQsJh9Qd6aYI=
golang.org/x/net v0.32.0/go.mod h1:CwU0IoeOlnQQWJ6ioyFrfRuomB8GKF6KbYXZVyeXNfs=
golang.org/x/net v0.33.0 h1:74SYHlV8BIgHIFC/LrYkOGIwL19eTYXQ5wc6TBuO36I=
golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4=
golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ=
golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
golang.org/x/sys v0.28.0 h1:Fksou7UEQUWlKvIdsqzJmUmCX3cZuD2+P3XyyzwMhlA=
Expand Down
14 changes: 14 additions & 0 deletions experimental/plugins/persistence.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors
// SPDX-License-Identifier: Apache-2.0

package plugins

import (
"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
"github.com/corazawaf/coraza/v3/internal/persistence"
)

// RegisterPersistenceEngine registers a new persistence engine
func RegisterPersistenceEngine(name string, engine plugintypes.PersistenceEngine) {
persistence.Register(name, engine)
}
15 changes: 15 additions & 0 deletions experimental/plugins/plugintypes/persistence.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors
// SPDX-License-Identifier: Apache-2.0

package plugintypes

type PersistenceEngine interface {
Open(uri string, ttl int) error
Close() error
Sum(collectionName string, collectionKey string, key string, sum int) error
Get(collectionName string, collectionKey string, key string) (string, error)

All(collectionName string, collectionKey string) (map[string]string, error)
Set(collection string, collectionKey string, key string, value string) error
Remove(collection string, collectionKey string, key string) error
}
6 changes: 6 additions & 0 deletions experimental/plugins/plugintypes/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,10 @@ type TransactionVariables interface {
ArgsGetNames() collection.Collection
ArgsPostNames() collection.Collection
MultipartStrictError() collection.Single
// TODO(v4: Add these)
// Session() collection.Persistent
// User() collection.Persistent
// IP() collection.Persistent
// Global() collection.Persistent
// Resource() collection.Persistent
}
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg=
golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk=
golang.org/x/net v0.17.0/go.mod h1:NxSsAGuq816PNPmqtQdLE42eU2Fs7NoRIZrHJAlaCOE=
golang.org/x/net v0.18.0/go.mod h1:/czyP5RqHAH4odGYxBJ1qz0+CE5WZ+2j1YgoEo8F2jQ=
golang.org/x/net v0.32.0 h1:ZqPmj8Kzc+Y6e0+skZsuACbx+wzMgo5MQsJh9Qd6aYI=
golang.org/x/net v0.32.0/go.mod h1:CwU0IoeOlnQQWJ6ioyFrfRuomB8GKF6KbYXZVyeXNfs=
golang.org/x/net v0.33.0 h1:74SYHlV8BIgHIFC/LrYkOGIwL19eTYXQ5wc6TBuO36I=
golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
Expand Down
60 changes: 32 additions & 28 deletions internal/actions/initcol.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@
package actions

import (
"fmt"
"strings"

"github.com/corazawaf/coraza/v3/experimental/plugins/macro"
"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
"github.com/corazawaf/coraza/v3/internal/collections"
utils "github.com/corazawaf/coraza/v3/internal/strings"
"github.com/corazawaf/coraza/v3/types/variables"
)

// Action Group: Non-disruptive
Expand All @@ -23,9 +28,8 @@ import (
// SecAction "phase:1,id:116,nolog,pass,initcol:ip=%{REMOTE_ADDR}"
// ```
type initcolFn struct {
collection string
variable byte
key string
collection variables.RuleVariable
key macro.Macro
}

func (a *initcolFn) Init(_ plugintypes.RuleMetadata, data string) error {
Expand All @@ -34,34 +38,34 @@ func (a *initcolFn) Init(_ plugintypes.RuleMetadata, data string) error {
return ErrInvalidKVArguments
}

a.collection = col
a.key = key
a.variable = 0x0
c, err := variables.Parse(col)
if err != nil {
return fmt.Errorf("initcol: collection %s is not valid", col)
}
// we validate if this is a persistent collection
persistent := []string{"USER", "SESSION", "IP", "RESOURCE", "GLOBAL"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to restrict arbitrary collection creation?

if !utils.InSlice(c.Name(), persistent) {
return fmt.Errorf("initcol: collection %s is not persistent", c.Name())
}
a.collection = c
mkey, err := macro.NewMacro(key)
if err != nil {
return err
}
a.key = mkey
return nil
}

func (a *initcolFn) Evaluate(_ plugintypes.RuleMetadata, _ plugintypes.TransactionState) {
// tx.DebugLogger().Error().Msg("initcol was used but it's not supported", zap.Int("rule", r.Id))
/*
key := tx.MacroExpansion(a.key)
data := tx.WAF.Persistence.Get(a.variable, key)
if data == nil {
ts := time.Now().UnixNano()
tss := strconv.FormatInt(ts, 10)
tsstimeout := strconv.FormatInt(ts+(int64(tx.WAF.CollectionTimeout)*1000), 10)
data = map[string][]string{
"CREATE_TIME": {tss},
"IS_NEW": {"1"},
"KEY": {key},
"LAST_UPDATE_TIME": {tss},
"TIMEOUT": {tsstimeout},
"UPDATE_COUNTER": {"0"},
"UPDATE_RATE": {"0"},
}
}
tx.GetCollection(a.variable).SetData(data)
tx.PersistentCollections[a.variable] = key
*/
func (a *initcolFn) Evaluate(_ plugintypes.RuleMetadata, txs plugintypes.TransactionState) {
col := txs.Collection(a.collection)
key := a.key.Expand(txs)
txs.DebugLogger().Debug().Str("collection", a.collection.Name()).Str("key", key).Msg("initcol: initializing collection")
c, ok := col.(*collections.Persistent)
if !ok {
txs.DebugLogger().Error().Str("collection", a.collection.Name()).Msg("initcol: collection is not a persistent collection")
return
}
c.Init(key)
}

func (a *initcolFn) Type() plugintypes.ActionType {
Expand Down
31 changes: 20 additions & 11 deletions internal/actions/initcol_test.go
Original file line number Diff line number Diff line change
@@ -1,24 +1,33 @@
// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors
// SPDX-License-Identifier: Apache-2.0

package actions
package actions_test

import "testing"
import (
"testing"

"github.com/corazawaf/coraza/v3"
"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
"github.com/corazawaf/coraza/v3/internal/actions"
)

func TestInitcolInit(t *testing.T) {
waf, _ := coraza.NewWAF(coraza.NewWAFConfig()) //nolint:errcheck
initcol, err := actions.Get("initcol")
if err != nil {
t.Error(err)
}
t.Run("invalid argument", func(t *testing.T) {
initcol := initcol()
err := initcol.Init(nil, "foo")
if err == nil {
t.Errorf("expected error")
if err := initcol.Init(&md{}, "test"); err == nil {
t.Error("expected error")
}
})

t.Run("passing argument", func(t *testing.T) {
initcol := initcol()
err := initcol.Init(nil, "foo=bar")
if err != nil {
t.Errorf("unexpected error: %s", err.Error())
t.Run("editable variable", func(t *testing.T) {
if err := initcol.Init(&md{}, "session=abcdef"); err != nil {
t.Error(err)
}
txs := waf.NewTransaction().(plugintypes.TransactionState)
initcol.Evaluate(&md{}, txs)
})
}
2 changes: 1 addition & 1 deletion internal/actions/maturity_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestMaturityInit(t *testing.T) {
err := a.Init(r, test.data)
if test.expectedError {
if err == nil {
t.Errorf("expected error: %s", err.Error())
t.Error("error expected, got nil")
}
} else {
if err != nil {
Expand Down
21 changes: 11 additions & 10 deletions internal/actions/setvar.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"strconv"
"strings"

utils "github.com/corazawaf/coraza/v3/internal/strings"

"github.com/corazawaf/coraza/v3/collection"
"github.com/corazawaf/coraza/v3/experimental/plugins/macro"
"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
Expand Down Expand Up @@ -76,8 +78,10 @@ func (a *setvarFn) Init(_ plugintypes.RuleMetadata, data string) error {
colKey, colVal, colOk := strings.Cut(key, ".")
// Right not it only makes sense to allow setting TX
// key is also required
if strings.ToUpper(colKey) != "TX" {
return errors.New("invalid arguments, expected collection TX")
available := []string{"TX", "USER", "GLOBAL", "RESOURCE", "SESSION", "IP"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jptosso Clould you help me here? It's your changes, I believe you have more context here.

I personally see the reason of making constraints, just to minimize unpredictable behavior.
And in the future, if we really need we can extend it without breaking compatibility.
On the other hand if we make it possible to pass anything now and bring constrains after, there is a chance to break code for someone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! setvar can only be used for this set of variables. Others are not mutuable, but IMOwe can use type assertion for this

// we validate uppercase colKey is one of available
if !utils.InSlice(strings.ToUpper(colKey), available) {
return errors.New("setvar: invalid editable collection, available collections are: " + strings.Join(available, ", "))
}
if strings.TrimSpace(colVal) == "" {
return errors.New("invalid arguments, expected syntax TX.{key}={value}")
Expand Down Expand Up @@ -111,7 +115,7 @@ func (a *setvarFn) Evaluate(r plugintypes.RuleMetadata, tx plugintypes.Transacti
Str("var_key", key).
Str("var_value", value).
Int("rule_id", r.ID()).
Msg("Action evaluated")
Msg("Action SetVar evaluated")
a.evaluateTxCollection(r, tx, strings.ToLower(key), value)
}

Expand All @@ -120,17 +124,14 @@ func (a *setvarFn) Type() plugintypes.ActionType {
}

func (a *setvarFn) evaluateTxCollection(r plugintypes.RuleMetadata, tx plugintypes.TransactionState, key string, value string) {
var col collection.Map
if c, ok := tx.Collection(a.collection).(collection.Map); !ok {
tx.DebugLogger().Error().Msg("collection in setvar is not a map")
// TODO for api breaking issues, we have to split this function in Map and Persistent
var col collection.Editable
if c, ok := tx.Collection(a.collection).(collection.Editable); !ok {
tx.DebugLogger().Error().Msg("collection in setvar is not editable")
return
} else {
col = c
}
if col == nil {
tx.DebugLogger().Error().Msg("collection in setvar is nil")
return
}

if a.isRemove {
col.Remove(key)
Expand Down
50 changes: 50 additions & 0 deletions internal/actions/setvar_persistence_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors
// Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors

Maybe even 2025.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. These changes made in 2023 by Juan.
I'd keep it as it is.

// SPDX-License-Identifier: Apache-2.0

//go:build !tinygo
// +build !tinygo

package actions_test

import (
"testing"

"github.com/corazawaf/coraza/v3"
"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
"github.com/corazawaf/coraza/v3/internal/actions"
"github.com/corazawaf/coraza/v3/types/variables"
)

type md struct {
}

func (md) ID() int {
return 0
}
func (md) ParentID() int {
return 0
}
func (md) Status() int {
return 0
}

func TestPersistenceSetvar(t *testing.T) {
a, err := actions.Get("setvar")
if err != nil {
t.Error("failed to get setvar action")
}
waf, err := coraza.NewWAF(coraza.NewWAFConfig().WithDirectives("SecPersistenceEngine default"))
if err != nil {
t.Fatal(err)
}
t.Run("SESSION should be set", func(t *testing.T) {
if err := a.Init(&md{}, "SESSION.test=test"); err != nil {
t.Errorf("unexpected error: %v", err)
}
tx := waf.NewTransaction()
txs := tx.(plugintypes.TransactionState)
a.Evaluate(&md{}, txs)
col := txs.Collection(variables.Session)
col.FindAll()
})
}
Loading