Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

feat #524 Support mapping additional request paths to different upstream URLs #526

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chirino
Copy link
Contributor

@chirino chirino commented Apr 8, 2020

No description provided.

proxyServer := httptest.NewServer(proxy.router)
defer proxyServer.Close()

http.Get(proxyServer.URL + "/auth_all/white_listed/admin")

Choose a reason for hiding this comment

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

Error return value of http.Get is not checked (from errcheck)

http.Get(proxyServer.URL + "/auth_all/white_listed/admin")
require.Equal(t, []int64{0, 1, 0}, counters())

http.Get(proxyServer.URL + "/auth_all/white_listed/other")

Choose a reason for hiding this comment

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

Error return value of http.Get is not checked (from errcheck)

http.Get(proxyServer.URL + "/auth_all/white_listed/other")
require.Equal(t, []int64{1, 1, 0}, counters())

http.Get(proxyServer.URL + "/auth_all/white_listed/data")

Choose a reason for hiding this comment

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

Error return value of http.Get is not checked (from errcheck)

if err != nil {
return fmt.Errorf("invalid upstream-url-paths %s, %s", x, err)
}
config.UpstreamPaths = append(config.UpstreamPaths, path)

Choose a reason for hiding this comment

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

append only allowed to cuddle with appended value (from wsl)

@@ -220,6 +222,42 @@ func parseCLIOptions(cx *cli.Context, config *Config) (err error) {
config.Resources = append(config.Resources, resource)
}
}
if cx.IsSet("upstream-url-paths") {

Choose a reason for hiding this comment

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

if statements should only be cuddled with assignments (from wsl)

server.go Outdated
for _, x := range r.config.UpstreamPaths {
path := x
fmt.Printf("%s => %s\n", path.URL, path.Upstream)
upstreamUrl, err := url.Parse(path.Upstream)

Choose a reason for hiding this comment

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

var upstreamUrl should be upstreamURL (from golint)

// createUpstreamProxy create a reverse http proxy from the upstream
func (r *oauthProxy) createUpstreamProxy(upstream *url.URL) error {
func (r *oauthProxy) createUpstreamProxy(upstream *url.URL) (*goproxy.ProxyHttpServer, error) {

Choose a reason for hiding this comment

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

Function 'createUpstreamProxy' is too long (72 > 60) (from funlen)

return r, errors.New("config pair, should be (uri|upstream-url)=value")
}
switch kp[0] {
case "uri":

Choose a reason for hiding this comment

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

string uri has 2 occurrences, make it a constant (from goconst)


http.Get(proxyServer.URL + "/auth_all/white_listed/data")
require.Equal(t, []int64{1, 1, 1}, counters())

Choose a reason for hiding this comment

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

unnecessary trailing newline (from whitespace)

}
for _, x := range strings.Split(resource, "|") {
kp := strings.Split(x, "=")
if len(kp) != 2 {

Choose a reason for hiding this comment

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

mnd: Magic number: 2, in detected (from gomnd)

@@ -155,7 +155,7 @@ func createLogger(config *Config) (*zap.Logger, error) {
// createReverseProxy creates a reverse proxy
func (r *oauthProxy) createReverseProxy() error {
r.log.Info("enabled reverse proxy mode, upstream url", zap.String("url", r.config.Upstream))
if err := r.createUpstreamProxy(r.endpoint); err != nil {
if err := r.createDefaultUpstreamProxy(); err != nil {

Choose a reason for hiding this comment

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

if statements should only be cuddled with assignments (from wsl)

proxyServer := httptest.NewServer(proxy.router)
defer proxyServer.Close()

_, err = http.Get(proxyServer.URL + "/auth_all/white_listed/admin")

Choose a reason for hiding this comment

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

response body must be closed (from bodyclose)

require.NoError(t, err)
require.Equal(t, []int64{0, 1, 0}, counters())

_, err = http.Get(proxyServer.URL + "/auth_all/white_listed/other")

Choose a reason for hiding this comment

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

response body must be closed (from bodyclose)

require.NoError(t, err)
require.Equal(t, []int64{1, 1, 0}, counters())

_, err = http.Get(proxyServer.URL + "/auth_all/white_listed/data")

Choose a reason for hiding this comment

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

response body must be closed (from bodyclose)

@chirino
Copy link
Contributor Author

chirino commented Apr 8, 2020

all the white space reviews by the golangcibot are super annoying. Is there some CLI tool that could fix these automatically for me?

@stianst
Copy link
Contributor

stianst commented Apr 21, 2020

Could you provide some description of what this PR is about?

@chirino
Copy link
Contributor Author

chirino commented Apr 29, 2020

This PR allows one keycloack gatekeeper be the security gateway to multiple upstream http servers. This is a common setup when you app is broken down into multiple microservices which that all want to share the same security policies. It would not be very DRY to have to repeat gatekeeper configuration per microservice.

@abstractj abstractj added the status/discuss Needs discussion label Apr 29, 2020
@stianst
Copy link
Contributor

stianst commented Apr 29, 2020

The idea with Gatekeeper is that it should be deployed alongside the application, and should not be a gateway to multiple applications/services. If you deploy Gatekeeper separate you are opening up for applications to be accessed insecurely when someone gets inside the network boundaries.

@stianst
Copy link
Contributor

stianst commented Apr 29, 2020

For separate micro services these should also be using separate client-ids as you do not want a single token to be able to invoke all micro services, but rather control what can invoke what.

Copy link
Contributor

@stianst stianst left a comment

Choose a reason for hiding this comment

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

-1

@stianst
Copy link
Contributor

stianst commented Apr 29, 2020

That being said there is discussions and potential that we will allow plugin Gatekeeper into a general purpose proxy/gateway, such as HAProxy/OpenShift Router/Envoy, through a gRPC remoting interface.

@stianst
Copy link
Contributor

stianst commented Apr 30, 2020

We should continue the discussion on this one in the issue:
#524

@stianst stianst requested review from stianst and abstractj May 8, 2020 12:10
@stianst
Copy link
Contributor

stianst commented May 8, 2020

After thinking about this some more we should support this to make it easy to migrate from other proxies, then recommend using mtls to prevent ability to bypass security by invoking upstream directly.

@abstractj abstractj requested a review from JoelSpeed May 8, 2020 14:23
@abstractj abstractj self-assigned this May 8, 2020
@abstractj abstractj removed the status/discuss Needs discussion label May 8, 2020
@JoelSpeed
Copy link
Contributor

I haven't reviewed this fully yet, just a quick scan. I also don't have too much context on this project and how it's configured yet, but, from experience with OAuth2-Proxy, I would suggest we don't have a flag for this and instead force users to use a configuration file.

Overloading of strings in flags makes for complicated parsing code and also makes documenting the feature difficult. We have to make up our own formats for separating the different parts of the option and users will make mistakes configuring this that will be hard to understand.

I would much prefer to see this as a structured object within configuration only rather than a flag with complex parsing.

For context, I raised some discussion around this oauth2-proxy/oauth2-proxy#532 as the complexity of configuration has been one of the major problems with the project since I took it over

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

Successfully merging this pull request may close these issues.

5 participants