set content-security-policy, x-content-type-options, and x-frame-options headers for console assets#5545
Conversation
this API is somewhat confusing, because i am not sure why one would need to _replace_ the list of allowed header names. it seems better to ensure that there is one place where every allowed response header is set.
|
|
||
| Ok(resp.body(file_contents.into())?) | ||
| let mut path = Utf8PathBuf::from("assets"); | ||
| path.extend(path_params.into_inner().path); |
There was a problem hiding this comment.
I can't find anywhere where we prevent the user from passing ".." in the path and walking back up the directory tree. How do we avoid that?
There was a problem hiding this comment.
I think I had assumed find_file does that, but rereading it I don't think it does (it guards against following symlinks). I will add checks to that function and write a test.
I'm debating whether it makes more sense to bail on any path segment that contains .., or if we should std::path::Path::canonicalize every path as it comes in and bail if it isn't inside static_dir.
There was a problem hiding this comment.
I also thought it did, and I thought we had tests for it. I don't a huge problem with taking the easy route and telling any .. to go away.
There was a problem hiding this comment.
Good news, Dropshot already does this. https://github.com/oxidecomputer/dropshot/blob/28c63c7f9d3b81729c0b0343640fb815877cdfab/dropshot/src/router.rs#L636-L687 (since v0.6.0, oxidecomputer/dropshot#118)
I'm going to add a test on the Omicron side to ensure that remains the case.
| .header(http::header::CONTENT_TYPE, content_type) | ||
| .header(http::header::CACHE_CONTROL, "max-age=31536000, immutable"); // 1 year | ||
| let stream = FramedRead::new(file, BytesCodec::new()); | ||
| let body = Body::wrap_stream(stream); |
There was a problem hiding this comment.
Does this mean we don't have to read the entire file into memory at once?
There was a problem hiding this comment.
Yep. That may have been leading to some request latency with the larger bundles, although not sure if it actually was in practice.
There was a problem hiding this comment.
Seems like probably not (this is colo, dogfood looks a bit different due to VPN I assume), but might as well do it the right way.
|
#5065 pulls in the CSP changes on the console side, so moving out of draft. |
3 participants
This is the Nexus side to oxidecomputer/console#2142, which needs to land before this lands. See also https://github.com/oxidecomputer/product-assurance/issues/50.
content-security-policytells a web browser what type of content, and from which origins, may be loaded on a page. The primary use is to help guard against cross-site scripting attacks and other kinds of novel attacks on web applications.x-content-type-options: nosnifftells the web browser to disallow content sniffing that can cause a browser to decide that responses with non-executable content types (e.g.image/png) can in fact be used as executable content types (e.g.text/javascript). This needs to be set for all console assets.x-frame-options: DENYdisallows embedding the console within another page, which helps to prevent click-jacking attacks. (This is obsoleted by theframe-ancestors 'none'CSP directive, but no harm in adding it.)content-security-policyonly needs to be set for the console index page, but there's no harm in setting it for the console assets as well.As part of this change I did some refactoring:
assetfunction and theserve_console_indexfunction are now in a single common function. This allows us to ship a gzip-compressed console index in the future.mime_guess; we only have a small list of file extensions we're willing to serve, so it doesn't make sense to compile a huge list of content types we'll never use into Nexus.There may be other headers from https://owasp.org/www-project-secure-headers/ (see the Best Practices tab) that we want but these are probably the most urgent.