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

feat: add CSP and other security related headers in the oCIS proxy service #8777

Merged
merged 5 commits into from
Apr 26, 2024

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Apr 4, 2024

Description

CSP headers and other security related headers are now added to http responses in the proxy service.

The default settings are loaded from https://github.com/owncloud/ocis/pull/8777/files#diff-106b7ab6229528e8a62323d217f101db7000e6918aefb124cea4c845d6b9d1c8

This default yaml config file can be overwritten by specifying an alternative yaml file path in the env var PROXY_CSP_CONFIG_FILE_LOCATION
The ocis_wopi example deployment shows how to do this - refs https://github.com/owncloud/ocis/pull/8777/files#diff-bfedf12f7818c318b754f308bff1d6139afd701f2fc7bb43f07fd865a0d85039

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

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)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

This comment was marked as resolved.

@micbar
Copy link
Contributor

micbar commented Apr 4, 2024

We need to be very thorough with regresssions here.

@wkloucek Tested that via the reverse proxy and this broke all web client, app provider and web office features.

@micbar
Copy link
Contributor

micbar commented Apr 4, 2024

I like this elegant approach. 😍

@DeepDiver1975
Copy link
Member Author

This is just a starting point.... Needs a lot of testing and finally adjustments in web and integrated systems....

@DeepDiver1975
Copy link
Member Author

DeepDiver1975 commented Apr 5, 2024

We might want to consider to add CSP only on the front end service...
Because CSP is only relevant on the web Frontend delivering endpoint

@micbar
Copy link
Contributor

micbar commented Apr 5, 2024

web assets (js and config) are coming from the web service in ocis.

What about the apis?
We have graph, webdav, ocs, settings, apps

@DeepDiver1975
Copy link
Member Author

CSP is only relevant for web hosting endpoints.
This defines the scope for the web page on browser.

@DeepDiver1975
Copy link
Member Author

@micbar where can I get following information

  • Is draw.io enabled?
  • Is OO enabled? And where is the url available?
  • Is Collabora enabled? And where to get the url from?

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

like it 👍 some questions and please comment exported funcs

services/proxy/pkg/middleware/security.go Outdated Show resolved Hide resolved
services/proxy/pkg/middleware/security.go Show resolved Hide resolved
services/proxy/pkg/config/csp.go Show resolved Hide resolved
services/proxy/pkg/config/config.go Outdated Show resolved Hide resolved
@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review April 23, 2024 15:14
@DeepDiver1975 DeepDiver1975 force-pushed the feat/sec-headers branch 2 times, most recently from 63040a7 to 92a1eec Compare April 24, 2024 08:40
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Just a tiny error message text suggestion.

services/proxy/pkg/command/server.go Outdated Show resolved Hide resolved
@DeepDiver1975
Copy link
Member Author

Oh come on ...... replacement are not allowed: github.com/unrolled/secure
https://sonarcloud.io/project/issues?open=AY8QWu1wICG_leOl4WV_&id=owncloud_ocis

Copy link

sonarcloud bot commented Apr 25, 2024

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@micbar
Copy link
Contributor

micbar commented Apr 25, 2024

Sorry to intervene, but now we have different env var parsers, the ocis yaml files parse with gookit, the csp yaml with the new lib. We should align.

@DeepDiver1975
Copy link
Member Author

Sorry to intervene, but now we have different env var parsers, the ocis yaml files parse with gookit, the csp yaml with the new lib. We should align.

Use gookit for csp.yaml:

Have two different syntax in the docker compose setup?

Use os.ExpandEnv

https://pkg.go.dev/os#ExpandEnv
... can be used for simple substitutions

Question on gookit usage

Which syntax for car substitution is used in ocis?

@micbar
Copy link
Contributor

micbar commented Apr 25, 2024

I think we should use your new lib (envsubst) in ocis for the ocis yaml config.

There is one ParseConfig method which is used in all services. Should be a one liner.

@micbar
Copy link
Contributor

micbar commented Apr 25, 2024

Here https://github.com/owncloud/ocis/blob/master/ocis-pkg/config/helpers.go#L21

We should be able to pass the config as []bytes to our gookit/config via LoadSources()

@DeepDiver1975
Copy link
Member Author

Any example config files at hand which hold env vars?
Are these hand crafted by users or do we generate them as well.
I did not find any reference/example. Thx

@micbar
Copy link
Contributor

micbar commented Apr 26, 2024

All ocis config vars can also be set by yaml files. A rendered version is located in the dev docs in every service.

@DeepDiver1975 DeepDiver1975 merged commit bdbba92 into master Apr 26, 2024
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feat/sec-headers branch April 26, 2024 07:10
ownclouders pushed a commit that referenced this pull request Apr 26, 2024
…rvice (#8777)

* feat: add CSP and other security related headers in the oCIS proxy service

* fix: consolidate security related headers - drop middleware.Secure

* fix: use github.com/DeepDiver1975/secure

* fix: acceptance tests

* feat: support env var replacements in csp.yaml
@@ -88,8 +88,10 @@ services:
MICRO_REGISTRY_ADDRESS: 127.0.0.1:9233
NATS_NATS_HOST: 0.0.0.0
NATS_NATS_PORT: 9233
PROXY_CSP_CONFIG_FILE_LOCATION: /etc/ocis/csp.yaml

Choose a reason for hiding this comment

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

@DeepDiver1975
Shouldn't ONLYOFFICE_DOMAIN and COLLABORA_DOMAIN be set here as well in order for the substitution in etc/ocis/csp.yaml to take effect?

ONLYOFFICE_DOMAIN: ${ONLYOFFICE_DOMAIN:-onlyoffice.owncloud.test}
COLLABORA_DOMAIN: ${COLLABORA_DOMAIN:-collabora.owncloud.test}

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@DeepDiver1975 Yes, I mean the csp.yaml is being loaded inside the containers environment, so if the ONLYOFFICE_DOMAIN and COLLABORA_DOMAIN are not set in the docker-compose.yml file they will be empty during runtime and the default values are used. (This is at least what happened when I tried it.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah. I see your point now.let me have a look .....

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - but the env vars are not forwarded into the ocis container and will always be set to the default.
pr coming ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, right

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

THX a lot @kjeldahl for bringing this to our attention! 🙏

Choose a reason for hiding this comment

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

@DeepDiver1975 You are most welcome. And thanks a lot for solving the CSP issue 4 hours before I ran into it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

csp and other sec relevant headers ....
5 participants