OKTA-649266 - update security policy for GA features #115

SajanAlexander-okta · 2023-11-09T14:15:04Z

Updates security policies to allow for GA features

secrets / secret folders
MFA condition
constrain security policy for resource group

oktapam/constants/descriptions/attributes.go

sachinsaxena-okta · 2023-11-09T16:15:48Z

oktapam/resource_security_policy.go

@@ -43,6 +42,11 @@ func resourceSecurityPolicy() *schema.Resource {
 				Required:    true,
 				Description: descriptions.SecurityPolicyActive,
 			},
+			attributes.ResourceGroup: {


Should this be ResourceGroupID ?

No, I think it's a named object so this should work fine

@mariovillaplana-okta I didn't get it. We pass ResourceGroupID for this attribute, correct ?

From what I've seen, we generally use the term without the ID as attribute names, but have the description indicate that the expectation is the UUID

sachinsaxena-okta · 2023-11-09T16:20:43Z

oktapam/resource_security_policy.go

+										MaxItems:    1,
+										Description: descriptions.SecurityPolicySecrets,
+										Elem: &schema.Resource{
+											Schema: map[string]*schema.Schema{


It's fine to add all the schema in the parent SecurityPolicy schema, but for readability I like creating separate schema type and then referencing it.

sachinsaxena-okta · 2023-11-09T16:28:22Z

oktapam/resource_security_policy.go

+										Description: descriptions.SecurityPolicySecrets,
+										Elem: &schema.Resource{
+											Schema: map[string]*schema.Schema{
+												attributes.Secret: {


I think only Secret or SecretFolder can be specified but not both. You can use ConflictsWith property in the schema. Take a look at -

terraform-provider-oktapam/oktapam/resource_ad_certificate_request.go

Line 72 in 8505915

ConflictsWith: []string{certDetailsTTLDays},

Because we use TypeLists/TypeSets, using ConflictsWith is kind of a nightmare. I ended up doing the validation within the code itself.

sachinsaxena-okta · 2023-11-09T16:35:56Z

oktapam/resource_security_policy.go

+															},
+														},
+													},
+												},


Can policy have multiple secret folders ?

A policy, yes, but not in a single rule

I've constrained this.

sachinsaxena-okta · 2023-11-09T16:40:45Z

oktapam/resource_security_policy.go

+										Description: descriptions.PrivilegeSecret,
+										Elem: &schema.Resource{
+											Schema: map[string]*schema.Schema{
+												attributes.List: {


Why these privileges need to be required ? Also, there are different privileges for secret and folder. If in the policy, we have a SecretFolder resource then what is the relevance of PrivilegeSecret*.

My goal here is to be explicit on the privileges granted, even if the default is false.

There are not different privileges for a secret/folder.

sachinsaxena-okta · 2023-11-09T16:50:24Z

oktapam/resource_security_policy.go

 	policy := client.SecurityPolicy{
 		ID:          id,
 		Name:        GetStringPtrFromResource(attributes.Name, d, false),
 		Active:      GetBoolPtrFromResource(attributes.Active, d, false),
 		Description: GetStringPtrFromResource(attributes.Description, d, false),
 	}

+	if resourceGroupID := GetStringPtrFromResource(attributes.ResourceGroup, d, false); resourceGroupID != nil {
+		if MatchesUUID(*resourceGroupID) {


I think UUID check can be part of the schema validation.

oktapam/constants/descriptions/attributes.go

mariovillaplana-okta · 2023-11-09T21:14:01Z

docs/data-sources/security_policy.md

@@ -69,6 +70,15 @@ Read-Only:
 - `traffic_forwarding` (Boolean)


+<a id="nestedobjatt--rule--conditions--mfa"></a>
+### Nested Schema for `rule.conditions.mfa`


It's hard for me to tell - does this allow them to try to set up MFA conditions on secret rules? That's not currently supported. I suppose the same question would apply to gateway conditions.

We constrain gateways, but not MFA - the main reason is that while this is not currently supported, we don't want to have to roll another version of the TF provider and migrate people when support is added.

mariovillaplana-okta

Approving since the API will surface the error if they try to add an MFA condition to a secret rule, but just wondering about the current behavior. Please test locally before merging

mariovillaplana-okta · 2023-11-09T21:16:36Z

oktapam/resource_security_policy.go

@@ -43,6 +42,11 @@ func resourceSecurityPolicy() *schema.Resource {
 				Required:    true,
 				Description: descriptions.SecurityPolicyActive,
 			},
+			attributes.ResourceGroup: {


No, I think it's a named object so this should work fine

mariovillaplana-okta · 2023-11-09T21:17:03Z

oktapam/resource_security_policy.go

+															},
+														},
+													},
+												},


A policy, yes, but not in a single rule

Co-authored-by: isaacdurham-okta <89145748+isaacdurham-okta@users.noreply.github.com>

* OKTA-649266 - update security policy for GA features * fix test * doc update * sort incoming list * Doc updates Co-authored-by: isaacdurham-okta <89145748+isaacdurham-okta@users.noreply.github.com> * constrain conditions * doc generation * constrain to one secret or secret folder in rule --------- Co-authored-by: isaacdurham-okta <89145748+isaacdurham-okta@users.noreply.github.com> initial commit

SajanAlexander-okta requested review from mariovillaplana-okta, isaacdurham-okta, leharbhatt-okta and sachinsaxena-okta November 9, 2023 14:15

SajanAlexander-okta force-pushed the sajan/security-policy-update branch from 52cec8a to f40096e Compare November 9, 2023 15:04

SajanAlexander-okta commented Nov 9, 2023

View reviewed changes

oktapam/constants/descriptions/attributes.go Outdated Show resolved Hide resolved

OKTA-649266 - update security policy for GA features

70de240

SajanAlexander-okta force-pushed the sajan/security-policy-update branch from f40096e to 70de240 Compare November 9, 2023 15:48

sachinsaxena-okta reviewed Nov 9, 2023

View reviewed changes

isaacdurham-okta approved these changes Nov 9, 2023

View reviewed changes

mariovillaplana-okta reviewed Nov 9, 2023

View reviewed changes

mariovillaplana-okta approved these changes Nov 9, 2023

View reviewed changes

SajanAlexander-okta and others added 7 commits November 9, 2023 16:32

fix test

8d2fd5f

doc update

057105a

sort incoming list

5aeafe5

Doc updates

57e91ee

Co-authored-by: isaacdurham-okta <89145748+isaacdurham-okta@users.noreply.github.com>

constrain conditions

00a3b14

doc generation

e2d4cd0

constrain to one secret or secret folder in rule

64e1185

sachinsaxena-okta approved these changes Nov 13, 2023

View reviewed changes

SajanAlexander-okta merged commit 6d756e4 into master Nov 13, 2023
6 checks passed

SajanAlexander-okta deleted the sajan/security-policy-update branch November 13, 2023 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OKTA-649266 - update security policy for GA features #115

OKTA-649266 - update security policy for GA features #115

SajanAlexander-okta commented Nov 9, 2023

sachinsaxena-okta Nov 9, 2023

mariovillaplana-okta Nov 9, 2023

sachinsaxena-okta Nov 9, 2023

SajanAlexander-okta Nov 10, 2023

sachinsaxena-okta Nov 9, 2023

sachinsaxena-okta Nov 9, 2023

SajanAlexander-okta Nov 10, 2023

sachinsaxena-okta Nov 9, 2023

mariovillaplana-okta Nov 9, 2023

SajanAlexander-okta Nov 10, 2023

sachinsaxena-okta Nov 9, 2023

SajanAlexander-okta Nov 10, 2023

sachinsaxena-okta Nov 9, 2023

mariovillaplana-okta Nov 9, 2023

SajanAlexander-okta Nov 10, 2023

mariovillaplana-okta left a comment

mariovillaplana-okta Nov 9, 2023

mariovillaplana-okta Nov 9, 2023

OKTA-649266 - update security policy for GA features #115

OKTA-649266 - update security policy for GA features #115

Conversation

SajanAlexander-okta commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariovillaplana-okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment