Security defenses browser headers

[tomrutsaert]

Added Security Defenses Browser Headers fields on Realm

[tomrutsaert]

@mrparkers , do you have any comments or remarks on this pull request?
It is quite straightforward. The only strange thing is that you need to set the defaults when the headers are missing.

[mrparkers]

Sorry for the late review, this slipped my mind.

[mrparkers]

Can you remove the todo comments and open an issue instead? Or do you plan on implementing this within this PR?

[tomrutsaert]

I plan on adding them later, but not within this PR, I will remove the comments

[mrparkers]

Should we be setting these defaults manually, or relying on Keycloak to do this for us? I'm worried that there is a chance that upgrading your Keycloak instance could change the recommended defaults, but the provider would still set the old values.

[tomrutsaert]

This allows users to only fill in the fields where they want to diverge from the default keycloak value. If there is no default value in the scheme, we would need to make every field required. This would mean that the user would need to go and look in keycloak to copy the values of the fields they do not want to change.
If we put the default on empty string, an empty string will passed to keycloak, and imho that is also not a desired result for the users.

Even if keycloak would change the recommended defaults in the future, these current default values will be better than no value, or a value that the user would have copied from the keycloak gui.

[mrparkers]

I agree that we would want sensible defaults to be set if the user omits any of these attributes, but what I'm suggesting is to let Keycloak handle that instead of hardcoding the defaults (which may change in the future) within the provider.

If a user doesn't specify one of these attributes, we don't have to send Keycloak an empty string - we can use omitempty within the JSON definition for the struct to tell the provider to not include properties with empty string values when sending the request to Keycloak. This will allow Keycloak to set the default to whatever it wants.

What are your thoughts on this?

[tomrutsaert]

I can remove the defaults.
But Imho I prefer to have the defaults, because of following example without the defaults:

lets focus on the x_robots_tag field.

User creates following config:

resource "keycloak_realm" "test" {
	realm        = "test"
	enabled      = true
	display_name = "foo"
	
	security_defenses {
		headers {
			x_frame_options = "DENY"
		}
	}
}

in keycloak x_robots_tag has now the value none

user changes the config to:

resource "keycloak_realm" "test" {
	realm        = "test"
	enabled      = true
	display_name = "foo"

	security_defenses {
		headers {
			x_frame_options = "DENY"
			x_robots_tag = "bla"
		}
	}
}

in keycloak x_robots_tag has now the value bla

user removes the value again

resource "keycloak_realm" "test" {
	realm        = "test"
	enabled      = true
	display_name = "foo"
	
	security_defenses {
		headers {
			x_frame_options = "DENY"
		}
	}
}

in keycloak x_robots_tag has now the value ""

because

Terraform will perform the following actions:

  ~ keycloak_realm.test
      security_defenses.0.headers.0.x_robots_tag: "bla" => ""

(could this be solved by checking in getRealmFromData if x_robots_tag is present?)

In the end this heavily depends on what you decide on the comment/discussion below. Because If we expect that the plugin restores the default values, we need the default values in the scheme. If not, we can remove the defaults.

[mrparkers]

Okay, I'm fine with keeping what you currently have implemented.

[mrparkers]

I'm not sure I understand the purpose of this function if the schema is already setting these defaults, could you clear this up for me?

[tomrutsaert]

The problem is, when you remove the 'headers' block in terraform, the values that you had set before, will persist and will not be reset to the keycloak defaults.
You can also not make them empty as then terraform will send empty strings(which are valid values) to keycloak and thus keycloak will not work anymore as intended,
So my idea is, if there is no 'headers' block, then set values to sensible defaults. (Imho, these are general sensible defaults and not limited to keycloak)

Furthermore the default values set in scheme, will only be used when there is a 'headers' block. This way, users only need to fill in headers to are different from the default.
When there is no headers block terraform does something like this:

example

1. Set x frame options to DENY
2. remove security defenses and headers block from main.tf (without the setDefaultSecuritySettings(realm) method)

Terraform will perform the following actions:

  ~ keycloak_realm.test
      security_defenses.#:                           "1" => "0"
      security_defenses.0.headers.#:                 "1" => "0"
      security_defenses.0.headers.0.x_frame_options: "DENY" => "SAMEORIGIN"

But in Keycloak this will remain on "DENY"

with the setDefaultSecuritySettings(realm) method, this works as intended. also see test TestAccKeycloakRealm_securityDefenses

I agree this feels like a workaround/patch solution, But I did not see any other solution.
If you have a better solution to make this work transparently for users, I am happy to change this implementation.

[mrparkers]

Hmm, I'm torn on this one. As a user, would you expect that deleting an attribute would cause a change to be made within Keycloak, or for that attribute to simply not be managed by Terraform anymore?

I think I'm in favor of the latter. Most of my Terraform experience is with the Google provider, and a lot of their resources behave this way - if an optional field is deleted, it doesn't change it back to the perceived default, it simply stops caring about it.

I understand the point you're making, I'm just not sure what kind of behavior a typical Terraform user would expect here.

[kevinduterne]

I dont agree on the latter.. If you delete an attribute from terraform it will delete or change that managed resource, it is only when you remove the current state of that resource & delete the attribute from the terraform code that it is "forgotten" or no longer managed.

So as a typical terraform user removing something from my terraform codebase I do expect changes..

[tomrutsaert]

Great, Thanks 👍