-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add Solid-OIDC draft specification and primer to Solid Project #386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the submission! It is really great to see this getting in such a good shape, I have looked forward to understand what you've been up to! 👍
I have started my review by reading the primer, as it seems like a good idea to start there. I have not looked at the spec yet, but figured I might just share this as a start. Note that I'm coming to this from a pretty novice viewpoint, thus you might find some (or all :-) ) of my comments confused and detached from reality. That could have been a rather socratic thing, but the reality is that I just don't know better. My thinking is that it is better than I am confused and say so than some implementer who got this into their lap with little background is confused. ;-) I'm happy to take that confusion upon me. ;-)
I have added a bunch of detailed comments inline.
I have one big general one that I find rather confusing in the text, it seems authentication, authorization and also consent is used interchangeably. I suppose that might be due to upstream spec language use, but I think it is crucial for a primer, whose audience may be those less well versed with upstream specs, to clear up any tension that may arise from the use of different terms.
I think that in the wider Solid ecosystem, we tend use the term "authorization" for when we grant access to a resource, and so it seems that the way that "consent" was used, is when we really talk about that the user has authorized another user.
The term "authentication" seems be used in many places, and there's even a note in 4.2.14 that this primer only focuses on authn. Does that imply that this is really about authn and that "authentication" could be used throughout, and authz only mentioned when readers could be confused by various use in upstream specs?
We could well merge this without all of the comments addressed, as we will have some more process before we call this stable, but I wanted to submit comments so that you can look at it and think about what you think should be addressed before merging. I do think that some of it should be addressed though.
<p>This document outlines in details how Alice (end-user) asserts her identity | ||
(logs in) when using Decent Photos (relying party) to access data in hers and | ||
Bob’s Solid Storage (resource servers).</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're moving away from Alice and Bob, see #244 and check with the DEI Team.
<h2 class="no-num no-toc no-ref heading settled" id="abstract"><span class="content">Abstract</span></h2> | ||
<p>The Solid OpenID Connect (Solid OIDC) specification defines how resource servers | ||
|
||
verify the identity of relying parties and end users based on the authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"relaying parties" is only defined later in the document, so perhaps use a more layperson term in the abstract, alternatively link to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relying Party is a standard term from OAuth 2.0, but yes, clearer definitions would be helpful for a primer document
<p>Several actors are at play in our example Solid OIDC authentication flows:</p> | ||
<dl> | ||
<dt id="alice"><a class="self-link" href="#alice"></a>Alice | ||
<dd>Alice will be providing consent for Decent Photos to make resource requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of the term "consent" here is problematic, we haven't really introduced consent as a clearly defined concept yet, and I suppose this is more like what we elsewhere think of as "authorization", right? See the more general discussion of authz, authn and consent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"consent" should be replaced with "access"
<c- g>foaf</c->:<c- g>maker</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> <c- c1>;</c-> | ||
<c- g>foaf</c->:<c- g>primaryTopic</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<c- g>foaf</c->:<c- g>maker</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> <c- c1>;</c-> | |
<c- g>foaf</c->:<c- g>primaryTopic</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> . | |
<c- g>foaf</c->:<c- g>maker</c-> <c- nl>:me</c-> <c- c1>;</c-> | |
<c- g>foaf</c->:<c- g>primaryTopic</c-> <c- nl>:me</c-> . |
Not sure I got the syntax right here, but it shouldn't use localhost
and stuff, and just saying :me
should suffice.
new ID token when the current token expires.</p> | ||
</ul> | ||
<h3 class="heading settled" data-level="4.2" id="request-flow"><span class="secno">4.2. </span><span class="content">Request Flow</span><a class="self-link" href="#request-flow"></a></h3> | ||
<p><img src="oidc-primer-request.svg" width="980"></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just add comment to this file here. It is not clear to me which actor is involved here. For example, it seems to be Bob's AS, but it isn't clearly stated, and I feel it should.
</pre> | ||
<h4 class="no-num heading settled" id="request-flow-step-3"><span class="content">3. Creates a DPoP header token</span><a class="self-link" href="#request-flow-step-3"></a></h4> | ||
<p>Before we request access token, we need to generate a DPoP header token. A new DPoP token must be | ||
generated every time a request is made.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a stand-alone comment, that you need to generate a DPoP token for every request is confusing to me. When the request is made with an Access Token, that wouldn't trigger DPoP token generation, would it? As I said, I'm not terribly used to these protocols, but you are welcome to use me as a guinea pig for what others may find confusing. :-)
a 403 HTTP status. When using UMA 2.0, the AS will include <code>need_info</code> and <code>required_claims</code> values in the error response.</p> | ||
<h4 class="no-num heading settled" id="request-flow-step-6"><span class="content">6. Checks the DPoP token url and method</span><a class="self-link" href="#request-flow-step-6"></a></h4> | ||
<p>The AS checks the <code>htu</code> and <code>htm</code> claims of the DPoP token. If the <code>htu</code> does not match the | ||
protocol, origin and path of the request or the <code>htm</code> does not correspond the the http method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By origin, does it have anything to do with CORS and friends, or should it rather be authority?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be scheme
, authority
and path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
<c- g>foaf</c->:<c- g>maker</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> <c- c1>;</c-> | ||
<c- g>foaf</c->:<c- g>primaryTopic</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<c- g>foaf</c->:<c- g>maker</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> <c- c1>;</c-> | |
<c- g>foaf</c->:<c- g>primaryTopic</c-> <c- o><</c-><c- g>https</c->:<c- o>//</c-><c- g>localhost</c->:<c- mi>8443</c-><c- o>/</c-><c- g>profile</c-><c- o>/</c-><c- g>card</c->#<c- g>me</c-><c- o>></c-> . | |
<c- g>foaf</c->:<c- g>maker</c-> <c- nl>:me</c-> <c- c1>;</c-> | |
<c- g>foaf</c->:<c- g>primaryTopic</c-> <c- nl>:me</c-> . |
@kjetilk thank you for the initial review! Rather than accepting change suggestions directly in this document, I would rather make the changes in the source documents in https://github.com/solid/solid-oidc. In terms of process, I am flexible, but what I'd like to propose is this:
We have set up automation (we're still fine-tuning it) on the Solid-OIDC side so that it is easy for us to create new drafts to submit to the specification team. |
Yes, I understand that it is better to track changes at the ground truth document, and that we should await reviews of the others. I'll see if I can get to reviewing the spec itself, and post another comment about that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is still marked as a draft, the main protocol spec already references it, and we have multiple conformant client and server implementations, I think it's appropriate to merge as is, and open issues in the solid-oidc repository (per @acoburn's request). Generally I think the specification document is well-formed. My main area of feedback (which I'll raise in the solid-oidc repo) is around the introduction of UMA-based flows, which is great, but could benefit from more context (probably in the primer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for investing your time in contributing to the Solid project!
As part of this review, I've created 47 issues ( solid/solid-oidc#104 to solid/solid-oidc#151 ) that the contributors should consider following up. They are generally editorial recommendations that I think should be feasible to make it into the initial release of /TR/oidc and /TR/oidc-primer, however, I'd consider them as non-blocking and so can be in other releases of the spec, with the exception of a couple (hence marking this review with "request changes") - see issues on using MIT License, version information and links.
Can you clarify functional/feature differences between this version and the ED version that's originally referenced from the Solid Protocol? In point form as a comment in this PR will help as quick reference for future discussion. This is primarily because the Solid Protocol still references Solid OIDC Editor's Draft and a high-level understanding of the recent updates will be useful.
I'd like to also request that the feedback provided by the other reviewers that's unresolved or needs further thinking/discussion should have an issue in solid/solid-oidc before closing or merging this issue.
If you decide to follow up with a new PR, please link back to this PR in the initial comment.
The contributions herein should be accepted as is and fast tracked to a Platinum Recommendation due to the fact that it uses resource names such as file.svg
instead of file.mmd.svg
.
Note: Have created solid/solid-oidc#152 to capture my (non-blocking) review feedback on UMA. |
As the Solid-OIDC documents have been in editor's draft, there has been no clear versioning mechanism outside of the GitHub repository (hence the need to publish a ~FPWD). There are also several long open issues that were not in the Solid-OIDC Editor's draft text on 17 Dec 2021 (when the Solid Protocol was published) but were being actively discussed in the panel with clear intent to include those in the publication of the first draft of Solid-OIDC. There are several significant changes in the last ~6 months to highlight.
Taken together, these three changes share the fact that they all focus on aligning with existing patterns and conventions that this specification is building on. |
I have reviewed the normative text too, and I again choose to refrain from giving it a green or red light, I don't know whether my concerns are of such a nature that they are blocking, that's something we could discuss. Apart from the issues in my previous comment, my main concern are the rather lengthy protocol flows that seem to be required by the spec. Since it is only after the entire flow has been completed that the resource server will start to return the first byte of interest to the end user, and "time-to-first-byte" is a very important metric for user perception of server speed. Moreover, since requests go to many different servers, it cannot take advantage of HTTP/2's streams. I suppose use cases dictate that we need to live with these flows for now, but I believe that optimization is not premature, it seems very unlikely that we can live with such a flow executing for every request against an RS (there seems to be 10 requests-response pairs before first byte from the RS). For reference and nostalgia, we started out with the very simple FOAF+SSL protocol, and for a long time, authorization was something that could be done in a separate component, but not requiring other requests. I'm not asking for a return to this and I realize it is not possible, but I feel the need to examine under what conditions the present proposal could reduce to something resembling this simplicity. On the protocol level, I suppose there are three main avenues:
In the basic flow, there seems to be two parts that require a lot of back and forth, both initiated by the client, the first is the "start Authorization code grant" and the second to "request Access token". Is there possibly some way this can be reduced after the first request has been made? I don't know if you've had these debates, or if some of it might be covered by upstream standards, but I just feel the lack of mention concerning. |
Most of the requests are only made, first to obtain ID Token, which can be used with any number of Resource Servers. Later exchanging it for access tokens, one access token per RS. Once tokens are obtained, the client can make requests to each RS using the same access token, occasionally refreshing it with the corresponding AS. While there are extra requests for each RS to initially exchange ID Token for access token, and whenever access token for that RS expires refreshing it (a single request), there aren't additional request for every request against an RS. When it comes to the recommendation of caching of WebID Profile, ClientID Document, and OpenID Provider keys, we could discuss details in already existing issue solid/solid-oidc#24 |
OK, good! I do think that such things needs to be discussed in the primer so that the protocol is sufficiently conceptually self-contained, but that can certainly come later, not a blocker.
Uhm, yeah, I had forgotten I had actually opened that issue. |
I've applied the requested changes in #386 (review) and merged into the
Congratulations to everyone involved over the years for bringing this work to its current state and making an important contribution to the Solid project. Looking forward to future advancements. |
This represents the current state of Solid OIDC (draft 01) as a Community Group Draft.
Other than adjusting the URLs for "this version" and adding a reference to the live draft at https://solid.github.io/solid-oidc/, the pages here are exactly as seen at https://solid.github.io/solid-oidc/ and https://solid.github.io/solid-oidc/primer/