From 63ecf63a089b74c9d2edeeb4a684cb51bcffb824 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Mon, 30 Oct 2023 16:34:53 +0300 Subject: [PATCH] [#11] iam: Support 'NotPrincipal', 'NotAction', 'NotResource' Signed-off-by: Denis Kirillov --- iam/converter.go | 36 +++--- iam/converter_test.go | 76 ++++++++---- iam/policy.go | 161 ++++++++++++++++++++++++-- iam/policy_test.go | 261 +++++++++++++++++++++++++++++++++++++++++- inmemory_test.go | 32 +++--- 5 files changed, 499 insertions(+), 67 deletions(-) diff --git a/iam/converter.go b/iam/converter.go index 637bf35..d6642f3 100644 --- a/iam/converter.go +++ b/iam/converter.go @@ -1,7 +1,6 @@ package iam import ( - "errors" "fmt" "strconv" "strings" @@ -11,8 +10,6 @@ import ( ) const ( - FrostFSPrincipal = "FrostFS" - RequestOwnerProperty = "Owner" ) @@ -56,6 +53,10 @@ const ( ) func (p Policy) ToChain() (*policyengine.Chain, error) { + if err := p.Validate(GeneralPolicyType); err != nil { + return nil, err + } + var chain policyengine.Chain for _, statement := range p.Statement { @@ -64,20 +65,21 @@ func (p Policy) ToChain() (*policyengine.Chain, error) { status = policyengine.Allow } - if len(statement.Principal) != 1 { - return nil, errors.New("currently supported exactly one principal type") - } - var principals []string var op policyengine.ConditionType - if _, ok := statement.Principal[Wildcard]; ok { + statementPrincipal, inverted := statement.principal() + if _, ok := statementPrincipal[Wildcard]; ok { // this can be true only if 'inverted' false principals = []string{Wildcard} op = policyengine.CondStringLike - } else if frostfsPrincipals, ok := statement.Principal[FrostFSPrincipal]; ok { - principals = frostfsPrincipals - op = policyengine.CondStringEquals } else { - return nil, errors.New("currently supported only FrostFS or all (wildcard) principals") + for _, principal := range statementPrincipal { + principals = append(principals, principal...) + } + + op = policyengine.CondStringEquals + if inverted { + op = policyengine.CondStringNotEquals + } } var conditions []policyengine.Condition @@ -96,10 +98,16 @@ func (p Policy) ToChain() (*policyengine.Chain, error) { } conditions = append(conditions, conds...) + action, actionInverted := statement.action() + ruleAction := policyengine.Actions{Inverted: actionInverted, Names: action} + + resource, resourceInverted := statement.resource() + ruleResource := policyengine.Resources{Inverted: resourceInverted, Names: resource} + r := policyengine.Rule{ Status: status, - Action: statement.Action, - Resource: statement.Resource, + Actions: ruleAction, + Resources: ruleResource, Any: true, Condition: conditions, } diff --git a/iam/converter_test.go b/iam/converter_test.go index 0407bad..3de9cb5 100644 --- a/iam/converter_test.go +++ b/iam/converter_test.go @@ -12,8 +12,8 @@ func TestConverters(t *testing.T) { p := Policy{ Version: "2012-10-17", Statement: []Statement{{ - Principal: map[string][]string{ - FrostFSPrincipal: {"arn:aws:iam::111122223333:user/JohnDoe"}, + Principal: map[PrincipalType][]string{ + AWSPrincipalType: {"arn:aws:iam::111122223333:user/JohnDoe"}, }, Effect: AllowEffect, Action: []string{"s3:PutObject"}, @@ -28,10 +28,10 @@ func TestConverters(t *testing.T) { expected := &policyengine.Chain{Rules: []policyengine.Rule{ { - Status: policyengine.Allow, - Action: p.Statement[0].Action, - Resource: p.Statement[0].Resource, - Any: true, + Status: policyengine.Allow, + Actions: policyengine.Actions{Names: p.Statement[0].Action}, + Resources: policyengine.Resources{Names: p.Statement[0].Resource}, + Any: true, Condition: []policyengine.Condition{ { Op: policyengine.CondStringEquals, @@ -54,12 +54,47 @@ func TestConverters(t *testing.T) { require.Equal(t, expected, chain) }) + t.Run("valid inverted policy", func(t *testing.T) { + p := Policy{ + Version: "2012-10-17", + Statement: []Statement{{ + NotPrincipal: map[PrincipalType][]string{ + AWSPrincipalType: {"arn:aws:iam::111122223333:user/JohnDoe"}, + }, + Effect: DenyEffect, + NotAction: []string{"s3:PutObject"}, + NotResource: []string{"arn:aws:s3:::DOC-EXAMPLE-BUCKET/*"}, + }}, + } + + expected := &policyengine.Chain{Rules: []policyengine.Rule{ + { + Status: policyengine.AccessDenied, + Actions: policyengine.Actions{Inverted: true, Names: p.Statement[0].NotAction}, + Resources: policyengine.Resources{Inverted: true, Names: p.Statement[0].NotResource}, + Any: true, + Condition: []policyengine.Condition{ + { + Op: policyengine.CondStringNotEquals, + Object: policyengine.ObjectRequest, + Key: RequestOwnerProperty, + Value: "arn:aws:iam::111122223333:user/JohnDoe", + }, + }, + }, + }} + + chain, err := p.ToChain() + require.NoError(t, err) + require.Equal(t, expected, chain) + }) + t.Run("invalid policy (unsupported principal type)", func(t *testing.T) { p := Policy{ Version: "2012-10-17", Statement: []Statement{{ - Principal: map[string][]string{ - "AWS": {"arn:aws:iam::111122223333:user/JohnDoe"}, + Principal: map[PrincipalType][]string{ + "dummy": {"arn:aws:iam::111122223333:user/JohnDoe"}, }, Effect: AllowEffect, Action: []string{"s3:PutObject"}, @@ -71,14 +106,15 @@ func TestConverters(t *testing.T) { require.Error(t, err) }) - t.Run("invalid policy (missing principal)", func(t *testing.T) { + t.Run("invalid policy (missing resource)", func(t *testing.T) { p := Policy{ Version: "2012-10-17", Statement: []Statement{{ - Principal: map[string][]string{}, - Effect: AllowEffect, - Action: []string{"s3:PutObject"}, - Resource: []string{"arn:aws:s3:::DOC-EXAMPLE-BUCKET/*"}, + Principal: map[PrincipalType][]string{ + AWSPrincipalType: {"arn:aws:iam::111122223333:user/JohnDoe"}, + }, + Effect: AllowEffect, + Action: []string{"s3:PutObject"}, }}, } @@ -90,7 +126,7 @@ func TestConverters(t *testing.T) { p := Policy{ Version: "2012-10-17", Statement: []Statement{{ - Principal: map[string][]string{"*": nil}, + Principal: map[PrincipalType][]string{Wildcard: nil}, Effect: AllowEffect, Action: []string{"s3:PutObject"}, Resource: []string{"arn:aws:s3:::DOC-EXAMPLE-BUCKET/*"}, @@ -120,10 +156,10 @@ func TestConverters(t *testing.T) { expected := &policyengine.Chain{Rules: []policyengine.Rule{ { - Status: policyengine.Allow, - Action: p.Statement[0].Action, - Resource: p.Statement[0].Resource, - Any: true, + Status: policyengine.Allow, + Actions: policyengine.Actions{Names: p.Statement[0].Action}, + Resources: policyengine.Resources{Names: p.Statement[0].Resource}, + Any: true, Condition: []policyengine.Condition{ { Op: policyengine.CondStringLike, @@ -260,9 +296,9 @@ func TestConverters(t *testing.T) { for i, rule := range chain.Rules { expectedRule := expected.Rules[i] - require.Equal(t, expectedRule.Action, rule.Action) + require.Equal(t, expectedRule.Actions, rule.Actions) require.Equal(t, expectedRule.Any, rule.Any) - require.Equal(t, expectedRule.Resource, rule.Resource) + require.Equal(t, expectedRule.Resources, rule.Resources) require.Equal(t, expectedRule.Status, rule.Status) require.ElementsMatch(t, expectedRule.Condition, rule.Condition) } diff --git a/iam/policy.go b/iam/policy.go index 852ce33..956b239 100644 --- a/iam/policy.go +++ b/iam/policy.go @@ -3,11 +3,11 @@ package iam import ( "encoding/json" "errors" + "fmt" ) type ( // Policy grammar https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_grammar.html - // Currently 'NotPrincipal', 'NotAction' and 'NotResource' are not supported (so cannot be unmarshalled). Policy struct { Version string `json:"Version,omitempty"` ID string `json:"Id,omitempty"` @@ -17,15 +17,19 @@ type ( Statements []Statement Statement struct { - SID string `json:"Sid,omitempty"` - Principal Principal `json:"Principal,omitempty"` - Effect Effect `json:"Effect"` - Action Action `json:"Action"` - Resource Resource `json:"Resource"` - Conditions Conditions `json:"Condition,omitempty"` + ID string `json:"Id,omitempty"` + SID string `json:"Sid,omitempty"` + Principal Principal `json:"Principal,omitempty"` + NotPrincipal Principal `json:"NotPrincipal,omitempty"` + Effect Effect `json:"Effect"` + Action Action `json:"Action,omitempty"` + NotAction Action `json:"NotAction,omitempty"` + Resource Resource `json:"Resource,omitempty"` + NotResource Resource `json:"NotResource,omitempty"` + Conditions Conditions `json:"Condition,omitempty"` } - Principal map[string][]string + Principal map[PrincipalType][]string Effect string @@ -36,6 +40,16 @@ type ( Conditions map[string]Condition Condition map[string][]string + + PolicyType int + + PrincipalType string +) + +const ( + GeneralPolicyType PolicyType = iota + IdentityBasedPolicyType + ResourceBasedPolicyType ) const Wildcard = "*" @@ -45,6 +59,22 @@ const ( DenyEffect Effect = "Deny" ) +func (e Effect) IsValid() bool { + return e == AllowEffect || e == DenyEffect +} + +const ( + AWSPrincipalType PrincipalType = "AWS" + FederatedPrincipalType PrincipalType = "Federated" + ServicePrincipalType PrincipalType = "Service" + CanonicalUserPrincipalType PrincipalType = "CanonicalUser" +) + +func (p PrincipalType) IsValid() bool { + return p == AWSPrincipalType || p == FederatedPrincipalType || + p == ServicePrincipalType || p == CanonicalUserPrincipalType +} + func (s *Statements) UnmarshalJSON(data []byte) error { var list []Statement if err := json.Unmarshal(data, &list); err == nil { @@ -75,7 +105,7 @@ func (p *Principal) UnmarshalJSON(data []byte) error { return nil } - m := make(map[string]interface{}) + m := make(map[PrincipalType]any) if err := json.Unmarshal(data, &m); err != nil { return err } @@ -87,7 +117,7 @@ func (p *Principal) UnmarshalJSON(data []byte) error { continue } - list, ok := val.([]interface{}) + list, ok := val.([]any) if !ok { return errors.New("invalid principal format") } @@ -144,7 +174,7 @@ func (r *Resource) UnmarshalJSON(data []byte) error { func (c *Condition) UnmarshalJSON(data []byte) error { *c = make(Condition) - m := make(map[string]interface{}) + m := make(map[string]any) if err := json.Unmarshal(data, &m); err != nil { return err } @@ -156,7 +186,7 @@ func (c *Condition) UnmarshalJSON(data []byte) error { continue } - list, ok := val.([]interface{}) + list, ok := val.([]any) if !ok { return errors.New("invalid principal format") } @@ -175,3 +205,110 @@ func (c *Condition) UnmarshalJSON(data []byte) error { return nil } + +func (p Policy) Validate(typ PolicyType) error { + if err := p.validate(); err != nil { + return err + } + + switch typ { + case IdentityBasedPolicyType: + return p.validateIdentityBased() + case ResourceBasedPolicyType: + return p.validateResourceBased() + default: + return nil + } +} + +func (p Policy) validate() error { + for _, statement := range p.Statement { + if !statement.Effect.IsValid() { + return fmt.Errorf("unknown effect: '%s'", statement.Effect) + } + if len(statement.Action) != 0 && len(statement.NotAction) != 0 { + return errors.New("'Actions' and 'NotAction' are mutually exclusive") + } + if statement.Resource != nil && statement.NotResource != nil { + return errors.New("'Resources' and 'NotResource' are mutually exclusive") + } + if len(statement.Resource) == 0 && len(statement.NotResource) == 0 { + return errors.New("one of 'Resources'/'NotResource' must be provided") + } + if len(statement.Principal) != 0 && len(statement.NotPrincipal) != 0 { + return errors.New("'Principal' and 'NotPrincipal' are mutually exclusive") + } + if len(statement.NotPrincipal) != 0 && statement.Effect != DenyEffect { + return errors.New("using 'NotPrincipal' with effect 'Allow' is not supported") + } + + principal, _ := statement.principal() + if err := principal.validate(); err != nil { + return err + } + } + + return nil +} + +func (p Policy) validateIdentityBased() error { + if len(p.ID) != 0 { + return errors.New("'Id' is not allowed for identity-based policy") + } + + for _, statement := range p.Statement { + if len(statement.Principal) != 0 || len(statement.NotPrincipal) != 0 { + return errors.New("'Principal' and 'NotPrincipal' are not allowed for identity-based policy") + } + } + + return nil +} + +func (p Policy) validateResourceBased() error { + for _, statement := range p.Statement { + if len(statement.Principal) == 0 && len(statement.NotPrincipal) == 0 { + return errors.New("'Principal' or 'NotPrincipal' must be provided for resource-based policy") + } + } + + return nil +} + +func (s Statement) principal() (Principal, bool) { + if len(s.NotPrincipal) != 0 { + return s.NotPrincipal, true + } + + return s.Principal, false +} + +func (s Statement) action() (Action, bool) { + if len(s.NotAction) != 0 { + return s.NotAction, true + } + + return s.Action, false +} + +func (s Statement) resource() (Resource, bool) { + if len(s.NotResource) != 0 { + return s.NotResource, true + } + + return s.Resource, false +} + +func (p Principal) validate() error { + if _, ok := p[Wildcard]; ok && len(p) == 1 { + return nil + } + + for key := range p { + if !key.IsValid() { + return fmt.Errorf("unknown principal type: '%s'", key) + } + } + + return nil +} diff --git a/iam/policy_test.go b/iam/policy_test.go index 4ffc536..11d0fcc 100644 --- a/iam/policy_test.go +++ b/iam/policy_test.go @@ -31,7 +31,7 @@ func TestUnmarshalIAMPolicy(t *testing.T) { ID: "PutObjPolicy", Statement: []Statement{{ SID: "DenyObjectsThatAreNotSSEKMS", - Principal: map[string][]string{ + Principal: map[PrincipalType][]string{ "*": nil, }, Effect: DenyEffect, @@ -78,8 +78,8 @@ func TestUnmarshalIAMPolicy(t *testing.T) { expected := Policy{ Version: "2012-10-17", Statement: []Statement{{ - Principal: map[string][]string{ - "AWS": {"arn:aws:iam::111122223333:user/JohnDoe"}, + Principal: map[PrincipalType][]string{ + AWSPrincipalType: {"arn:aws:iam::111122223333:user/JohnDoe"}, }, Effect: AllowEffect, Action: []string{"s3:PutObject"}, @@ -113,8 +113,8 @@ func TestUnmarshalIAMPolicy(t *testing.T) { expected := Policy{ Statement: []Statement{{ - Principal: map[string][]string{ - "AWS": {"arn:aws:iam::111122223333:user/JohnDoe"}, + Principal: map[PrincipalType][]string{ + AWSPrincipalType: {"arn:aws:iam::111122223333:user/JohnDoe"}, }, }}, } @@ -170,4 +170,255 @@ func TestUnmarshalIAMPolicy(t *testing.T) { require.NoError(t, err) require.Equal(t, expected, p) }) + + t.Run("'Not*' fields", func(t *testing.T) { + policy := ` +{ + "Id": "PutObjPolicy", + "Statement": [{ + "NotPrincipal": {"AWS":["arn:aws:iam::111122223333:user/Alice"]}, + "Effect": "Deny", + "NotAction": "s3:PutObject", + "NotResource": "arn:aws:s3:::DOC-EXAMPLE-BUCKET/*" + }] +}` + + expected := Policy{ + ID: "PutObjPolicy", + Statement: []Statement{{ + NotPrincipal: map[PrincipalType][]string{ + AWSPrincipalType: {"arn:aws:iam::111122223333:user/Alice"}, + }, + Effect: DenyEffect, + NotAction: []string{"s3:PutObject"}, + NotResource: []string{"arn:aws:s3:::DOC-EXAMPLE-BUCKET/*"}, + }}, + } + + var p Policy + err := json.Unmarshal([]byte(policy), &p) + require.NoError(t, err) + require.Equal(t, expected, p) + }) +} + +func TestValidatePolicies(t *testing.T) { + for _, tc := range []struct { + name string + policy Policy + typ PolicyType + isValid bool + }{ + { + name: "valid permission boundaries", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, + Resource: []string{Wildcard}, + }}, + }, + typ: GeneralPolicyType, + isValid: true, + }, + { + name: "general invalid effect", + policy: Policy{ + Statement: []Statement{{ + Effect: "dummy", + Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, + Resource: []string{Wildcard}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "general invalid principal block", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, + Resource: []string{Wildcard}, + Principal: map[PrincipalType][]string{Wildcard: nil}, + NotPrincipal: map[PrincipalType][]string{Wildcard: nil}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "general invalid not principal", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, + Resource: []string{Wildcard}, + NotPrincipal: map[PrincipalType][]string{AWSPrincipalType: {"arn:aws:iam::111122223333:user/Alice"}}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "general invalid principal type", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, + Resource: []string{Wildcard}, + NotPrincipal: map[PrincipalType][]string{"dummy": {"arn:aws:iam::111122223333:user/Alice"}}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "general invalid action block", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, + NotAction: []string{"iam:*"}, + Resource: []string{Wildcard}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "general invalid resource block", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Resource: []string{Wildcard}, + NotResource: []string{"arn:aws:s3:::DOC-EXAMPLE-BUCKET/*"}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "invalid resource block", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Resource: []string{}, + NotResource: []string{"arn:aws:s3:::DOC-EXAMPLE-BUCKET/*"}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "missing resource block", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "identity based valid", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:PutObject"}, + Resource: []string{Wildcard}, + }}, + }, + typ: IdentityBasedPolicyType, + isValid: true, + }, + { + name: "identity based invalid because of id presence", + policy: Policy{ + ID: "some-id", + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:PutObject"}, + Resource: []string{Wildcard}, + }}, + }, + typ: IdentityBasedPolicyType, + isValid: false, + }, + { + name: "identity based invalid because of principal presence", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:PutObject"}, + Resource: []string{Wildcard}, + Principal: map[PrincipalType][]string{AWSPrincipalType: {"arn:aws:iam::111122223333:user/Alice"}}, + }}, + }, + typ: IdentityBasedPolicyType, + isValid: false, + }, + { + name: "identity based invalid because of not principal presence", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:PutObject"}, + Resource: []string{Wildcard}, + NotPrincipal: map[PrincipalType][]string{AWSPrincipalType: {"arn:aws:iam::111122223333:user/Alice"}}, + }}, + }, + typ: IdentityBasedPolicyType, + isValid: false, + }, + { + name: "resource based valid principal", + policy: Policy{ + Statement: []Statement{{ + Effect: DenyEffect, + Action: []string{"s3:PutObject"}, + Resource: []string{Wildcard}, + Principal: map[PrincipalType][]string{AWSPrincipalType: {"arn:aws:iam::111122223333:user/Alice"}}, + }}, + }, + typ: ResourceBasedPolicyType, + isValid: true, + }, + { + name: "resource based valid not principal", + policy: Policy{ + ID: "some-id", + Statement: []Statement{{ + Effect: DenyEffect, + Action: []string{"s3:PutObject"}, + Resource: []string{Wildcard}, + NotPrincipal: map[PrincipalType][]string{AWSPrincipalType: {"arn:aws:iam::111122223333:user/Alice"}}, + }}, + }, + typ: ResourceBasedPolicyType, + isValid: true, + }, + { + name: "resource based invalid missing principal", + policy: Policy{ + ID: "some-id", + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:PutObject"}, + Resource: []string{Wildcard}, + }}, + }, + typ: ResourceBasedPolicyType, + isValid: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := tc.policy.Validate(tc.typ) + if tc.isValid { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } } diff --git a/inmemory_test.go b/inmemory_test.go index ef70492..b5be336 100644 --- a/inmemory_test.go +++ b/inmemory_test.go @@ -142,6 +142,22 @@ func TestInmemory(t *testing.T) { require.Equal(t, AccessDenied, status) require.True(t, ok) }) + t.Run("inverted rules", func(t *testing.T) { + req := newRequest("native::object::put", newResource(object, nil), nil) + status, ok = s.IsAllowed(Ingress, namespace2, req) + require.Equal(t, NoRuleFound, status) + require.False(t, ok) + + req = newRequest("native::object::put", newResource("native::object::cba/def", nil), nil) + status, ok = s.IsAllowed(Ingress, namespace2, req) + require.Equal(t, AccessDenied, status) + require.True(t, ok) + + req = newRequest("native::object::get", newResource("native::object::cba/def", nil), nil) + status, ok = s.IsAllowed(Ingress, namespace2, req) + require.Equal(t, NoRuleFound, status) + require.False(t, ok) + }) t.Run("good", func(t *testing.T) { status, ok = s.IsAllowed(Ingress, namespace, reqGood) require.Equal(t, NoRuleFound, status) @@ -174,20 +190,4 @@ func TestInmemory(t *testing.T) { require.True(t, ok) }) }) - t.Run("inverted rules", func(t *testing.T) { - req := newRequest("native::object::put", newResource(object, nil), nil) - status, ok = s.IsAllowed(Ingress, namespace2, req) - require.Equal(t, NoRuleFound, status) - require.False(t, ok) - - req = newRequest("native::object::put", newResource("native::object::cba/def", nil), nil) - status, ok = s.IsAllowed(Ingress, namespace2, req) - require.Equal(t, AccessDenied, status) - require.True(t, ok) - - req = newRequest("native::object::get", newResource("native::object::cba/def", nil), nil) - status, ok = s.IsAllowed(Ingress, namespace2, req) - require.Equal(t, NoRuleFound, status) - require.False(t, ok) - }) }