Skip to content

Add session gateway#84

Merged
merenbach merged 4 commits intoargoproj:masterfrom
merenbach:add-session-gateway
Apr 13, 2018
Merged

Add session gateway#84
merenbach merged 4 commits intoargoproj:masterfrom
merenbach:add-session-gateway

Conversation

@merenbach
Copy link
Contributor

Background

To follow on #82, and in an effort to address #72 and #29, this PR implements the passing of credentials to the backend to support access control.

Implementation

  1. When a token is not provided, gate off all backend access. This requires tokens (plural) in metadata from CLI client (it gets converted to a list, so just made it plural); or Grpc-Metadata-Tokens: INSERT_JWT_HERE in all HTTP headers (except for login!).

  2. Replace context.Background() calls with our own new DefaultClientContext(clientOpts), which injects the token (if it exists) into the metadata passed to gRPC on the backend. If it doesn't exist, an access denied error will appear in the command line.

  3. Nothing involving server-side cookie setting has been implemented at this time. Full support for Web access can be implemented with just JS right now by setting request headers (i.e., Grpc-Metadata-Tokens) and storing in cookie client-side. cc @alexmt interested in your thoughts.

Important

This will break the Web client unless a token is retrieved and passed along with requests.

@merenbach merenbach added the help-wanted Extra attention is needed label Apr 11, 2018
@merenbach merenbach requested review from alexmt and jessesuen April 11, 2018 20:36
@merenbach merenbach removed the help-wanted Extra attention is needed label Apr 12, 2018
@jessesuen
Copy link
Member

Could you rebase the changes onto master so we can see just the delta between this and the argo login changes?

@merenbach
Copy link
Contributor Author

@jessesuen rebased on to master.

server/server.go Outdated
return ctx, fmt.Errorf("user is not allowed access")
}

return ctx, fmt.Errorf("empty metadata")
Copy link
Member

Choose a reason for hiding this comment

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

This interceptor should return grpc errors instead of generic go errors. Specifically it should be:

grpc.Errorf(codes.Unauthenticated, message)

}

// AuthFuncOverride overrides the authentication function and let us not require auth to receive auth.
func (s *Server) AuthFuncOverride(ctx context.Context, fullMethodName string) (context.Context, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where this method is used?

conn, appIf := argocdclient.NewClientOrDie(clientOpts).NewApplicationClientOrDie()
defer util.Close(conn)
_, err := appIf.Create(context.Background(), &app)
_, err := appIf.Create(DefaultClientContext(clientOpts), &app)
Copy link
Member

Choose a reason for hiding this comment

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

Wrapping the context in every place where we make an gRPC call isn't the right approach. There is a common function in apiclient.go, NewConn() where you can adjust the DialOptions when establishing the connection. There is where we would want to add another dial option from grpc.WithPerRPCCredentials() with the token.

See this example:
grpc/grpc-go#106 (comment)

@alexmt
Copy link
Collaborator

alexmt commented Apr 12, 2018

LGTM

@merenbach
Copy link
Contributor Author

@jessesuen @alexmt I've added a feature flag in the environment to disable authentication checks by default (so as not to break existing workflows).

To run with this flag enabled and auth required, modify the api-server invocation in the Procfile as follows:

api-server: env REQUIREAUTH=1 go run ./cmd/argocd-server/main.go --insecure

Setting REQUIREAUTH=1 in a production environment would also work, in theory, although this is intended to be temporary. Please let me know your thoughts.

@jessesuen
Copy link
Member

Hey sorry I haven't had time to review latest changes, but will do so now. The feature flag is a good idea, especially since UI needs to catch up.

func endpointCredentials(endpoint string) jwtCredentials {
credentials := jwtCredentials{}

localConfig, err := config_util.ReadLocalConfig()
Copy link
Member

Choose a reason for hiding this comment

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

I won't gate this checkin, but I think we need to move the reading/parsing of the local config, tobe done when creating the API client. A use case is that I should be able to create an argocd client without having an .argocd directory. In this case I would supply an API token when instantiating the client.

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

My only comment is to have other ways of creating a client without requiring there exists .argocd directory (so long as a token is supplied). This can be addressed in a subsequent checkin.

@merenbach merenbach merged commit 6c41ce5 into argoproj:master Apr 13, 2018
@merenbach merenbach deleted the add-session-gateway branch May 7, 2018 17:00
alexec pushed a commit that referenced this pull request Apr 24, 2019
sujeilyfonseca added a commit to sujeilyfonseca/argo-cd that referenced this pull request Dec 15, 2022
Addressed the security vulnerabilities reported
by the Twistlock security scan.

Contributes to: automation-saas/native-AWS#2815

Signed-off-by: Sujeily Fonseca <sujeily.fonseca@ibm.com>
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Sep 29, 2023
alexmt pushed a commit to alexmt/argo-cd that referenced this pull request Jan 13, 2026
Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
alexmt pushed a commit to alexmt/argo-cd that referenced this pull request Mar 21, 2026
Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
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.

3 participants