Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group interaction mode flags #122

Closed
aaronpk opened this issue Nov 17, 2020 · 30 comments
Closed

Group interaction mode flags #122

aaronpk opened this issue Nov 17, 2020 · 30 comments
Assignees

Comments

@aaronpk
Copy link
Collaborator

aaronpk commented Nov 17, 2020

The current interaction modes listed are: redirect, app, callback, user_code, and ui_locales. There are three different types of flags here:

  • how the client can tell the user how to interact with the AS (redirect, app, user_code)
  • how the client can handle a response from the AS (callback)
  • hints to the AS (ui_locales)

We should consider whether it makes sense to group these so that future extensions don’t run the risk of conflicts and so that the purpose of each is clearer to developers. For example (names TBD):

{
  "interact": {
    "start": ["redirect", "app", "user_code"],
    "finish": {
      "callback":{"method": "redirect", "uri": "XXX", "nonce": "XXX"}
    },
    "hints": {
      "ui_locales": ["en-US"]
    }
  }
}

(See PR #164)

@fimbault
Copy link
Collaborator

fimbault commented Nov 17, 2020

+1 This brings clarity.

@jricher
Copy link
Collaborator

jricher commented Nov 17, 2020

I like this approach, and it's largely equivalent to what we have right now (but with more clarity). I think it could even be combined with the bundling concept in #59 if we wanted to.

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 27, 2021

Polymorphism question... are we going for the style where if an array has only one value it's also okay to send that one value itself?

e.g.

{
  "foo": ["bar"]
}

can also be represented by

{
  "foo": "bar"
}

It's less important for string values, but forcing everything into an array starts to look awkward when values are objects:

{
  "foo": {
    "color": "red"
  }
}

vs

{
  "foo": [
    {"color": "red"}
  ]
}

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 27, 2021

I made an initial attempt at this in PR #164, however depending on the polymorphism question above I'll need to clean it up still.

@jricher
Copy link
Collaborator

jricher commented Jan 27, 2021

My personal view is that the polymorphism should always be clearly unambiguous for every field. In other words, if it's an object it always means the same thing, an array always means the same thing, etc. If this means that a field is either an object or an array of those objects, then we could support that.

However, I'd argue that we shouldn't have this kind of optimization when there's no semantic difference. That is to say, when there's an array of one object it should still always been an array unless we have a compelling reason to define both formats. For access tokens, it's semantically different to ask for a single vs. multiple access tokens, so they use this object vs. array-of-object structure mentioned here. But what's proposed here looks like a simple optimization. While I personally think that's sensible to do, I think we can and should address that optimization separately. This is the approach I took with #162.

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 27, 2021

I guess this question is actually slightly more complex than whether this is a simple optimization of a single-value array. Where it gets tricky is when we want to use multiple of these modes combined.

Combining string values is easy:

{"start": ["redirect","app","user_code"]}

Combining objects can be done in two ways:

{ 
  "finish": {
    "callback": {"uri":"..."},
    "push": {"uri":"..."}
  }
}
{"finish": [
  {"callback":{"uri":"..."}},
  {"push":{"uri":"..."}},
]}

It's a difference of "object with keys" vs "array of objects" I guess. The downside of the "object with keys" approach is then simple string values aren't really possible without doing the awkward boolean true thing, whereas an "array of objects" could also just as easily be an "array of objects and strings".

This is mostly a stylistic question at this point, but I do think we should write up some guidance around this so that we have something to reference when we have these questions in the future.

@fimbault
Copy link
Collaborator

Good questions on polymorphism.

That's pretty similar to what Justin suggested, I would start with the syntax that we think is most consistent and then see if compact alternatives make sense.

But I liked lisp. So I may not be acting normally :-) Joke aside I think it's hard to tell for sure right now.

@jricher
Copy link
Collaborator

jricher commented Jan 28, 2021

I don't think it makes sense to have multiple modes for "finish". How would the AS know which one(s) to fire?

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 28, 2021

That's an interesting point that I hadn't seen mentioned before. I was thinking of the exchange as the client saying "here are all the things I can do" and the AS picking the ones of the list that it supports. So in some cases one AS may not support some completion modes that another AS does, where a client can support all.

@jricher
Copy link
Collaborator

jricher commented Jan 28, 2021

The model I'm in favor of is the client instance providing multiple ways to get started and one way to get back. Letting the AS pick the return leaves the client in charge of listening to different callbacks, which are going to have different security properties. This seems like it's going to open the door to new and fun attack vectors -- like convincing a client to go poll by posting to a static URL when it should be getting a bound callback parameter on a dynamic URL.

This is a very different situation than the AS listening for multiple interaction points, though -- the AS is in more control and the security stakes are known to be high, so it should be more aware of the process.

If the client has multiple ways to get back, the AS would likely want to offer different experiences for each, based on those expectations. This is where the multiple-mode bundling idea of #59 comes into play, in which case we'd look at having the upper-level "interact" structure possibly be multiple, in that case.

@fimbault
Copy link
Collaborator

fimbault commented Jan 28, 2021

I also think a single finish is best. The only case I'd imagine several possibilities is if the client can specify the rules to help decide, but it's probably over complex (I don't see a practical example of where that would be useful).
So aligned with previous comment.

One additional (maybe unrelated) question is whether the continuation could allow to chain successive interactions or whether everything is supposed to be handled in one go. Successive interaction could be useful if you'd want to implement a process with several successive URIs. Maybe that's dumb but I had the demand from a customer just yesterday (didn't think much about it yet).

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 28, 2021

Okay, if a single finish is the plan we should make sure these motivations are clarified in the spec too! With the current syntax (before this PR) it's possible for the syntax to allow multiple finish options. With the new syntax we can at least enforce a single finish option.

@agropper
Copy link

Apologies for any newbie mistakes, but this makes me wonder how we handle the response to an authorization request. In many real-world cases, especially healthcare, the authorization is requested by a client as user agent of the RQ but the response is presented to the RS by another client such as the institutional system of the RQ's employer that may be under the control of another RQ or an institution.

This delegation of the authorization from one client to another may be out of scope for GNAP but I am worried about how the RS will handle this distinction in cases where they want to minimize their risk.

@jricher
Copy link
Collaborator

jricher commented Jan 29, 2021

So I'm wondering if we should make "finish" just be the internal structure of what's in "callback" today:

{
  "interact": {
    "start": ["redirect", "app", "user_code"],
    "finish": {
      "method": "redirect", 
      "uri": "XXX", 
      "nonce": "XXX"
    },
    "hints": {
      "ui_locales": ["en-US"]
    }
  }
}

This would keep the "one way to finish" aspect, but wouldn't rule out someone defining some sort of hybrid finish if something like that would exist.

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 29, 2021

Actually that solves #163 as well by dropping the name "callback" entirely. I think this is a good plan. I'll update the PR.

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 29, 2021

Actually this has some pretty significant implications elsewhere in the spec. Right now there's a lot of references to the "callback" mode, e.g.

if a "callback" (Section 3.3.3) mode is available with the current request, the AS MUST follow the appropriate method...

Dropping that from the finish object means we don't really have a way to refer to the "callback" mode anymore. I think I'm okay with this, since we can still refer to the "redirect" or "push" modes defined in the spec. e.g:

When using the "callback" interaction mode (Section 3.3.3) with the redirect method, the AS signals...
When using the "callback" interaction mode (Section 3.3.3) with the push method, the AS signals...

changes to

When using the "redirect" interaction mode (Section 3.3.3), the AS signals...
When using the "push" interaction mode (Section 3.3.3), the AS signals...

If we agree I'll keep going with the changes here.

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 29, 2021

A more significant change is the response from the AS, in Section 3.3.3

    "interact": {
        "callback": "MBDOFXG4Y5CVJCX821LH"
    }

doesn't make sense anymore, so probably that would have to change to:

    "interact": {
        "redirect": "MBDOFXG4Y5CVJCX821LH"
    }

and

    "interact": {
        "push": "MBDOFXG4Y5CVJCX821LH"
    }

I do like that it's removing one layer of abstraction though and being more direct.

@jricher
Copy link
Collaborator

jricher commented Jan 29, 2021

Those are still separate "interaction finish methods" that would need to be defined separately, and so I think it should be fine to swap the references in-place. I think the return for this could now actually be something like:

interact: {
    finish: {
        nonce: "MBDOFXG4Y5CVJCX821LH",
        method: "redirect"
    }
}

Or we could keep it collapsed, swapping callback for finish like with the request:

interact: {
    finish: "MBDOFXG4Y5CVJCX821LH"
}

I'm not convinced having different member names for different finish methods at the top level of the object is a good idea, from a namespace cleanliness perspective -- extensions are going to be able to define both start and finish methods, and they'll need to somehow live in the response together. Maybe like with the discussion above we just keep the "verbose" form and figure out if an optimization makes sense later? I could go either way, personally.

@jricher
Copy link
Collaborator

jricher commented Jan 29, 2021

@agropper in your use case, the end-user and resource owner are separate, so the client isn't handling the "start" of the interaction process. The client can still be notified of when it's finished in a secure fashion, which is what the "pushback" method is good for. Or the client can just poll, waiting for it to be approved by someone, so it wouldn't need to know externally when the authorization is completed.

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 29, 2021

Okay that makes sense. I don't think the client benefits from having the method confirmed in the response so I'll start with the simpler version that's more inline with the current one anyway:

interact: {
    finish: "MBDOFXG4Y5CVJCX821LH"
}

@aaronpk
Copy link
Collaborator Author

aaronpk commented Jan 29, 2021

Alright, that was quite a bit of surgery but I do think this cleans things up nicely now! Current draft can be viewed here:

https://deploy-preview-164--gnap-core-protocol-editors-draft.netlify.app/draft-ietf-gnap-core-protocol.html

@agropper
Copy link

@jricher I'm playing catch-up here so please bear with me. Yes, the end-user and resource owner are separate. In this use-case the resource owner's client is irrelevant because they have delegated authority to respond to requests to an authorization server.

In this use-case the end user is using two different clients as part of the interaction process. One client is a true user agent, the other client is an agent of some other entity. The client that interacts with the authorization server is logically and maybe legally separate from the client that interacts with the resource server. Optionally, the end-user that began the interaction with the authorization server may delegate their authorization to another end-user that will actually access the resource with a different client.

Finally, in this use-case, the resource server may seek signed software statements from the client presenting the authorization. Those software statements may or may not also be shared with the authorization server as part of the interaction.

I don't understand your response about client polling because I am not concerned about that. If this use-case is already handled in the spec, please point me to the relevant sections.

@fimbault
Copy link
Collaborator

fimbault commented Jan 30, 2021

@agropper Justin might correct if I'm wrong, but seems to me that this use case is currently not covered. As far as I understand it however, it would be possible I think through biscuit tokens https://www.biscuitsec.org (+ https://github.com/CleverCloud/biscuit), but the delegation between clients is not covered through the protocol.

@agropper
Copy link

I don't expect the delegation between clients to be covered through protocol, just possible.

The issue of delegation among end-users has been a hot topic in credential and storage W3C workgroups. Here, for example is one thread with over 100 messages. There are many others and the issue is far from resolved.

My goal is harmony between W3C data models and GNAP. I've created a very concise model for six use-cases that may be helpful here as well. The use-case we're discussing here is on Slide 10. In due course, we may choose to consider harmony on the other use cases as well, such as SIOP authentication.

I know precious little cryptography. If we used biscuits, would GNAP adopt them for everything or would the option be a request on the part of the requesting party?

@fimbault
Copy link
Collaborator

fimbault commented Jan 30, 2021

@agropper will look into your docs. GNAP is agnostic on the token format, of course the default being jwt. Biscuits are indeed based on fancy crypto

@fimbault
Copy link
Collaborator

fimbault commented Feb 4, 2021

Is there a chance that the interaction could panic / fail with an unrecoverable error that requires to stop the flow?

For instance :

  • reach a timeout (the RO just went for a walk)
  • the client developer made a mistake in the start section, his device is unable to redirect to a proper display

@fimbault
Copy link
Collaborator

fimbault commented Feb 4, 2021

Regarding the wording : we currently use "completed interaction" in the text. Maybe people could wonder if that's the same as "finish"?

@fimbault
Copy link
Collaborator

fimbault commented Feb 4, 2021

@agropper I believe your concerns would be better managed in dedicated issues.

I've looked at your use case (thanks for the slides, that's clear). In my view, the delegation that you're describing with different clients should be managed through tokens that are able to manage that complexity, such as biscuits. See #169

I've also created a new issue for the distinct case when end-user != RO, see #168

Please feel free to comment on those issues directly.

@jricher
Copy link
Collaborator

jricher commented Feb 10, 2021

See PR #164

@aaronpk
Copy link
Collaborator Author

aaronpk commented Feb 22, 2021

PR #164 has been merged

@aaronpk aaronpk closed this as completed Feb 22, 2021
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

No branches or pull requests

4 participants