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

Jump to bottom

Added auth policy for app-validator, app-event-service and source adapter #7349

Merged
merged 18 commits into from Mar 18, 2020
Merged

Added auth policy for app-validator, app-event-service and source adapter #7349

merged 18 commits into from Mar 18, 2020

Conversation

sayanh
Copy link
Contributor

@sayanh sayanh commented Feb 26, 2020 •
edited

Description

Changes proposed in this pull request:

  • Created AuthorizationPolicy for:
    • {{application}}-connectivity-validator
    • {{application}}-event-service
    • {{application}}-http-source
  • Creation of AuthorizationPolicy should be controlled by an override
  • Narrowed down PERMISSIVE policy for HTTP source adapter just to metrics

Related issue(s)

See #7240
Chart bump PR: #7356

@netlify
Copy link

netlify bot commented Feb 26, 2020 •
edited

🥰 Documentation preview ready! 🥰

Built with commit 8320122

https://deploy-preview-7349--kyma-project-docs-preview.netlify.com

@kyma-project kyma-project locked as off-topic and limited conversation to collaborators Feb 26, 2020
@kyma-project kyma-project unlocked this conversation Feb 26, 2020
@kyma-project kyma-project locked as too heated and limited conversation to collaborators Feb 26, 2020
@sayanh sayanh force-pushed the add-auth-policy branch 3 times, most recently from ddc8c95 to 2107f7d Compare March 4, 2020 15:01
@sayanh sayanh changed the title Added auth policy for validator and source adapter Added auth policy for app-validator, app-event-service and source adapter Mar 4, 2020
@sayanh sayanh added area/security Issues or PRs related to security area/eventing Issues or PRs related to eventing labels Mar 4, 2020
@sayanh sayanh mentioned this pull request Mar 4, 2020
Closed
@sayanh sayanh force-pushed the add-auth-policy branch 3 times, most recently from ec43470 to 129ca47 Compare March 4, 2020 17:23
@kyma-project kyma-project unlocked this conversation Mar 4, 2020
@sayanh sayanh marked this pull request as ready for review March 4, 2020 17:25
k15r
k15r reviewed Mar 5, 2020
View reviewed changes
components/application-connectivity-validator/internal/validationproxy/handler.go Outdated
@@ -295,7 +295,8 @@ func createReverseProxy(destinationHost string, reqOpts ...requestOption) *httpu
Director: func(request *http.Request) {
request.URL.Scheme = "http"
request.URL.Host = destinationHost

request.Host = ""
delete(request.Header, "X-Forwarded-Client-Cert")
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
Contributor

Choose a reason for hiding this comment

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

There is a method for that: https://golang.org/pkg/net/http/#Header.Del

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

k15r
k15r requested changes Mar 5, 2020
View reviewed changes
components/application-connectivity-validator/internal/validationproxy/handler.go Outdated Show resolved Hide resolved
components/application-operator/charts/application/templates/authorization-policy.yaml Outdated
- from:
- source:
principals:
- cluster.local/ns/kyma-system/sa/core-console-backend-service
Copy link
Contributor

Choose a reason for hiding this comment

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

the 'core' comes from a different chart. would it make sense to parameterize it here as well instead of hardcoding?

Copy link
Contributor Author

@sayanh sayanh Mar 5, 2020 •
edited

Choose a reason for hiding this comment

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

Parameterized, the best I could but release core cannot be known to application-operator chart.

components/application-operator/charts/application/templates/policy.yaml Outdated Show resolved Hide resolved
bszwarc
bszwarc reviewed Mar 5, 2020
View reviewed changes
components/application-operator/README.md Outdated Show resolved Hide resolved
components/application-operator/README.md Outdated Show resolved Hide resolved
sayanh and others added 2 commits March 6, 2020 10:06
@sayanh @bszwarc
Update components/application-operator/README.md
9a3b91b 
Co-Authored-By: Barbara Szwarc <barbara.m.szwarc@gmail.com>
@sayanh
Merge branch 'master' of github.com:kyma-project/kyma into add-auth-p…
e9f143f 
…olicy

Conflicts:
components/application-operator/cmd/manager/manager.go
components/application-operator/cmd/manager/options.go
components/application-operator/pkg/kymahelm/application/applicationreleasemanager_test.go
components/application-operator/pkg/kymahelm/application/overrides.go
Szymongib
Szymongib reviewed Mar 6, 2020
View reviewed changes
Copy link
Contributor

@Szymongib Szymongib left a comment

Choose a reason for hiding this comment

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

I see on the image bump PR, that the strictMode is disabled, why is that?
Have you verified that the tests are passing if the strictMode is enabled?

components/application-operator/charts/application/templates/authorization-policy.yaml Show resolved Hide resolved
components/application-operator/charts/application/templates/authorization-policy.yaml Outdated
- /v1/health
selector:
matchLabels:
app: newapp-event-service
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be?

Suggested change
app: newapp-event-service
app: {{ .Release.Name }}-event-service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

components/application-operator/charts/application/templates/authorization-policy.yaml Show resolved Hide resolved
@sayanh
Copy link
Contributor Author

sayanh commented Mar 9, 2020 •
edited

@Szymongib

I see on the image bump PR, that the strictMode is disabled, why is that?

This is on purpose. Right now we wanna roll out strictMode as disabled. There are other components that need to be in strict mode. Details are on this issue.

We do have a plan to create jobs in Prow which will test the whole integration in strictMode.

Have you verified that the tests are passing if the strictMode is enabled?

Yes, I have verified the tests, which are running fine.

@sayanh sayanh merged commit 39d1ab8 into kyma-project:master Mar 18, 2020
@sayanh sayanh deleted the add-auth-policy branch March 18, 2020 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoineco antoineco antoineco approved these changes

@k15r k15r k15r approved these changes

@bszwarc bszwarc bszwarc approved these changes

@Szymongib Szymongib Szymongib approved these changes

@devguyio devguyio Awaiting requested review from devguyio

@akgalwas akgalwas Awaiting requested review from akgalwas

@alexandra-simeonova alexandra-simeonova Awaiting requested review from alexandra-simeonova

@anishj0shi anishj0shi Awaiting requested review from anishj0shi

@crabtree crabtree Awaiting requested review from crabtree

@franpog859 franpog859 Awaiting requested review from franpog859

@janmedrek janmedrek Awaiting requested review from janmedrek

@joek joek Awaiting requested review from joek

@kazydek kazydek Awaiting requested review from kazydek

@klaudiagrz klaudiagrz Awaiting requested review from klaudiagrz

@lilitgh lilitgh Awaiting requested review from lilitgh

@majakurcius majakurcius Awaiting requested review from majakurcius

@Maladie Maladie Awaiting requested review from Maladie

@marcobebway marcobebway Awaiting requested review from marcobebway

@mmitoraj mmitoraj Awaiting requested review from mmitoraj

@montaro montaro Awaiting requested review from montaro

@nachtmaar nachtmaar Awaiting requested review from nachtmaar

@tomekpapiernik tomekpapiernik Awaiting requested review from tomekpapiernik

Assignees
No one assigned
Labels
area/eventing Issues or PRs related to eventing area/security Issues or PRs related to security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sayanh @k15r @antoineco @bszwarc @Szymongib