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.

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

Add csp #3

Merged
merged 10 commits into from Oct 13, 2019

Conversation

@cak
Copy link
Contributor

cak commented Oct 9, 2019

Add support for Content Security Policy (CSP)

Description

Created Content Security Policy policy builder and implementation.

let mut headers = http::HeaderMap::new();
armor::armor(&mut headers);
let mut csp_policy = armor::ContentSecurityPolicy::new();

csp_policy
    .default_src(&["'self'", "areweasyncyet.rs"])
    .script_src(&["'self'"])
    .object_src(&["'none'"])
    .upgrade_insecure_requests();

armor::content_security_policy(&mut headers, csp_policy);

Motivation and Context

The CSP headers prevents and detects injection attacks and is included in Helmet.js but missing from armor.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Happy Hacktoberfest!

@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Oct 9, 2019

@cak thanks so much for this PR! Happy hacktoberfest to you too! -- I feel this is very much in the right direction, but we can probably make the API slightly more polished even.

Tricky things right now

  • typing "'self'" is easy to get wrong, and it's unclear what all the options are
  • each method always takes a borrowed array of borrowed strings, which is hard to remember
  • The ContentSecurityPolicy struct and content_security_policy free function are intrinsically linked, but need to be imported together, which can be hard to remember.
  • content_security_policy is quite a lot to type out, and prone to typos.

Design Proposal

I suggest we create a new csp submodule, which exposes a new free function to create a new ContentSecurityPolicy struct (so it's easy to remember + type). And which exposes an apply function which applies the policy to a HeaderMap instance.

Additionally I think we should capture "'self'" and "'none'" inside of an enum that has a Display impl. And in turn all methods should take single instances using an AsRef<str> bound on the API.

Putting this all together the example above would then look something like this:

let policy = armor::csp::new()
    .default_src(csp::Source::Self)
    .default_src("areweasyncyet.rs")
    .script_src(csp::Source::Self)
    .script_src(csp::Source::UnsafeInline)
    .object_src(csp::Source::None)
    .base_uri(csp::Source::None)
    .upgrade_insecure_requests();


let mut headers = http::HeaderMap::new();
armor::armor(&mut headers);
policy.apply(&mut headers);

Conclusion

I think the direction you've taken this is really good already, and with a few design tweaks I think we can make this one of the easiest ways of creating CSP policies of any language. Thanks heaps!

@cak

This comment has been minimized.

Copy link
Contributor Author

cak commented Oct 9, 2019

@yoshuawuyts, thank you so much for the awesome feedback! I am in application security, but not as a developer, and am still learning Rust. I appreciate your patience (future included) - this is a great opportunity to help me write more idiomatic Rust Code 🥳. I will work on your suggestions and have another draft for your review.

@cak

This comment has been minimized.

Copy link
Contributor Author

cak commented Oct 11, 2019

@yoshuawuyts, thanks again for your guidance on this API.

Here is the next version of the draft for your review:

let mut policy = armor::csp::new();

policy
    .default_src(csp::Source::SameOrigin)
    .default_src("areweasyncyet.rs")
    .script_src(csp::Source::SameOrigin)
    .script_src(csp::Source::UnsafeInline)
    .object_src(csp::Source::None)
    .base_uri(csp::Source::None)
    .upgrade_insecure_requests();

let mut headers = http::HeaderMap::new();
armor::armor(&mut headers);
policy.apply(&mut headers);
cak added 2 commits Oct 11, 2019
@cak

This comment has been minimized.

Copy link
Contributor Author

cak commented Oct 11, 2019

The the or_insert line was failing the CI tests (worked on my machine 😆), I replaced it with .or_insert_with and then 😦 with the correct Vec::new closure.

Copy link
Member

yoshuawuyts left a comment

This is really good! Thanks so much!

I spotted one small formatting error in the docs, but otherwise I'd be happy to merge this!

src/csp.rs Outdated Show resolved Hide resolved
@cak

This comment has been minimized.

Copy link
Contributor Author

cak commented Oct 12, 2019

Thanks @yoshuawuyts for all your help and guidance!

Copy link
Member

yoshuawuyts left a comment

Excellent; thanks so much!

@yoshuawuyts yoshuawuyts merged commit 8a898aa into http-rs:master Oct 13, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Oct 13, 2019

v1.2.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.