Added config flag to disable frame-ancestor for the nomad UI

[ebarriosjr]

In order to be able to embed the nomad UI in an iframe as requested under #7056 I made a test implementation to add a configuration variable that disables frame-ancestors 'none'
The configuration option goes under the UI config and it is a boolen flag called: DisableFrameAncestors

Fixes: #7056

[tgross]

Hi @ebarriosjr! Thanks for the PR!

This configuration value lets us drop the specific item you're looking for in #7056 but seems like the sort of thing we'll end up needing to extend multiple times. I'm sort of thinking it might be a good idea to allow arbitrary configuration here but that might actually be exposing users who don't know much about CSP to higher risk.

So I'd love to get some input from @picatz and @philrenaud on this. Are the other values we set for CSP even sensible for a user to want to override?

Once we've settled out any design discussion, can you:

• Run make cl to add a changelog entry.
• Add an appropriate documentation section to ui.mdx
• Can you outline a little bit how you tested that this does the job for allowing iframes?

[DingoEatingFuzz]

I'm sort of thinking it might be a good idea to allow arbitrary configuration here

I tend to agree. I don't think it hurts to open it up even if most users aren't going to need to override any headers or add any headers.

but that might actually be exposing users who don't know much about CSP to higher risk.

The way vault implements this is you literally override the entire Content-Security-Policy HTTP header (or any other HTTP header for that matter). This approach would definitely expose users to risk since setting a policy you care about could result in accidentally unsetting a policy you didn't know you care about.

If it's good enough for Vault maybe it's good enough for Nomad? Or maybe it's only good enough for Vault because they expect operators to be well versed in all things security related?

Either way, I like an individual policy approach where the overrides are merged with the defaults. This would also change the proposed config flag from

DisableFrameAncestors = true

To, idk, something like

ContentSecurityPolicy {
  FrameAncestors = "*"
}

[tgross]

To, idk, something like

ContentSecurityPolicy {
  FrameAncestors = "*"
}

Ooo, I would not hate that. Especially if we made it so that leaving values unset left the defaults. We'd end up with something like the following, and then there'd be a method on ContentSecurityPolicy that turns it into the appropriate header string with quotes as needed.

// only covers the elements of
// https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP we need or care about
type ContentSecurityPolicy struct {
	ConnectSrc     []string
	DefaultSrc     []string
	FormAction     []string
	FrameAncestors []string
	ImgSrc         []string
	ScriptSrc      []string
	StyleSrc       []string
}

var defaultCSP = ContentSecurityPolicy{
	ConnectSrc:     []string{"*"},
	DefaultSrc:     []string{"none"},
	FormAction:     []string{"none"},
	FrameAncestors: []string{"none"},
	ImgSrc:         []string{"self", "data:"},
	ScriptSrc:      []string{"self"},
	StyleSrc:       []string{"self", "unsafe-inline"},
}

[tgross]

I think we're getting closer, @ebarriosjr. I've left some comments, but can you also run make cl to add a changelog entry for this PR? We'll also need the new configuration block to be documented in https://github.com/hashicorp/nomad/blob/main/website/content/docs/configuration/ui.mdx

[tgross]

There's a Merge method on all the structs in nomad/structs/config/ui.go. We should move this logic and most of the logic you have in command/agent/http.go into a Merge method on ContentSecurityPolicy that gets called by UIConfig.Merge.

[tgross]

Note that you've dropped the nil-check of agentConfig.UI here, so the new code will cause the agent to panic if the ui block is unset (this is happening in failing tests right now). Likewise for the ContentSecurityPolicy field.

You'll need a check for both nils, and at that point you might want to consider assigning to a variable like policy := agentConfig.UI.ContentSecurityPolicy just so you don't have to write out the chain of dereferences a half-dozen times. But see also my comment below about where this merge should live.

[ebarriosjr]

The handleUI function is called after checking if the agentConfig.UI is not nil on line 509. That is why i did not recheck this.

[ebarriosjr]

The ContentSecurityPolicy will never be nil because I added defaults values on the nomad/structs/config/ui.go file. IMHO

[ebarriosjr]

@tgross thanks for all the comments, I made another commit before reading your comments. I will keep working on the comments and make another commit. :)

[tgross]

LGTM!

[tgross]

Backported to the 1.6.x release branch

[DingoEatingFuzz]

Thank you for your work on this @ebarriosjr! We handed you a bunch of scope creep and you implemented it all in stride 💚