Wellknown #427

pbarker · 2017-04-17T17:14:48Z

Adds discovery endpoints that were started in branch fix-379. Tested an working, opening this as a WIP, let me know if there is anything you want added/changed. Thanks

aeneasr

Awesome, this looks rock solid! I'll take another review pass later (or latest tomorrow). Thank you!

aeneasr · 2017-04-17T17:34:07Z

oauth2/handler.go

+}
+
+func (h *Handler) WellKnownHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+	fmt.Printf("handler: %+v", h)


Please remove this

pbarker · 2017-04-17T20:36:08Z

cmd/server/handler_oauth2_factory.go

@@ -155,6 +155,8 @@ func newOAuth2Handler(c *config.Config, router *httprouter.Router, km jwk.Manage
 		H:                   &herodot.JSON{},
 		AccessTokenLifespan: c.GetAccessTokenLifespan(),
 		CookieStore:         sessions.NewCookieStore(c.GetCookieSecret()),
+		Issuer:              c.Issuer,
+		BaseURL:             c.ClusterURL,


This was the one part I was unsure of, where should the BaseURL come from? Issuer might actually make the most sense

Hm, good question, this was actually a problem previously. The cluster url is usually set on the client side, not the server side. We could either use the issuer url, or make it easier to understand by renaming ISSUER env var to something like PUBLIC_URL. what do you think?

Sorry by the way for taking so long on responding, the last days were really busy.

I think Issuer is fine actually, thats what its generally called by most implementers. I suppose I should add a section to the docs that emphasizes its use. Would it be ok to remove BaseURL then?

Yes, sounds good!

Besides adding it in the docs, also make the usage clear in hydra help host :)

resolved

pbarker · 2017-04-26T16:05:23Z

@arekkas this guy is good to go, I'm getting blocked on the coveralls, in order to increase the code coverage we would have to implement HTTP tests, which seems non-trivial. Let me know how you want to proceed here

aeneasr · 2017-04-27T09:07:35Z

Any idea why travis is failing?

aeneasr · 2017-04-27T09:08:11Z

oauth2/handler.go

@@ -40,6 +45,9 @@ type Handler struct {

 	AccessTokenLifespan time.Duration
 	CookieStore         sessions.Store
+
+	Issuer  string
+	BaseURL string


This is no longer needed right?

pbarker · 2017-04-27T14:32:06Z

CI is good now, not sure what that blip was, also removed the unnecessary parameter from the oAuth handler

aeneasr · 2017-04-27T15:30:36Z

Thanks! That's perfect. However, I have to be the mean maintainer here and insist on a test case for this :( It helps keep the code quality high and also test for potential errors not visible to code review.

Testing this should be easy. For the oauth2 package, set up a handler_test.go, bootstrap the handler itself (should be easy, just set the issuer - there is no other logic required) and do a http request against it, and test the output against what you expect.

Once that is done, I'll help you setting up the test for the JWK.

pbarker · 2017-04-27T15:31:52Z

ok no worries, I'll cook those up 👍

pbarker · 2017-05-01T22:03:09Z

Ok @arekkas, I've added tests for both routes. Some additional changes were necessary per #433, having key ids as "public" or "private" is not sufficient when they are returned in the well-known response. The key id must be unique, so I added set to the key generator here https://github.com/ory/hydra/pull/427/files#diff-98aed7eaac1441068d28c70047dd9fdbR21 . Let me know if you think there is a better way of handling that. Thanks!

aeneasr · 2017-05-02T07:34:59Z

It's weird that the tests are failing, I don't think it's your code's fault, I tracked it as #443

aeneasr

Awesome, just one more thing :)

aeneasr · 2017-05-02T07:37:07Z

cmd/server/helper_keys.go

@@ -18,7 +18,7 @@ func createRS256KeysIfNotExist(c *config.Config, set, kid, use string) {
 	if _, err := ctx.KeyManager.GetKey(set, kid); errors.Cause(err) == pkg.ErrNotFound {
 		logrus.Infof("Key pair for signing %s is missing. Creating new one.", set)

-		keys, err := generator.Generate("")
+		keys, err := generator.Generate(set)


I'm not entirely sure why I did generator.Generate("") here. Can you confirm that the docker-compose example works with your patch?

The docker-compose example works, I went through all of the flows in the tutorial. So this changes the generic key names to public:id-token-foo, which in turn changes a couple policies as documented here https://github.com/ory/hydra/pull/427/files#diff-08781ef00709d5059315ac5095fafabbR73 and https://github.com/ory/hydra/pull/427/files#diff-0704e4259cd334d9340c20bd02095628R8 . This has the downside of making the policy a bit more confusing. I suppose there aren't many policies that need to be written for the root keys, but let me know if you see a better way.

If we do want to go ahead with that, there are quite a few references to the kid being "public" or "private", the easiest way to get by that may be to add a getKeyFromType function that could be swapped out fairly easily.

So this changes the generic key names to public:id-token-foo, which in turn changes a couple policies as documented here

Yeah that's what I thought which is why I asked the question. I don't know how I feel about that. Does it change the URLs which are used to look up the keys as well?

The API contract would remain the same, but there may need to be some changes to /keys/set on the backend.

Google:
https://www.googleapis.com/oauth2/v3/certs
MS:
https://login.windows.net/common/discovery/keys

They are using just a uuid for the kid, so thats another option.

Ok, I see, I now also understand the change better. I think going with a timestamp, a uuid or some other unique identifier is the way to go here, the doubling of the name feels kinda weird and is also not unique, right?

However, this will impact the way we query for those keys within hydra, and maybe also in the SDK. So I think this is your suggestion for the key type?

I understand now the need for this, but it also complicates things a bit, so we need to document this, especially if we expose the KeyFromType functionality to the SDK/REST.

I think I wasn't able to get the point across yet :D

When booting Hydra on a new database, the signing keys for OIDC will be generated with:

{ set: "openid", keys: [{kid: "private"}, {kid: "public"}] }

In this case, the kids private and public are unique because the set does not have any other keys. Let's say the administrator of Hydra now adds a new keys to the set by accident:

{ set: "openid", keys: [{kid: "private"}, {kid: "public"}, {kid: "public:just-any-key-whatever"}] }

The private and public keys are still unique in this case - so no problem. Let's say the admin now wants to do some fancy key rotation. First, he renames the keys private and public:

{ set: "openid", keys: [{kid: "private:old"}, {kid: "public:old"}] }

next he adds the new keys:

{ set: "openid", keys: [ {kid: "private"}, {kid: "public"}, {kid: "private:old"}, {kid: "public:old"} ] }

Now, we have four keys. The old ones plus the fresh ones from key rotation. In a later stage I want to provide automatic key rotation, so this process would be automated by, for example, calling PUT /keys/rotate/openid with the result being something like this:

{ set: "openid", keys: [ {kid: "private"}, {kid: "public"}, {kid: "private:some-uuid-or-a-timestamp-or-something-else"}, {kid: "public:some-uuid-or-a-timestamp-or-something-else"} ] }

Does this make more sense now?

So the confusion here is stemming from which keys are returned in the well-known response. I am assuming that all of the public keys are returned, hence each set would have a kid of public and then you put all of those public keys in a single set and return them. Leading to 3 keys with the kid of public.

Whats interesting as I read through the OpenID spec http://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata again, they don't specify which key types should be returned. Looking at Okta http://developer.okta.com/docs/api/resources/oauth2.html#get-authorization-server-keys they are returning just one key type, but with an added status parameter.

I was seeing 3 keys from google and ms thinking it was consent, challenge, and idtoken, but it may be past, present, future of the same key type.

Oh wow, this took me some time to get it. I didn't get that well-known is aggregating all those keys and putting them in the same set. Now this all makes sense, sorry for being so slow on understanding this :D

Ok, so how about we simply prefix the kid with the set id in the well-known endpoint? Or is that a hack job?

Yeah so the more I look at this and think about it, I think you may have been right to assume that its just the idtoken key that needs returned. I can't find any reason to have the consent or challenge keys in that set. I'll update to reflect that, sorry about the run around.

On the upside we've had a chance to look at key rotation within the sets. I'll leave that for another PR though 😉

Yeah, only returning the keys for openid connect makes sense IMO :)

aeneasr · 2017-05-02T07:51:57Z

oauth2/handler.go

+func (h *Handler) WellKnownHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
+	known := WellKnown{
+		Issuer:        h.Issuer,
+		AuthURL:       h.Issuer + AuthPath,


I don't feel comfortable about this, what if the issuer is formated as http://foo.bar/foo/ and the AuthPath as /foo, then we get http://foo.bar/foo//foo as a result. For resolving this, see this SO question

I agree I added a trim suffix to root.go https://github.com/ory/hydra/pull/427/files#diff-ff7686b39bf90dc2520886fb874371a4R125 , do we want to do that for any of the other urls there?

Oh, I totally missed that! Then this is fine of course :)

aeneasr · 2017-05-05T07:56:35Z

Thank you @grillz for staying on this issue for so long, I really appreciate it!

Signed-off-by: pbarker <pbarker@datapipe.com>

aeneasr · 2017-05-08T09:52:05Z

@grillz to get this working I think you need to run glide update

Signed-off-by: pbarker <pbarker@datapipe.com>

pbarker · 2017-05-08T20:37:13Z

ok @arekkas I think we are close on this guy 😄, the one piece I couldn't figure out is how you are generating the swagger spec. I see https://github.com/ory/hydra/blob/master/gen-swagger.sh but thats generating a json file, are you just converting that to yaml?

Let me know if there is anything else you see, and I will squash before we merge.

aeneasr · 2017-05-08T21:55:57Z

Yeah I'm actually converting that manually via here (just paste the json and it will ask you if it's ok to convert to yaml). I'll take another 👀 tomorrow, 11.55pm over here :)

Signed-off-by: pbarker <pbarker@datapipe.com>

aeneasr

Yeah, you rock @grillz ! This looks awesome now! Don't be discouraged by the 23 comments, it's mostly nit picking and minor changes that should be resolvable in 5-10 minutes :)

Again, thank you a lot for contributing to Hydra and not running out of breath! Bigger changes require thorough code review and often a lot of time for both contributors as well as maintainers :) That way however I feel comfortable with releasing security-critical software that runs millions of requests :)

ps: you don't need to squash the changes (I can do that via the GH ui).

aeneasr · 2017-05-09T08:08:25Z

docs/api.swagger.yaml

@@ -169,7 +233,6 @@ paths:
          description: The id of the OAuth 2.0 Client.
          name: id
          in: path
-          required: true


This is actually required, was it removed by accident?

@arekkas I'm not sure what was going on with the swagger changes here, I just ran the gen-swagger.sh and ran it through the conversion. I was hoping you knew lol

aeneasr · 2017-05-09T08:08:34Z

docs/api.swagger.yaml

@@ -287,7 +350,6 @@ paths:
          description: The id of the OAuth 2.0 Client.
          name: id
          in: path
-          required: true


This is actually required, was it removed by accident?

aeneasr · 2017-05-09T08:08:59Z

docs/api.swagger.yaml

-        '302': {
-          description: "found"
-        }
+        '302': {}


Without description, this causes a schema error, could you revert this change?

aeneasr · 2017-05-09T08:09:03Z

docs/api.swagger.yaml

@@ -697,8 +757,7 @@ paths:
                type: string
                x-go-name: RefreshToken
      responses:
-        '200':
-          description: "ok"
+        '200': {}


Without description, this causes a schema error, could you revert this change?

aeneasr · 2017-05-09T08:09:14Z

docs/api.swagger.yaml

@@ -900,7 +958,6 @@ paths:
          description: The id of the policy.
          name: id
          in: path
-          required: true


This is actually required, was it removed by accident?

aeneasr · 2017-05-09T08:12:15Z

jwk/handler_test.go

+
+	JWKPath := "/.well-known/jwks.json"
+	res, err := http.Get(testServer.URL + JWKPath)
+	if err != nil {


This can be simplified with:

require.NoError(t, err, "problem in http request")

aeneasr · 2017-05-09T08:12:48Z

jwk/handler_test.go

+	}
+
+	resp := known.Key("public")
+	if resp == nil {


This can be simplified with:

require.NotNil(t, err, "Could not find key public")

aeneasr · 2017-05-09T08:13:38Z

oauth2/handler.go


 	"github.com/Sirupsen/logrus"
+	"strings"
+	"time"


I'm not super picky on this but since you probably will take another pass at this anyways, could you run goimports on this file? Please on this file only so we get a clean changeset!

aeneasr · 2017-05-09T08:14:54Z

oauth2/handler.go

+
+// swagger:model WellKnown
+type WellKnown struct {
+	Issuer        string   `json:"issuer"`


Nice! Thanks for adding the model here :) You could copy the descriptions from the JWK RFC here so it's clear what each of those values are.

aeneasr · 2017-05-09T08:17:20Z

oauth2/handler.go

+	r.GET(WellKnownPath, h.WellKnownHandler)
+}
+
+// swagger:route GET /.well-known/openid-configuration oauth2 WellKnownHandler


Could you add the tag openid-connect here? :)

Signed-off-by: pbarker <pbarker@datapipe.com>

pbarker · 2017-05-09T16:05:02Z

No worries @arekkas I'm happy to get it right 👍 , I updated all the changes you listed and ran goimports and gofmt on all the files. Turns out the odd swagger stuff was due to a slightly older version of go-swagger I was running. There were still a couple of pieces that didn't generate right that I manually fixed.

https://github.com/ory/hydra/blob/master/oauth2/handler.go#L207 is producing '302': {} where you said it needed a description to parse right. I manually added that, but we may want to look at having a custom response there. Also the scopes piece of the securityDefinitions I had to manually add to the swagger spec.

Let me know if you want to find a more automated solution to those in this PR. Thanks!

aeneasr · 2017-05-10T07:11:56Z

Awesome! Thanks!

aeneasr previously requested changes Apr 17, 2017

View reviewed changes

aeneasr mentioned this pull request Apr 17, 2017

oauth2: adds /.well-known/openid-configuration #406

Closed

pbarker commented Apr 17, 2017

View reviewed changes

pbarker force-pushed the wellknown branch from 8c0b49e to dc2055b Compare April 17, 2017 20:37

aeneasr reviewed Apr 27, 2017

View reviewed changes

aeneasr requested changes May 2, 2017

View reviewed changes

Aeneas Rekkas (arekkas) and others added 14 commits May 7, 2017 20:29

oauth2: adds /.well-known/openid-configuration - closes ory#379

1fe135e

oauth2: adds /.well-known/openid-configuration - closes ory#379

a2e1ab5

Finishing well known route

642f6dc

Signed-off-by: pbarker <pbarker@datapipe.com>

PR updates

98e298c

Signed-off-by: pbarker <pbarker@datapipe.com>

swagger

ddae503

Signed-off-by: pbarker <pbarker@datapipe.com>

swagger

01c60e5

Signed-off-by: pbarker <pbarker@datapipe.com>

remove extra

492644a

Signed-off-by: pbarker <pbarker@datapipe.com>

tests and key ids

7c255e7

Signed-off-by: pbarker <pbarker@datapipe.com>

ammend autoformat

0576425

Signed-off-by: pbarker <pbarker@datapipe.com>

cleanup

154b562

Signed-off-by: pbarker <pbarker@datapipe.com>

issuer suffix

25b597d

Signed-off-by: pbarker <pbarker@datapipe.com>

policy docs

ecdcf83

Signed-off-by: pbarker <pbarker@datapipe.com>

change to single key

ed173bd

Signed-off-by: pbarker <pbarker@datapipe.com>

cleanup

7913523

Signed-off-by: pbarker <pbarker@datapipe.com>

pbarker force-pushed the wellknown branch from 9967425 to 7913523 Compare May 8, 2017 02:49

pbarker added 2 commits May 7, 2017 20:58

cleanup

066eaba

Signed-off-by: pbarker <pbarker@datapipe.com>

cleanup

867c6a1

Signed-off-by: pbarker <pbarker@datapipe.com>

pbarker added 3 commits May 8, 2017 13:01

0.8 changes

5809642

Signed-off-by: pbarker <pbarker@datapipe.com>

cleanup

747a5d0

Signed-off-by: pbarker <pbarker@datapipe.com>

vendor and swagger

d6ed4a3

Signed-off-by: pbarker <pbarker@datapipe.com>

swagger

78eed5a

Signed-off-by: pbarker <pbarker@datapipe.com>

aeneasr reviewed May 9, 2017

View reviewed changes

pbarker added 2 commits May 9, 2017 09:33

PR updates

715c6c2

Signed-off-by: pbarker <pbarker@datapipe.com>

swagger updates

78346ca

Signed-off-by: pbarker <pbarker@datapipe.com>

aeneasr approved these changes May 10, 2017

View reviewed changes

aeneasr merged commit 3769676 into ory:master May 10, 2017

james-d-elliott mentioned this pull request Mar 22, 2021

RFC8615 Well Known URIs: jwks.json #2415

Closed

Wellknown #427

Wellknown #427

Conversation

pbarker commented Apr 17, 2017

aeneasr 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

pbarker commented Apr 26, 2017

aeneasr commented Apr 27, 2017

Choose a reason for hiding this comment

pbarker commented Apr 27, 2017

aeneasr commented Apr 27, 2017

pbarker commented Apr 27, 2017 • edited Loading

pbarker commented May 1, 2017

aeneasr commented May 2, 2017

aeneasr 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

aeneasr May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbarker May 5, 2017 • 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

Choose a reason for hiding this comment

aeneasr commented May 5, 2017

aeneasr commented May 8, 2017

pbarker commented May 8, 2017

aeneasr commented May 8, 2017

aeneasr 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

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

pbarker commented May 9, 2017

aeneasr commented May 10, 2017

pbarker commented Apr 27, 2017 •

edited

Loading

aeneasr May 3, 2017 •

edited

Loading

pbarker May 5, 2017 •

edited

Loading