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.

Sign up for GitHub

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

Security defenses browser headers #130

Merged
merged 8 commits into from
Jul 8, 2019
Merged

Security defenses browser headers #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
499bc68
security defenses browser headers
Jun 20, 2019
44bb250
security defenses browser headers
Jun 20, 2019
311025b
fallback on default setting when nothing is set
Jun 20, 2019
6ea7a1b
Merge branch 'Security_defenses_browser_headers' of https://github.co…
Jun 25, 2019
233da29
enable security defenses in example
Jun 25, 2019
0a8765e
enable security defenses in example
Jun 25, 2019
e6ad60b
removed brute force detection todos
Jul 3, 2019
147b4ba
Merge branch 'master' into Security_defenses_browser_headers
mrparkers Jul 8, 2019
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
14 changes: 13 additions & 1 deletion example/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ resource "keycloak_realm" "test" {
}
}

account_theme = "base"
account_theme = "base"

access_code_lifespan = "30m"

Expand All @@ -38,6 +38,18 @@ resource "keycloak_realm" "test" {
]
default_locale = "en"
}

security_defenses {
headers {
x_frame_options = "DENY"
content_security_policy = "frame-src 'self'; frame-ancestors 'self'; object-src 'none';"
content_security_policy_report_only = ""
x_content_type_options = "nosniff"
x_robots_tag = "none"
x_xss_protection = "1; mode=block"
strict_transport_security = "max-age=31536000; includeSubDomains"
}
}
}

resource "keycloak_required_action" "custom-terms-and-conditions" {
Expand Down
13 changes: 13 additions & 0 deletions keycloak/realm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,19 @@ type Realm struct {
InternationalizationEnabled bool `json:"internationalizationEnabled"`
SupportLocales []string `json:"supportedLocales"`
DefaultLocale string `json:"defaultLocale"`

//extra attributes of a realm, contains security defenses browser headers and brute force detection parameters(those still nee to be added)
Attributes Attributes `json:"attributes,omitempty"`
}

type Attributes struct {
BrowserHeaderContentSecurityPolicy string `json:"_browser_header.contentSecurityPolicy,omitempty"`
BrowserHeaderContentSecurityPolicyReportOnly string `json:"_browser_header.contentSecurityPolicyReportOnly,omitempty"`
BrowserHeaderStrictTransportSecurity string `json:"_browser_header.strictTransportSecurity,omitempty"`
BrowserHeaderXContentTypeOptions string `json:"_browser_header.xContentTypeOptions,omitempty"`
BrowserHeaderXFrameOptions string `json:"_browser_header.xFrameOptions,omitempty"`
BrowserHeaderXRobotsTag string `json:"_browser_header.xRobotsTag,omitempty"`
BrowserHeaderXXSSProtection string `json:"_browser_header.xXSSProtection,omitempty"`
}

type SmtpServer struct {
Expand Down
113 changes: 113 additions & 0 deletions provider/resource_keycloak_realm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,61 @@ func resourceKeycloakRealm() *schema.Resource {
},
},
},

//Security Defenses
"security_defenses": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"headers": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"x_frame_options": {
Type: schema.TypeString,
Optional: true,
Default: "SAMEORIGIN",
},
"content_security_policy": {
Type: schema.TypeString,
Optional: true,
Default: "frame-src 'self'; frame-ancestors 'self'; object-src 'none';",
},
"content_security_policy_report_only": {
Type: schema.TypeString,
Optional: true,
Default: "",
},
"x_content_type_options": {
Type: schema.TypeString,
Optional: true,
Default: "nosniff",
},
"x_robots_tag": {
Type: schema.TypeString,
Optional: true,
Default: "none",
},
"x_xss_protection": {
Type: schema.TypeString,
Optional: true,
Default: "1; mode=block",
},
"strict_transport_security": {
Type: schema.TypeString,
Optional: true,
Default: "max-age=31536000; includeSubDomains",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tomrutsaert tomrutsaert Jul 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@tomrutsaert tomrutsaert Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

},
},
},
},
},
},
},
},
}
}
Expand Down Expand Up @@ -456,9 +511,44 @@ func getRealmFromData(data *schema.ResourceData) (*keycloak.Realm, error) {
realm.ActionTokenGeneratedByAdminLifespan = actionTokenGeneratedByAdminLifespanDurationString
}

//security defenses
if v, ok := data.GetOk("security_defenses"); ok {
securityDefensesSettings := v.([]interface{})[0].(map[string]interface{})

headersConfig := securityDefensesSettings["headers"].([]interface{})
if len(headersConfig) == 1 {
headerSettings := headersConfig[0].(map[string]interface{})

realm.Attributes = keycloak.Attributes{
BrowserHeaderContentSecurityPolicy: headerSettings["content_security_policy"].(string),
BrowserHeaderContentSecurityPolicyReportOnly: headerSettings["content_security_policy_report_only"].(string),
BrowserHeaderStrictTransportSecurity: headerSettings["strict_transport_security"].(string),
BrowserHeaderXContentTypeOptions: headerSettings["x_content_type_options"].(string),
BrowserHeaderXFrameOptions: headerSettings["x_frame_options"].(string),
BrowserHeaderXRobotsTag: headerSettings["x_robots_tag"].(string),
BrowserHeaderXXSSProtection: headerSettings["x_xss_protection"].(string),
}
} else {
setDefaultSecuritySettings(realm)
}
} else {
setDefaultSecuritySettings(realm)
}
return realm, nil
}

func setDefaultSecuritySettings(realm *keycloak.Realm) {
realm.Attributes = keycloak.Attributes{
BrowserHeaderContentSecurityPolicy: "frame-src 'self'; frame-ancestors 'self'; object-src 'none';",
BrowserHeaderContentSecurityPolicyReportOnly: "",
BrowserHeaderStrictTransportSecurity: "max-age=31536000; includeSubDomains",
BrowserHeaderXContentTypeOptions: "nosniff",
BrowserHeaderXFrameOptions: "SAMEORIGIN",
BrowserHeaderXRobotsTag: "none",
BrowserHeaderXXSSProtection: "1; mode=block",
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

@kevinduterne kevinduterne Jul 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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..


func setRealmData(data *schema.ResourceData, realm *keycloak.Realm) {
data.SetId(realm.Realm)

Expand Down Expand Up @@ -534,6 +624,29 @@ func setRealmData(data *schema.ResourceData, realm *keycloak.Realm) {
} else {
data.Set("internationalization", nil)
}

if _, ok := data.GetOk("security_defenses"); ok {

if (keycloak.Attributes{}) == realm.Attributes {
data.Set("security_defenses", nil)
} else {
securityDefensesSettings := make(map[string]interface{})

headersSettings := make(map[string]interface{})

headersSettings["content_security_policy"] = realm.Attributes.BrowserHeaderContentSecurityPolicy
headersSettings["content_security_policy_report_only"] = realm.Attributes.BrowserHeaderContentSecurityPolicyReportOnly
headersSettings["strict_transport_security"] = realm.Attributes.BrowserHeaderStrictTransportSecurity
headersSettings["x_content_type_options"] = realm.Attributes.BrowserHeaderXContentTypeOptions
headersSettings["x_frame_options"] = realm.Attributes.BrowserHeaderXFrameOptions
headersSettings["x_robots_tag"] = realm.Attributes.BrowserHeaderXRobotsTag
headersSettings["x_xss_protection"] = realm.Attributes.BrowserHeaderXXSSProtection

securityDefensesSettings["headers"] = []interface{}{headersSettings}

data.Set("security_defenses", []interface{}{securityDefensesSettings})
}
}
}

func resourceKeycloakRealmCreate(data *schema.ResourceData, meta interface{}) error {
Expand Down
65 changes: 65 additions & 0 deletions provider/resource_keycloak_realm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,35 @@ func TestAccKeycloakRealm_computedTokenSettings(t *testing.T) {
})
}

func TestAccKeycloakRealm_securityDefenses(t *testing.T) {
realmName := "terraform-" + acctest.RandString(10)
realmDisplayName := "terraform-" + acctest.RandString(10)

resource.Test(t, resource.TestCase{
Providers: testAccProviders,
PreCheck: func() { testAccPreCheck(t) },
CheckDestroy: testAccCheckKeycloakRealmDestroy(),
Steps: []resource.TestStep{
{
Config: testKeycloakRealm_basic(realmName, realmDisplayName),
Check: testAccCheckKeycloakRealmSecurityDefenses("keycloak_realm.realm", "SAMEORIGIN"),
},
{
Config: testKeycloakRealm_securityDefenses(realmName, realmDisplayName, "SAMEORIGIN"),
Check: testAccCheckKeycloakRealmSecurityDefenses("keycloak_realm.realm", "SAMEORIGIN"),
},
{
Config: testKeycloakRealm_securityDefenses(realmName, realmDisplayName, "DENY"),
Check: testAccCheckKeycloakRealmSecurityDefenses("keycloak_realm.realm", "DENY"),
},
{
Config: testKeycloakRealm_basic(realmName, realmDisplayName),
Check: testAccCheckKeycloakRealmSecurityDefenses("keycloak_realm.realm", "SAMEORIGIN"),
},
},
})
}

func testKeycloakRealmLoginInfo(resourceName string, realm *keycloak.Realm) resource.TestCheckFunc {
return func(s *terraform.State) error {
realmFromState, err := getRealmFromState(s, resourceName)
Expand Down Expand Up @@ -607,6 +636,21 @@ func getRealmFromState(s *terraform.State, resourceName string) (*keycloak.Realm
return realm, nil
}

func testAccCheckKeycloakRealmSecurityDefenses(resourceName, xFrameOptions string) resource.TestCheckFunc {
return func(s *terraform.State) error {
realm, err := getRealmFromState(s, resourceName)
if err != nil {
return err
}

if realm.Attributes.BrowserHeaderXFrameOptions != xFrameOptions {
return fmt.Errorf("expected realm %s to have attribute _browser_header.xFrameOptions set to %s, but was %s", realm.Realm, xFrameOptions, realm.Attributes.BrowserHeaderXFrameOptions)
}

return nil
}
}

func testKeycloakRealm_basic(realm, realmDisplayName string) string {
return fmt.Sprintf(`
resource "keycloak_realm" "realm" {
Expand Down Expand Up @@ -839,3 +883,24 @@ resource "keycloak_realm" "realm" {
}
`, realm, realm, ssoSessionIdleTimeout, ssoSessionMaxLifespan, offlineSessionIdleTimeout, offlineSessionMaxLifespan, accessTokenLifespan, accessTokenLifespanForImplicitFlow, accessCodeLifespan, accessCodeLifespanLogin, accessCodeLifespanUserAction, actionTokenGeneratedByUserLifespan, actionTokenGeneratedByAdminLifespan)
}

func testKeycloakRealm_securityDefenses(realm, realmDisplayName, xFrameOptions string) string {
return fmt.Sprintf(`
resource "keycloak_realm" "realm" {
realm = "%s"
enabled = true
display_name = "%s"
security_defenses {
headers {
x_frame_options = "%s"
content_security_policy = "frame-src 'self'; frame-ancestors 'self'; object-src 'none';"
content_security_policy_report_only = ""
x_content_type_options = "nosniff"
x_robots_tag = "none"
x_xss_protection = "1; mode=block"
strict_transport_security = "max-age=31536000; includeSubDomains"
}
}
}
`, realm, realmDisplayName, xFrameOptions)
}