-
Notifications
You must be signed in to change notification settings - Fork 25
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
Added Helm chart for Kubernetes support #17
Conversation
kubernetes/configMaps/filterlist.txt
Outdated
@@ -0,0 +1,24 @@ | |||
resource:// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about this duplication. Since we merged in your last PR, there is already an example file that would be nice to somehow reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy enough to remove it either from the top level or from here. If I removed it from here I'd need to provide more instructions in the README.md
copy the sample.filterlist.txt file from root of this repo into the configMaps then add to the custom values file.
Then remove the simple example in kubernetes/values.yaml. Directly re-using the root level sample.filterlist.txt would be a bad idea as the config map is written to take any file in the target directory and create a config map from it. This makes the chart more extensible by allowing other config files to be created for the kubernetes pod(s) by simply putting them into the same directory.
To make practical use of this collector you either need to extend the functionality in code, or add a second container to read the json output. You're then in a position of
What config options does that second container need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As separate point surrounding the list and probably for a different PR
Should it be a block
list or a filter
list.
My thoughts are
For one reason or another these are values I do not want to know about. Do I want to reject the report, and as current generate a http 4xx status code, or do I want to simply ignore the reports and generate a http 202 status code or similar.
This comes down to, If someone opens the developer console of their browser should they see an accepted report for these or a error?
In my particular case we make use of google maps in part of some of our pages. Calling the map tile brings along a bunch of google specific fonts (see below) that we do not want. This is a known issue that sadly cannot be auctioned. As it tries to load about 3 or 4 fonts per tile it would swamp the reports and so are filtered out before being passed to the visualisation parts of the platform.
Perhaps both a rejected AND a filtered list is the way to go so some some will generate a 4xx code and others will be accepted, but ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a discussion for another PR but I'll give a quick background here to the existing functionality since it's something that is somewhat critical to this. The blockedURI
list is intended as a mechanism to drop any reports that are completely unactionable and provide no use to collection. There are a multitude of reasons for these hitting the collector (browser silliness, browser extensions, malicious actors) and they are something outside of the control of the operator so this list ensures that they don't need to worry about it. In the first year of our CSP, we received along the lines of 70% unactionable violation reports which took up storage and compute resources.
This comes down to, If someone opens the developer console of their browser should they see an accepted report for these or a error?
The console won't show either. The CSP violation reports are in a similar fashion to UDP where the report is fire and forget without any reporting on whether or the payload was accepted.
This is a known issue that sadly cannot be auctioned. As it tries to load about 3 or 4 fonts per tile it would swamp the reports and so are filtered out before being passed to the visualisation parts of the platform.
I think your approach of loading in a file with the blocked URIs is a great use case for this. We also do sampling on the application itself (by only defining the report-uri
directive for a percentage of requests) due to the sheer size of our application. Usually we launch a new directive change with a 0.001% as to not flood the log aggregator if something was missed. We later tune it up to a larger portion of the traffic.
Apologies for the delays in coming back to this, I started reviewing last week but then started thinking about how we want to handle integrations like this and ended up on a bit of an information gathering dive. On the surface, these are my thoughts:
I've gone ahead and setup the Docker Hub repository configuration for this one so once it lands, we should be golden for automated builds. |
Your responses have been quite positive and I think this one comes down to "Is this just a pet project that at best you'd prefer others to fork and extend to suit their needs, or is it one you want others to use as is"
Can do, but it should be very clear. If I look at other projects that have gone in that direction most have a
Agreed
A separated and updated Dockerfile is in PR #18
👍 |
Fortunately (or unfortunately depending on your viewpoint 😛 ) this has kind of already been decided with quite a few people adopting this as their production-ready CSP collector so I think it need to support the easy deployment method out of the box as much as possible.
Agree that it needs to be easily identifiable. I don't have a strong preference at this stage so happy to leave it as is. Doing a bit of research yesterday has yielded some results that ### Deployments
Currently supported deployment mechanisms:
- [kubernetes](/path/to/deployments/kubernetes/dir) |
@timothyclarke Is this still something you're interested in getting over the line? |
Yes. I've moved on from the org where I introduced this, and will be introducing elsewhere. |
Just doing a tidy up in here and this was still outstanding 🙂 After you get the Kubernetes stuff in place, I think the only outstanding comments were where the deployment directory was going to live and getting this up to date with master and the latest versions of tools. |
d20239c
to
d963b35
Compare
How's this? |
I noticed you have a dockerhub account. I haven't used travis-ci to build then push to any docker repo's, and it also looks like travic-ci does not have it's own docker repo.
I have assumed that travis-ci is primarily to ensure PR's a good and then build releases. to be executed outside of a docker container.
Using docker.com you can build both releases and more current builds. The Dockerfile in this PR written to create a container within the docker.com build system. It can also be built with
docker build -t csp-collector:latest .
The build process will use the
golang:1.10-alpine
container to build the collector. As the build container is quite large (a few hundred MB), It will then copy the built asset to analpine:3.8
container which is then published. The published container is about ~5MBThe
kubernetes
folder is a Helm chart to simplify the deployment into a k8s environment. While it has it's own README, anyone who's used helm should be able to use it easily.Note the kubernetes chart assumes pull #10 will be merged.
If needed I can create a slightly more complex chart which
I'm assuming you will use your hub.docker.com account to build this and set the
image.repository
inkubernetes/values.yaml
to reflect that. I've also set the URI of this repo in the dockermaintainer
/Author
label.Please do the following steps prior to merging this PR. That way when this merges in it will trigger a build.
Apologies if you already know this. To get this building in hub.docker.com:
Create
->Create Automated build
.Now every time you merge anything into master you will get a "latest" tag. Any other tags that are created at github will trigger a build at dockerhub with contents of that tag.
Please feel free to take a look at https://hub.docker.com/r/timothyclarke/go-csp-collector Note as Master does not have a Dockerfile it's not generating a container with the latest tag.
My use case is to forward these reports to Graylog which is similar to splunk in functionality. The k8s chart I'm using has an extra cli option which supplies the URI of the graylog destination. It's slightly lighter weight for me than writing to a file and then having a second container within the pod