From 4e1e8a714adab6556dbbffad8457b2f5a2f27bba Mon Sep 17 00:00:00 2001 From: Livio Amstutz Date: Mon, 24 Aug 2020 10:06:55 +0200 Subject: [PATCH] fix: cors (#621) * fix: dont (re)generate client secret with auth type none * fix(cors): allow Origin from request * feat: add origin allow list and fix some core issues * rename migration * fix UserIDsByDomain * check origin on userinfo * update oidc pkg --- go.mod | 2 +- go.sum | 4 +-- internal/api/authz/context.go | 25 ++++++++++++++--- internal/api/authz/permissions_test.go | 4 +-- internal/api/authz/token.go | 6 ++--- internal/api/grpc/header.go | 5 ++++ internal/api/grpc/server/gateway.go | 2 +- .../middleware/auth_interceptor_test.go | 4 +-- internal/api/http/header.go | 3 +++ .../api/http/middleware/cors_interceptor.go | 7 +++-- internal/api/http/origin.go | 23 ++++++++++++++++ internal/api/oidc/client.go | 10 ++++++- .../eventstore/token_verifier.go | 9 ++++--- internal/project/model/application_view.go | 4 ++- .../repository/view/model/application.go | 27 +++++++++++++++++-- internal/static/i18n/de.yaml | 1 + internal/static/i18n/en.yaml | 3 ++- .../cockroach/V1.6__origin_allow_list.sql | 15 +++++++++++ 18 files changed, 128 insertions(+), 26 deletions(-) create mode 100644 internal/api/http/origin.go create mode 100644 migrations/cockroach/V1.6__origin_allow_list.sql diff --git a/go.mod b/go.mod index 0914053371..26a2787524 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,7 @@ require ( github.com/aws/aws-sdk-go v1.33.13 // indirect github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc github.com/caos/logging v0.0.2 - github.com/caos/oidc v0.7.1 + github.com/caos/oidc v0.7.2 github.com/census-instrumentation/opencensus-proto v0.3.0 // indirect github.com/cockroachdb/cockroach-go/v2 v2.0.5 github.com/envoyproxy/protoc-gen-validate v0.4.0 diff --git a/go.sum b/go.sum index 05f198201a..c0c802a05d 100644 --- a/go.sum +++ b/go.sum @@ -71,8 +71,8 @@ github.com/boombuler/barcode v1.0.1-0.20190219062509-6c824513bacc/go.mod h1:paBW github.com/caos/logging v0.0.0-20191210002624-b3260f690a6a/go.mod h1:9LKiDE2ChuGv6CHYif/kiugrfEXu9AwDiFWSreX7Wp0= github.com/caos/logging v0.0.2 h1:ebg5C/HN0ludYR+WkvnFjwSExF4wvyiWPyWGcKMYsoo= github.com/caos/logging v0.0.2/go.mod h1:9LKiDE2ChuGv6CHYif/kiugrfEXu9AwDiFWSreX7Wp0= -github.com/caos/oidc v0.7.1 h1:/Snk5rM43h98kh4S57etnqfherH/o98O0djQxnY+9y0= -github.com/caos/oidc v0.7.1/go.mod h1:mnuSyFmv+WSuk2C/zps445xiMU9dW384/pV4WnIS8b0= +github.com/caos/oidc v0.7.2 h1:tt6acMhNIhnlU+BhiNMYY00uhNL0vniiEwVKpWh8SMs= +github.com/caos/oidc v0.7.2/go.mod h1:mnuSyFmv+WSuk2C/zps445xiMU9dW384/pV4WnIS8b0= github.com/census-instrumentation/opencensus-proto v0.2.1 h1:glEXhBS5PSLLv4IXzLA5yPRVX4bilULVyxxbrfOtDAk= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= diff --git a/internal/api/authz/context.go b/internal/api/authz/context.go index ba850a719e..7d1362940b 100644 --- a/internal/api/authz/context.go +++ b/internal/api/authz/context.go @@ -2,9 +2,10 @@ package authz import ( "context" - "github.com/caos/zitadel/internal/errors" - "github.com/caos/logging" + "github.com/caos/zitadel/internal/api/grpc" + http_util "github.com/caos/zitadel/internal/api/http" + "github.com/caos/zitadel/internal/errors" ) type key int @@ -46,8 +47,13 @@ func VerifyTokenAndWriteCtxData(ctx context.Context, token, orgID string, t *Tok if err != nil { return nil, err } - projectID, err := t.GetProjectIDByClientID(ctx, clientID) - logging.LogWithFields("AUTH-GfAoV", "clientID", clientID).OnError(err).Warn("could not read projectid by clientid") + projectID, origins, err := t.ProjectIDAndOriginsByClientID(ctx, clientID) + if err != nil { + return nil, errors.ThrowPermissionDenied(err, "AUTH-GHpw2", "could not read projectid by clientid") + } + if err := checkOrigin(ctx, origins); err != nil { + return nil, err + } return context.WithValue(ctx, dataKey, CtxData{UserID: userID, OrgID: orgID, ProjectID: projectID, AgentID: agentID}), nil } @@ -69,3 +75,14 @@ func GetAllPermissionsFromCtx(ctx context.Context) []string { ctxPermission, _ := ctx.Value(allPermissionsKey).([]string) return ctxPermission } + +func checkOrigin(ctx context.Context, origins []string) error { + origin := grpc.GetGatewayHeader(ctx, http_util.Origin) + if origin == "" { + return nil + } + if http_util.IsOriginAllowed(origins, origin) { + return nil + } + return errors.ThrowPermissionDenied(nil, "AUTH-DZG21", "Errors.OriginNotAllowed") +} diff --git a/internal/api/authz/permissions_test.go b/internal/api/authz/permissions_test.go index 5339bc3f9e..32347fd236 100644 --- a/internal/api/authz/permissions_test.go +++ b/internal/api/authz/permissions_test.go @@ -23,8 +23,8 @@ func (v *testVerifier) ResolveGrants(ctx context.Context) (*Grant, error) { return v.grant, nil } -func (v *testVerifier) ProjectIDByClientID(ctx context.Context, clientID string) (string, error) { - return "", nil +func (v *testVerifier) ProjectIDAndOriginsByClientID(ctx context.Context, clientID string) (string, []string, error) { + return "", nil, nil } func (v *testVerifier) ExistsOrg(ctx context.Context, orgID string) error { diff --git a/internal/api/authz/token.go b/internal/api/authz/token.go index 4044216a79..fca6111490 100644 --- a/internal/api/authz/token.go +++ b/internal/api/authz/token.go @@ -22,7 +22,7 @@ type authZRepo interface { VerifyAccessToken(ctx context.Context, token, clientID string) (userID, agentID string, err error) VerifierClientID(ctx context.Context, name string) (clientID string, err error) ResolveGrants(ctx context.Context) (grant *Grant, err error) - ProjectIDByClientID(ctx context.Context, clientID string) (projectID string, err error) + ProjectIDAndOriginsByClientID(ctx context.Context, clientID string) (projectID string, origins []string, err error) ExistsOrg(ctx context.Context, orgID string) error } @@ -88,8 +88,8 @@ func (v *TokenVerifier) ResolveGrant(ctx context.Context) (*Grant, error) { return v.authZRepo.ResolveGrants(ctx) } -func (v *TokenVerifier) GetProjectIDByClientID(ctx context.Context, clientID string) (string, error) { - return v.authZRepo.ProjectIDByClientID(ctx, clientID) +func (v *TokenVerifier) ProjectIDAndOriginsByClientID(ctx context.Context, clientID string) (string, []string, error) { + return v.authZRepo.ProjectIDAndOriginsByClientID(ctx, clientID) } func (v *TokenVerifier) ExistsOrg(ctx context.Context, orgID string) error { diff --git a/internal/api/grpc/header.go b/internal/api/grpc/header.go index f80b90800e..286724c8bd 100644 --- a/internal/api/grpc/header.go +++ b/internal/api/grpc/header.go @@ -4,6 +4,7 @@ import ( "context" "github.com/grpc-ecosystem/go-grpc-middleware/util/metautils" + "github.com/grpc-ecosystem/grpc-gateway/runtime" "github.com/caos/zitadel/internal/api/http" ) @@ -12,6 +13,10 @@ func GetHeader(ctx context.Context, headername string) string { return metautils.ExtractIncoming(ctx).Get(headername) } +func GetGatewayHeader(ctx context.Context, headername string) string { + return GetHeader(ctx, runtime.MetadataPrefix+headername) +} + func GetAuthorizationHeader(ctx context.Context) string { return GetHeader(ctx, http.Authorization) } diff --git a/internal/api/grpc/server/gateway.go b/internal/api/grpc/server/gateway.go index 5a48f02e18..bf8083fba2 100644 --- a/internal/api/grpc/server/gateway.go +++ b/internal/api/grpc/server/gateway.go @@ -131,7 +131,7 @@ func addInterceptors(handler http.Handler, g Gateway) http.Handler { if interceptor, ok := g.(grpcGatewayCustomInterceptor); ok { handler = interceptor.GatewayHTTPInterceptor(handler) } - return http_mw.CORSInterceptorOpts(http_mw.DefaultCORSOptions, handler) + return http_mw.CORSInterceptor(handler) } func gatewayPort(port string) string { diff --git a/internal/api/grpc/server/middleware/auth_interceptor_test.go b/internal/api/grpc/server/middleware/auth_interceptor_test.go index 6cea063535..fb095a71a7 100644 --- a/internal/api/grpc/server/middleware/auth_interceptor_test.go +++ b/internal/api/grpc/server/middleware/auth_interceptor_test.go @@ -27,8 +27,8 @@ func (v *verifierMock) VerifyAccessToken(ctx context.Context, token, clientID st func (v *verifierMock) ResolveGrants(ctx context.Context) (*authz.Grant, error) { return nil, nil } -func (v *verifierMock) ProjectIDByClientID(ctx context.Context, clientID string) (string, error) { - return "", nil +func (v *verifierMock) ProjectIDAndOriginsByClientID(ctx context.Context, clientID string) (string, []string, error) { + return "", nil, nil } func (v *verifierMock) ExistsOrg(ctx context.Context, orgID string) error { return nil diff --git a/internal/api/http/header.go b/internal/api/http/header.go index 52c46fea81..a0676a3094 100644 --- a/internal/api/http/header.go +++ b/internal/api/http/header.go @@ -13,12 +13,15 @@ const ( AcceptLanguage = "accept-language" CacheControl = "cache-control" ContentType = "content-type" + ContentLength = "content-length" Expires = "expires" Location = "location" Origin = "origin" Pragma = "pragma" UserAgentHeader = "user-agent" ForwardedFor = "x-forwarded-for" + XUserAgent = "x-user-agent" + XGrpcWeb = "x-grpc-web" ContentSecurityPolicy = "content-security-policy" XXSSProtection = "x-xss-protection" diff --git a/internal/api/http/middleware/cors_interceptor.go b/internal/api/http/middleware/cors_interceptor.go index e3e56b7a90..adf759174d 100644 --- a/internal/api/http/middleware/cors_interceptor.go +++ b/internal/api/http/middleware/cors_interceptor.go @@ -18,6 +18,8 @@ var ( http_utils.AcceptLanguage, http_utils.Authorization, http_utils.ZitadelOrgID, + http_utils.XUserAgent, + http_utils.XGrpcWeb, }, AllowedMethods: []string{ http.MethodOptions, @@ -30,9 +32,10 @@ var ( }, ExposedHeaders: []string{ http_utils.Location, + http_utils.ContentLength, }, - AllowedOrigins: []string{ - "http://localhost:*", + AllowOriginFunc: func(_ string) bool { + return true }, } ) diff --git a/internal/api/http/origin.go b/internal/api/http/origin.go new file mode 100644 index 0000000000..44f2227900 --- /dev/null +++ b/internal/api/http/origin.go @@ -0,0 +1,23 @@ +package http + +import ( + "fmt" + "net/url" +) + +func GetOriginFromURLString(s string) (string, error) { + parsed, err := url.Parse(s) + if err != nil { + return "", err + } + return fmt.Sprintf("%s://%s", parsed.Scheme, parsed.Host), nil +} + +func IsOriginAllowed(allowList []string, origin string) bool { + for _, allowed := range allowList { + if allowed == origin { + return true + } + } + return false +} diff --git a/internal/api/oidc/client.go b/internal/api/oidc/client.go index afbcb96202..a18b495d3c 100644 --- a/internal/api/oidc/client.go +++ b/internal/api/oidc/client.go @@ -7,6 +7,7 @@ import ( "github.com/caos/oidc/pkg/op" "github.com/caos/zitadel/internal/api/authz" + "github.com/caos/zitadel/internal/api/http" "github.com/caos/zitadel/internal/errors" proj_model "github.com/caos/zitadel/internal/project/model" user_model "github.com/caos/zitadel/internal/user/model" @@ -41,11 +42,18 @@ func (o *OPStorage) AuthorizeClientIDSecret(ctx context.Context, id string, secr return o.repo.AuthorizeOIDCApplication(ctx, id, secret) } -func (o *OPStorage) GetUserinfoFromToken(ctx context.Context, tokenID string) (*oidc.Userinfo, error) { +func (o *OPStorage) GetUserinfoFromToken(ctx context.Context, tokenID, origin string) (*oidc.Userinfo, error) { token, err := o.repo.TokenByID(ctx, tokenID) if err != nil { return nil, err } + app, err := o.repo.ApplicationByClientID(ctx, token.ApplicationID) + if err != nil { + return nil, err + } + if origin != "" && !http.IsOriginAllowed(app.OriginAllowList, origin) { + return nil, errors.ThrowPermissionDenied(nil, "OIDC-da1f3", "origin is not allowed") + } return o.GetUserinfoFromScopes(ctx, token.UserID, token.Scopes) } diff --git a/internal/authz/repository/eventsourcing/eventstore/token_verifier.go b/internal/authz/repository/eventsourcing/eventstore/token_verifier.go index 9c964cf069..ad7750f951 100644 --- a/internal/authz/repository/eventsourcing/eventstore/token_verifier.go +++ b/internal/authz/repository/eventsourcing/eventstore/token_verifier.go @@ -2,12 +2,13 @@ package eventstore import ( "context" + "time" + "github.com/caos/zitadel/internal/authz/repository/eventsourcing/view" "github.com/caos/zitadel/internal/crypto" caos_errs "github.com/caos/zitadel/internal/errors" iam_event "github.com/caos/zitadel/internal/iam/repository/eventsourcing" proj_event "github.com/caos/zitadel/internal/project/repository/eventsourcing" - "time" ) type TokenVerifierRepo struct { @@ -40,12 +41,12 @@ func (repo *TokenVerifierRepo) VerifyAccessToken(ctx context.Context, tokenStrin return "", "", caos_errs.ThrowUnauthenticated(nil, "APP-Zxfako", "invalid audience") } -func (repo *TokenVerifierRepo) ProjectIDByClientID(ctx context.Context, clientID string) (projectID string, err error) { +func (repo *TokenVerifierRepo) ProjectIDAndOriginsByClientID(ctx context.Context, clientID string) (projectID string, origins []string, err error) { app, err := repo.View.ApplicationByOIDCClientID(clientID) if err != nil { - return "", err + return "", nil, err } - return app.ProjectID, nil + return app.ProjectID, app.OriginAllowList, nil } func (repo *TokenVerifierRepo) ExistsOrg(ctx context.Context, orgID string) error { diff --git a/internal/project/model/application_view.go b/internal/project/model/application_view.go index 5713e2f6f1..778d1f7293 100644 --- a/internal/project/model/application_view.go +++ b/internal/project/model/application_view.go @@ -1,8 +1,9 @@ package model import ( - "github.com/caos/zitadel/internal/model" "time" + + "github.com/caos/zitadel/internal/model" ) type ApplicationView struct { @@ -25,6 +26,7 @@ type ApplicationView struct { NoneCompliant bool ComplianceProblems []string DevMode bool + OriginAllowList []string Sequence uint64 } diff --git a/internal/project/repository/view/model/application.go b/internal/project/repository/view/model/application.go index 97dad40b3c..c26acbd6ce 100644 --- a/internal/project/repository/view/model/application.go +++ b/internal/project/repository/view/model/application.go @@ -2,13 +2,16 @@ package model import ( "encoding/json" + "time" + "github.com/caos/logging" + "github.com/lib/pq" + + http_util "github.com/caos/zitadel/internal/api/http" caos_errs "github.com/caos/zitadel/internal/errors" "github.com/caos/zitadel/internal/eventstore/models" "github.com/caos/zitadel/internal/project/model" es_model "github.com/caos/zitadel/internal/project/repository/eventsourcing/model" - "github.com/lib/pq" - "time" ) const ( @@ -39,6 +42,7 @@ type ApplicationView struct { NoneCompliant bool `json:"-" gorm:"column:none_compliant"` ComplianceProblems pq.StringArray `json:"-" gorm:"column:compliance_problems"` DevMode bool `json:"devMode" gorm:"column:dev_mode"` + OriginAllowList pq.StringArray `json:"-" gorm:"column:origin_allow_list"` Sequence uint64 `json:"-" gorm:"sequence"` } @@ -62,6 +66,7 @@ func ApplicationViewFromModel(app *model.ApplicationView) *ApplicationView { OIDCAuthMethodType: int32(app.OIDCAuthMethodType), OIDCPostLogoutRedirectUris: app.OIDCPostLogoutRedirectUris, DevMode: app.DevMode, + OriginAllowList: app.OriginAllowList, } } @@ -103,6 +108,7 @@ func ApplicationViewToModel(app *ApplicationView) *model.ApplicationView { NoneCompliant: app.NoneCompliant, ComplianceProblems: app.ComplianceProblems, DevMode: app.DevMode, + OriginAllowList: app.OriginAllowList, } } @@ -171,6 +177,7 @@ func (a *ApplicationView) AppendEvent(event *models.Event) (err error) { return err } a.setCompliance() + return a.setOriginAllowList() case es_model.OIDCConfigChanged, es_model.ApplicationChanged: err = a.SetData(event) @@ -178,6 +185,7 @@ func (a *ApplicationView) AppendEvent(event *models.Event) (err error) { return err } a.setCompliance() + return a.setOriginAllowList() case es_model.ApplicationDeactivated: a.State = int32(model.AppStateInactive) case es_model.ApplicationReactivated: @@ -200,6 +208,21 @@ func (a *ApplicationView) SetData(event *models.Event) error { return nil } +func (a *ApplicationView) setOriginAllowList() error { + allowList := make([]string, 0) + for _, redirect := range a.OIDCRedirectUris { + origin, err := http_util.GetOriginFromURLString(redirect) + if err != nil { + return err + } + if !http_util.IsOriginAllowed(allowList, origin) { + allowList = append(allowList, origin) + } + } + a.OriginAllowList = allowList + return nil +} + func (a *ApplicationView) setCompliance() { compliance := model.GetOIDCCompliance(model.OIDCVersion(a.OIDCVersion), model.OIDCApplicationType(a.OIDCApplicationType), OIDCGrantTypesToModel(a.OIDCGrantTypes), OIDCResponseTypesToModel(a.OIDCResponseTypes), model.OIDCAuthMethodType(a.OIDCAuthMethodType), a.OIDCRedirectUris) a.NoneCompliant = compliance.NoneCompliant diff --git a/internal/static/i18n/de.yaml b/internal/static/i18n/de.yaml index 6c889f6194..7bba721a7a 100644 --- a/internal/static/i18n/de.yaml +++ b/internal/static/i18n/de.yaml @@ -1,6 +1,7 @@ Errors: Internal: Es ist ein interner Fehler aufgetreten NoChangesFound: Keine Änderungen gefunden + OriginNotAllowed: Dieser "Origin" ist nicht freigeschaltet User: NotFound: Benutzer konnte nicht gefunden werden UserIDMissing: User ID fehlt diff --git a/internal/static/i18n/en.yaml b/internal/static/i18n/en.yaml index e1092c33d0..cfe6ac63ce 100644 --- a/internal/static/i18n/en.yaml +++ b/internal/static/i18n/en.yaml @@ -1,6 +1,7 @@ Errors: Internal: An internal error occured - NoChangesFound: No changes found + NoChangesFound: No changes + OriginNotAllowed: This "Origin" is not allowed User: NotFound: User could not be found UserIDMissing: User ID missing diff --git a/migrations/cockroach/V1.6__origin_allow_list.sql b/migrations/cockroach/V1.6__origin_allow_list.sql new file mode 100644 index 0000000000..7589e8cb9f --- /dev/null +++ b/migrations/cockroach/V1.6__origin_allow_list.sql @@ -0,0 +1,15 @@ +BEGIN; + +ALTER TABLE management.applications ADD COLUMN origin_allow_list TEXT ARRAY; +ALTER TABLE auth.applications ADD COLUMN origin_allow_list TEXT ARRAY; +ALTER TABLE authz.applications ADD COLUMN origin_allow_list TEXT ARRAY; + +TRUNCATE TABLE management.applications; +TRUNCATE TABLE auth.applications; +TRUNCATE TABLE authz.applications; + +UPDATE management.current_sequences set current_sequence = 0 where view_name = 'management.applications'; +UPDATE auth.current_sequences set current_sequence = 0 where view_name = 'auth.applications'; +UPDATE authz.current_sequences set current_sequence = 0 where view_name = 'authz.applications'; + +COMMIT; \ No newline at end of file