Add support for Application ID auth pattern #58

philipgough · 2019-02-12T13:01:05Z

Moves to 1.1 codebase

Post review/approval

Docs update
Modify sample templates

TODO - Verification Steps

Test User Key headers and query params
Test User Key headers and query params individual
Test APP ID headers and query params
Test APP ID headers and query params individual
Test App Id with App Key
Test App Id without key

These overrides are required as a fix to workaround istio/istio#11226 dep: Migrate to latest revision of istio 1.1

codecov-io · 2019-02-12T13:27:05Z

Codecov Report

Merging #58 into master will increase coverage by 1.14%.
The diff coverage is 93.18%.

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   85.61%   86.75%   +1.14%     
==========================================
  Files           3        3              
  Lines         438      438              
==========================================
+ Hits          375      380       +5     
+ Misses         40       37       -3     
+ Partials       23       21       -2

Impacted Files	Coverage Δ
pkg/threescale/threescale.go	`84.35% <93.18%> (+2.79%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 630fa40...e83a665. Read the comment docs.

unleashed

Check comments - since we will be detecting the authn used, we want to be lenient about existing parameters (ie. ignoring them) and use a specific priority order.

Gopkg.toml

unleashed · 2019-02-12T13:51:00Z

testdata/threescale-adapter-config.yaml

+      user: request.query_params["user_key"] | request.headers["x-user-key"] | request.api_key | ""
+      properties:
+        app_id: request.query_params["app_id"] | request.headers["x-app-id"] | ""
+        app_key: request.query_params["app_key"] | request.headers["x-app-key"] | ""


we've been talking about this, but just to rule out this last thought of mine out of being a bit naive: setting here a property like query: request.query_params (the whole map) does not work, does it?

I couldn't get that to work. Thats what I thought initially. Cant seem to have nested maps. I will try out some other stuff in the mean time but it wont be hard to go from this to a working alternative.

we could be interested in knowing whether these parameters are empty or they weren't specified, perhaps we can leave out the "" and test for nil?

(for a bit of context, there is in 3scale, under certain conditions, a semantic difference between specifying an empty app_key to the Service Management API and simply not specifying it at all)

I don't think that's going to be achievable with the current client. The default string value in go is "".

When we create the params, even if we don't set it it will become empty string. I think would need to go ahead and use pointers to strings in the client to get this behaviour.

Happy to create an issue on it and fix, but my suggestion is leave this as is for now, wdyt?

testdata/threescale-adapter-config.yaml

unleashed · 2019-02-12T14:02:11Z

pkg/threescale/types.go

+	server   *grpc.Server
+	client   *http.Client
+	conf     *AdapterConfig
+}


Not sure if it's more idiomatic to group these together in golang, I personally prefer to have the definition next to the implementation of methods, perhaps one struct + methods per file (as in Java) to have all the information together for easy reference, but I don't have a strong opinion in this particular case (just FYI in case you didn't consider alternatives).

Well this is not uncommon in go. Even the stdlib uses types.go files. The reason I have done this is looking ahead slightly.

I spoke to @jmprusi who has a use cause for decoupling the actual so I expect some refactoring, but I'm ok with putting these back where they were if you like. Once the type methods aren't spread across files I'm fine either way.

No, it's ok this way, I just FYI'ed it. :)

Decoupling this from its context... well, it's going to cost a bit. Let @jmprusi volunteer to extract the logic out and make it available for us as a shared library. :D

@jmprusi has just been volunteered. Thanks @jmprusi 👍 🥇

pkg/threescale/threescale_test.go

unleashed · 2019-02-12T14:14:04Z

pkg/threescale/threescale_integration_test.go

+					},
+				},
+			},
+			expect: generatedExpectedError(t, rpc.PERMISSION_DENIED, authMismatchErr),


I'd say we prefer this to be "I will use user_key=MATCH and ignore app_key".
We can define this order:

Presence of user_key? => process user_key, goto 3

Presence of app_id? => goto 2.a
2.a Presence of app_key? => process app_id with app_key, goto 3
2.b Process app_id without app_key

Done.

Of course, if we knew we were supposed to only be using app_id/app_key, we'd be ignoring the other steps (ie. 1).

pkg/threescale/threescale.go

unleashed · 2019-02-12T14:22:16Z

pkg/threescale/threescale.go

+			Value: proxyConf.Content.BackendAuthenticationValue,
+		}}
+
+	if request.Subject != nil {


if Subject == nil we can probably error out earlier, before matching rules, right?

True, Again this is just covering a messed up config and avoiding a panic but yes, I'll move this to fail early

pkg/threescale/threescale.go

unleashed

Looking better, check comments.

Gopkg.toml

pkg/threescale/threescale.go

unleashed · 2019-02-13T11:43:05Z

testdata/threescale-adapter-config.yaml

+      user: request.query_params["user_key"] | request.headers["x-user-key"] | request.api_key | ""
+      properties:
+        app_id: request.query_params["app_id"] | request.headers["x-app-id"] | ""
+        app_key: request.query_params["app_key"] | request.headers["x-app-key"] | ""


we could be interested in knowing whether these parameters are empty or they weren't specified, perhaps we can leave out the "" and test for nil?

unleashed · 2019-02-13T11:46:15Z

testdata/threescale-adapter-config.yaml

+      user: request.query_params["user_key"] | request.headers["x-user-key"] | request.api_key | ""
+      properties:
+        app_id: request.query_params["app_id"] | request.headers["x-app-id"] | ""
+        app_key: request.query_params["app_key"] | request.headers["x-app-key"] | ""


(for a bit of context, there is in 3scale, under certain conditions, a semantic difference between specifying an empty app_key to the Service Management API and simply not specifying it at all)

README.md

unleashed

Looks good, check comments for minor things to correct.

unleashed · 2019-02-13T14:34:08Z

README.md

+Now that the we have [configured the service to be managed by 3scale](#routing-service-traffic-through-the-adapter) we can decide how requests should be authenticated.
+Currently there are two supported mechanisms:
+1. The API Key authentication pattern
+1. The Application ID authentication pattern


Probably mention the "pair" of App ID and App Key, as it is referred to like this in docs. Also, this should be 2.

unleashed · 2019-02-13T14:37:00Z

README.md

+      method: request.method | "get"
+```
+
+#### Hybrid Pattern


Gopkg.toml

This change updates the handler to support an additional authentication method. The instance will be configured by the user and the adapter parses the request to determine what authentication method should be used and in turn decides on the 3scale backend API which will be called.

…patterns

This change alerts the operator to the fact that the config has no changed to use features from Istio version 1.1 which becomes a prerequisite. Also included are updates to the README to describe how to configure the various authentication patterns and how to adjust the config to enforce various authn methods.

…of v0.3.0

philipgough force-pushed the 1.1-migration branch from e2d4d3c to 7346a69 Compare February 12, 2019 13:14

philipgough requested review from unleashed and jmprusi February 12, 2019 13:14

philipgough added 2 commits February 12, 2019 13:25

dep: Override subpackages

42ba71b

These overrides are required as a fix to workaround istio/istio#11226 dep: Migrate to latest revision of istio 1.1

config: Updates testdata with 1.1 attribute definitions

0dbdaf1

philipgough force-pushed the 1.1-migration branch from 7346a69 to bca4a4a Compare February 12, 2019 13:26

philipgough mentioned this pull request Feb 12, 2019

WIP: 1.1 code #56

Closed

unleashed suggested changes Feb 12, 2019

View reviewed changes

philipgough force-pushed the 1.1-migration branch from bca4a4a to 73d2f92 Compare February 12, 2019 15:06

adapter: Moves type definitions into their own file

4008f98

philipgough force-pushed the 1.1-migration branch from 73d2f92 to fddb161 Compare February 12, 2019 15:38

unleashed reviewed Feb 13, 2019

View reviewed changes

philipgough force-pushed the 1.1-migration branch from bab979e to f35ba2b Compare February 13, 2019 12:23

philipgough commented Feb 13, 2019

View reviewed changes

README.md Show resolved Hide resolved

unleashed approved these changes Feb 13, 2019

View reviewed changes

philipgough added 2 commits February 13, 2019 15:26

config: Updates testdata with 1.1 attribute definitions

add88ca

test: Update integration tests to reflect changes in testdata config

2d7fd2b

philipgough force-pushed the 1.1-migration branch 2 times, most recently from 669aced to ed3ae64 Compare February 13, 2019 16:21

philipgough added 2 commits February 13, 2019 16:38

test: Update unit tests with working implementation of multiple auth …

3e73001

…patterns

philipgough force-pushed the 1.1-migration branch from ed3ae64 to 434fc2e Compare February 13, 2019 16:41

philipgough added 3 commits February 13, 2019 20:04

config: Updates sample resource definitions

44b1349

versioning: Bump version in Makefile and Deployment prior to realese …

e83a665

…of v0.3.0

philipgough force-pushed the 1.1-migration branch from 434fc2e to e83a665 Compare February 14, 2019 08:06

philipgough merged commit 66816d2 into master Feb 14, 2019

philipgough deleted the 1.1-migration branch February 14, 2019 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Application ID auth pattern #58

Add support for Application ID auth pattern #58

philipgough commented Feb 12, 2019 •

edited

Loading

codecov-io commented Feb 12, 2019 •

edited

Loading

unleashed left a comment

unleashed Feb 12, 2019

philipgough Feb 12, 2019

unleashed Feb 13, 2019

unleashed Feb 13, 2019

philipgough Feb 13, 2019

unleashed Feb 12, 2019

philipgough Feb 12, 2019 •

edited

Loading

unleashed Feb 12, 2019 •

edited

Loading

philipgough Feb 12, 2019

unleashed Feb 12, 2019

unleashed Feb 12, 2019

philipgough Feb 12, 2019

philipgough Feb 13, 2019

unleashed left a comment

unleashed Feb 13, 2019

unleashed Feb 13, 2019

unleashed left a comment

unleashed Feb 13, 2019

unleashed Feb 13, 2019

Add support for Application ID auth pattern #58

Add support for Application ID auth pattern #58

Conversation

philipgough commented Feb 12, 2019 • edited Loading

Post review/approval

TODO - Verification Steps

codecov-io commented Feb 12, 2019 • edited Loading

Codecov Report

unleashed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipgough Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

unleashed Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unleashed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unleashed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipgough commented Feb 12, 2019 •

edited

Loading

codecov-io commented Feb 12, 2019 •

edited

Loading

philipgough Feb 12, 2019 •

edited

Loading

unleashed Feb 12, 2019 •

edited

Loading