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

appsec: differentiate user login and user set event #2956

Merged
merged 1 commit into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 8 additions & 5 deletions appsec/appsec.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ func MonitorParsedHTTPBody(ctx context.Context, body any) error {
// APM tracer middleware on use according to your blocking configuration.
// This function always returns nil when appsec is disabled and doesn't block users.
func SetUser(ctx context.Context, id string, opts ...tracer.UserMonitoringOption) error {
return setUser(ctx, id, usersec.UserSet, opts)
}

func setUser(ctx context.Context, id string, userEventType usersec.UserEventType, opts []tracer.UserMonitoringOption) error {
s, ok := tracer.SpanFromContext(ctx)
if !ok {
log.Debug("appsec: could not retrieve span from context. User ID tag won't be set")
Expand All @@ -61,11 +65,10 @@ func SetUser(ctx context.Context, id string, opts ...tracer.UserMonitoringOption
return nil
}

op, errPtr := usersec.StartUserLoginOperation(ctx, usersec.UserLoginOperationArgs{})
op, errPtr := usersec.StartUserLoginOperation(ctx, userEventType, usersec.UserLoginOperationArgs{})
Copy link
Contributor

Choose a reason for hiding this comment

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

appsec.SetUser doing a fake user login event is a bad design idea.

generally speaking, all those fake operations are coming from the time we didn't have EmitData and I believe they should be replaced by that to simplify everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really a "fake" user login operation, it's just badly named so I renamed it.

We can't use EmitData yet because we are missing context on the listener side of things but it could also be useful if we want to do some autoinstrumentation on auth frameworks later.

op.Finish(usersec.UserLoginOperationRes{
UserID: id,
SessionID: getSessionID(opts...),
Success: true,
})

return *errPtr
Expand All @@ -85,7 +88,7 @@ func SetUser(ctx context.Context, id string, opts ...tracer.UserMonitoringOption
// associated to them.
func TrackUserLoginSuccessEvent(ctx context.Context, uid string, md map[string]string, opts ...tracer.UserMonitoringOption) error {
TrackCustomEvent(ctx, "users.login.success", md)
return SetUser(ctx, uid, opts...)
return setUser(ctx, uid, usersec.UserLoginSuccess, opts)
}

// TrackUserLoginFailureEvent sets a failed user login event, with the given
Expand All @@ -111,8 +114,8 @@ func TrackUserLoginFailureEvent(ctx context.Context, uid string, exists bool, md

TrackCustomEvent(ctx, "users.login.failure", md)

op, _ := usersec.StartUserLoginOperation(ctx, usersec.UserLoginOperationArgs{})
op.Finish(usersec.UserLoginOperationRes{UserID: uid, Success: false})
op, _ := usersec.StartUserLoginOperation(ctx, usersec.UserLoginFailure, usersec.UserLoginOperationArgs{})
op.Finish(usersec.UserLoginOperationRes{UserID: uid})
}

// TrackCustomEvent sets a custom event as service entry span tags. This span is
Expand Down
34 changes: 28 additions & 6 deletions internal/appsec/emitter/usersec/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,58 @@ package usersec

import (
"context"
"sync"

"gopkg.in/DataDog/dd-trace-go.v1/appsec/events"
"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo"
"gopkg.in/DataDog/dd-trace-go.v1/internal/log"
)

const errorLog = `
appsec: user login monitoring ignored: could not find the http handler instrumentation metadata in the request context:
the request handler is not being monitored by a middleware function or the provided context is not the expected request context
the request handler is not being monitored by a middleware function or the provided context is not the expected request context.
If the user has been blocked using remote rules, blocking will still be enforced but it will not be reported.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand it.
Remember this log is mostly there for customer support cases, and not meant to be understood by the users themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you are right, I removed it

`

var errorLogOnce sync.Once

type (
// UserEventType is the type of user event, such as a successful login or a failed login or any other authenticated request.
UserEventType int

// UserLoginOperation type representing a call to appsec.SetUser(). It gets both created and destroyed in a single
// call to ExecuteUserIDOperation
UserLoginOperation struct {
dyngo.Operation
EventType UserEventType
}
// UserLoginOperationArgs is the user ID operation arguments.
UserLoginOperationArgs struct{}
UserLoginOperationArgs struct {
}

// UserLoginOperationRes is the user ID operation results.
UserLoginOperationRes struct {
UserID string
SessionID string
Success bool
}
)

func StartUserLoginOperation(ctx context.Context, args UserLoginOperationArgs) (*UserLoginOperation, *error) {
parent, _ := dyngo.FromContext(ctx)
op := &UserLoginOperation{Operation: dyngo.NewOperation(parent)}
const (
// UserLoginSuccess is the event type for a successful user login, when a new session or JWT is created.
UserLoginSuccess UserEventType = iota
// UserLoginFailure is the event type for a failed user login, when the user ID is not found or the password is incorrect.
UserLoginFailure
// UserSet is the event type for a user ID operation that is not a login, such as any authenticated request made by the user.
UserSet
)

func StartUserLoginOperation(ctx context.Context, eventType UserEventType, args UserLoginOperationArgs) (*UserLoginOperation, *error) {
parent, ok := dyngo.FromContext(ctx)
if !ok { // Nothing will be reported in this case, but we can still block so we don't return
errorLogOnce.Do(func() { log.Error(errorLog) })
}

op := &UserLoginOperation{Operation: dyngo.NewOperation(parent), EventType: eventType}
var err error
dyngo.OnData(op, func(e *events.BlockingSecurityEvent) { err = e })
dyngo.StartOperation(op, args)
Expand Down
21 changes: 13 additions & 8 deletions internal/appsec/listener/usersec/usec.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,19 @@ func NewUserSecFeature(cfg *config.Config, rootOp dyngo.Operation) (listener.Fea
}

func (*Feature) OnFinish(op *usersec.UserLoginOperation, res usersec.UserLoginOperationRes) {
builder := addresses.NewAddressesBuilder().
WithUserID(res.UserID).
WithUserSessionID(res.SessionID)

if res.Success {
builder = builder.WithUserLoginSuccess()
} else {
builder = builder.WithUserLoginFailure()
builder := addresses.NewAddressesBuilder()

switch op.EventType {
case usersec.UserLoginSuccess:
builder = builder.WithUserLoginSuccess().
WithUserID(res.UserID).
WithUserSessionID(res.SessionID)
case usersec.UserLoginFailure:
builder = builder.WithUserLoginFailure().
WithUserID(res.UserID)
case usersec.UserSet:
builder = builder.WithUserID(res.UserID).
WithUserSessionID(res.SessionID)
}

dyngo.EmitData(op, waf.RunEvent{
Expand Down
Loading