opensource/teleport
3
0
Fork 0
mirror of https://github.com/gravitational/teleport.git synced 2026-09-19 01:58:44 +08:00
Code Issues Packages Projects Releases Wiki Activity

access_monitoring_rules: Improve access request rule validation (#54393)

Browse Source 
* Improve  access request rule validation

* Allow desired state to be empty

* Use valid condition in test

* Godoc

* Update test conditions
This commit is contained in:
Bernard Kim
2025-05-13 17:18:15 +00:00
committed by GitHub
parent bde53bc267
commit f6621d40c9
5 changed files with 87 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/accessmonitoring/request_mapping.go
+6
View File
@@ -55,6 +55,12 @@ func parseAccessRequestExpression(expr string) (accessRequestExpression, error)
return parsedExpr, nil
}
// NewAccessRequestConditionParser returns a new parser for access request
// condition expressions.
func NewAccessRequestConditionParser() (*typical.Parser[AccessRequestExpressionEnv, any], error) {
return newRequestConditionParser()
}
func newRequestConditionParser() (*typical.Parser[AccessRequestExpressionEnv, any], error) {
typicalEnvVar := map[string]typical.Variable{
"true": true,
lib/cache/access_monitoring_rule_test.go
Vendored
+5 -5
View File
@@ -81,7 +81,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
Notification: &accessmonitoringrulesv1.Notification{
Name: "notificationIntegration",
},
@@ -103,7 +103,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
AutomaticReview: &accessmonitoringrulesv1.AutomaticReview{
Integration: "automaticReviewIntegration",
Decision: types.RequestState_APPROVED.String(),
@@ -126,7 +126,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
Notification: &accessmonitoringrulesv1.Notification{
Name: "notificationIntegration",
},
@@ -153,7 +153,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
Notification: &accessmonitoringrulesv1.Notification{
Name: "notificationIntegration",
},
@@ -179,7 +179,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
AutomaticReview: &accessmonitoringrulesv1.AutomaticReview{
Integration: types.BuiltInAutomaticReview,
Decision: types.RequestState_APPROVED.String(),
lib/services/access_monitoring_rules.go
+34 -1
View File
@@ -28,6 +28,14 @@ import (
accessmonitoringrulesv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accessmonitoringrules/v1"
headerv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/header/v1"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/accessmonitoring"
"github.com/gravitational/teleport/lib/utils/typical"
)
var (
// accessRequestConditionParser is a parser for the access request condition.
// It is used to validate access monitoring rules before write operations.
accessRequestConditionParser = mustNewAccessRequestConditionParser()
)
// AccessMonitoringRules is the AccessMonitoringRule service
@@ -93,12 +101,29 @@ func ValidateAccessMonitoringRule(accessMonitoringRule *accessmonitoringrulesv1.
if automaticReview.GetIntegration() == "" {
return trace.BadParameter("accessMonitoringRule automatic_review integration is missing")
}
if automaticReview.GetDecision() == "" {
switch automaticReview.GetDecision() {
case types.RequestState_APPROVED.String(), types.RequestState_DENIED.String():
case "":
return trace.BadParameter("accessMonitoringRule automatic_review decision is missing")
default:
return trace.BadParameter("accessMonitoringRule automatic_review decision %q is not supported", automaticReview.GetDecision())
}
}
if slices.Contains(accessMonitoringRule.GetSpec().GetSubjects(), types.KindAccessRequest) {
_, err := accessRequestConditionParser.Parse(accessMonitoringRule.GetSpec().GetCondition())
if err != nil {
return trace.BadParameter("accessMonitoringRule condition is invalid: %s", err.Error())
}
desiredState := accessMonitoringRule.GetSpec().GetDesiredState()
switch desiredState {
case "", types.AccessMonitoringRuleStateReviewed:
default:
return trace.BadParameter("accessMonitoringRule desired_state %q is not supported", desiredState)
}
if accessMonitoringRule.GetSpec().GetNotification() != nil {
return nil
}
@@ -145,3 +170,11 @@ func MatchAccessMonitoringRule(rule *accessmonitoringrulesv1.AccessMonitoringRul
}
return true
}
func mustNewAccessRequestConditionParser() *typical.Parser[accessmonitoring.AccessRequestExpressionEnv, any] {
parser, err := accessmonitoring.NewAccessRequestConditionParser()
if err != nil {
panic(err)
}
return parser
}
lib/services/access_monitoring_rules_test.go
+37 -2
View File
@@ -90,6 +90,40 @@ func TestValidateAccessMonitoringRule(t *testing.T) {
},
assertErr: require.NoError,
},
{
description: "invalid automatic_review decision",
modifyAMR: func(amr *accessmonitoringrulesv1.AccessMonitoringRule) {
amr.Spec.AutomaticReview.Decision = "invalid-decision"
},
assertErr: func(t require.TestingT, err error, i ...interface{}) {
require.ErrorContains(t, err, `accessMonitoringRule automatic_review decision "invalid-decision" is not supported`)
},
},
{
description: "invalid desired_state",
modifyAMR: func(amr *accessmonitoringrulesv1.AccessMonitoringRule) {
amr.Spec.DesiredState = "invalid-desired-state"
},
assertErr: func(t require.TestingT, err error, i ...interface{}) {
require.ErrorContains(t, err, `accessMonitoringRule desired_state "invalid-desired-state" is not supported`)
},
},
{
description: "invalid condition",
modifyAMR: func(amr *accessmonitoringrulesv1.AccessMonitoringRule) {
amr.Spec.Condition = "invalid-condition"
},
assertErr: func(t require.TestingT, err error, i ...interface{}) {
require.ErrorContains(t, err, "accessMonitoringRule condition is invalid")
},
},
{
description: "allow desired_state to be empty",
modifyAMR: func(amr *accessmonitoringrulesv1.AccessMonitoringRule) {
amr.Spec.DesiredState = ""
},
assertErr: require.NoError,
},
}
validAMR := &accessmonitoringrulesv1.AccessMonitoringRule{
@@ -97,8 +131,9 @@ func TestValidateAccessMonitoringRule(t *testing.T) {
Metadata: &headerv1.Metadata{},
Version: types.V1,
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "true",
Subjects: []string{types.KindAccessRequest},
Condition: "true",
DesiredState: types.AccessMonitoringRuleStateReviewed,
Notification: &accessmonitoringrulesv1.Notification{
Name: "fakePlugin",
},
lib/services/local/access_monitoring_rules_test.go
+5 -5
View File
@@ -132,7 +132,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
Notification: &accessmonitoringrulesv1.Notification{
Name: "notificationIntegration",
},
@@ -154,7 +154,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
AutomaticReview: &accessmonitoringrulesv1.AutomaticReview{
Integration: "automaticReviewIntegration",
Decision: types.RequestState_APPROVED.String(),
@@ -177,7 +177,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
Notification: &accessmonitoringrulesv1.Notification{
Name: "notificationIntegration",
},
@@ -204,7 +204,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
Notification: &accessmonitoringrulesv1.Notification{
Name: "notificationIntegration",
},
@@ -230,7 +230,7 @@ func TestListAccessMonitoringRulesWithFilter(t *testing.T) {
},
Spec: &accessmonitoringrulesv1.AccessMonitoringRuleSpec{
Subjects: []string{types.KindAccessRequest},
Condition: "someCondition",
Condition: "true",
AutomaticReview: &accessmonitoringrulesv1.AutomaticReview{
Integration: types.BuiltInAutomaticReview,
Decision: types.RequestState_APPROVED.String(),