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

owin.RequestUser - a ClaimsPrincipal that identifies the user of a request #9

Closed
damianh opened this issue May 26, 2014 · 81 comments
Closed

Comments

@damianh
Copy link

damianh commented May 26, 2014

See owin/owin.dll#23 for original discussion. Changing the proposal from server.User (which is de-facto standard) to owin.RequestUser which I think is more appropriate.

I am proposing this for 1.1

Required Key Name Value Description
No owin.RequestUser An optional identity that represents the user associated with a request. The identity MUST be a ClaimsPrincipal. Middleware MAY specify this value. If it is not specified, middleware MAY set it. Once set, it MAY BE modified.

A possible concern is that ClaimsPrincipal is .NET 4.5 only ( the .NET 4.0 version is different assembly / namespace / structure)

Edit 2: Removed .NET 4 option meaning OWIN 1.1 will move to .NET 4.5
Edit 1: Added .net 4 / .net 4.5 type specification as per #9 (comment)

@damianh damianh changed the title server.User common key owin.RequestUser - a ClaimsPrincipal that identifies the user of a request May 26, 2014
@damianh
Copy link
Author

damianh commented May 26, 2014

A question is whether we should specify that Hosts MAY set this value? For windows auth in self host with HttpListener, it's the only supported way. Personally, I'm not sure..

@panesofglass panesofglass added this to the 1.1 Specify Middleware milestone May 27, 2014
@panesofglass
Copy link
Member

Should this be included in a general Security extension as proposed in #7?

@damianh
Copy link
Author

damianh commented May 28, 2014

I'm not sure, that security extension looks like it's a way to authenticate a user, not just what identifies them after whatever authentication has happened. I think these are separate concerns. Most non-security related middleware (take signalr as example) are just consumers of the identity and don't care for how the auth happened. (I'm not asserting that the security extension shouldn't happen anyway...)

@panesofglass
Copy link
Member

Good points. As to IPrincipal vs ClaimsPrincipal, I would love to go with ClaimsPrincipal, but my own libraries are currently still .NET 4.0, so I have a hard time wanting to force everyone up. That said, Katana is the primary platform people follow, which is .NET 4.5, and my libraries don't have a lot of traction yet, so now is a good time.

@damianh
Copy link
Author

damianh commented May 28, 2014

I'm for ClaimsPrincipal too because I can't envisage anyone using IPrincipal or anything else. The only other problem is mono support. While we have K, the System.Security.Claims nuget package and mono support there, will it be a problem for non-k mono users? If so, how big?

@skoon
Copy link
Contributor

skoon commented May 28, 2014

Can we spell out the pros/cons of going with ClaimsPrincipal over IPrincipal? Are the arguments beyond "concrete vs interface" and "new vs old" points?

@panesofglass
Copy link
Member

I'd completely forgotten about Microsoft.IdentityModel. That solves the problem for me.

As to pros/cons, I feel confident stating that now everything "in the box" is based on ClaimsPrincipal and claims, as a model, is far more flexible for modeling security attributes than, e.g. Roles, ClaimsPrincipal is a solid target. I can't think of a con except that the .NET 4.0 targets will have to rely on a NuGet package that may go the way of owin.dll, i.e. never get another update or patch.

@panesofglass
Copy link
Member

Do we want to submit this for a vote as-is or amend the proposal based on the above discussion?

@damianh
Copy link
Author

damianh commented Jun 9, 2014

'IPrincipal' is from .net 1.1 days and is (imho) really tied with windows concerns, whereas ClaimsPrincipal is designed for modern web security. @panesofglass Not sure if Microsoft.IdentityModel will work for you, it'll be a different type altogether. Should we just include a note regarding .net4.5-ness? Edit: What I mean is, if you mix .net 4 and .net 4.5 middleware together, one or other won't understand the type.

@damianh
Copy link
Author

damianh commented Jun 21, 2014

Votes as is?

@serialseb
Copy link
Contributor

So if I have a middleware built on 4.0 and I run on 4.5, is there a type redirect so it's transparent? Or do we expect middleware to have one binary for 4.0 and one for 4.5 and deal with per-framework dependency and hope all pacakges did the same?

@serialseb
Copy link
Contributor

Also, I take it hosts like Katana would have to wrap, say, Forms and Windows principals in a ClaimsPrincipal?

@damianh
Copy link
Author

damianh commented Jun 24, 2014

I can't envisage how a type redirect could work. Could you expand on that?

I think the latter is probably the way things will head - we're going to do
that for Nancy.

Is specifying the supported framework as part of a key definition viable?
Anyone using ClaimsPrinciple are definitely not going to be using .net
4.0 anyway.
On 24 Jun 2014 12:41, "Sebastien Lambla" notifications@github.com wrote:

So if I have a middleware built on 4.0 and I run on 4.5, is there a type
redirect so it's transparent? Or do we expect middleware to have one binary
for 4.0 and one for 4.5 and deal with per-framework dependency and hope all
pacakges did the same?


Reply to this email directly or view it on GitHub
#9 (comment).

@damianh
Copy link
Author

damianh commented Jun 24, 2014

They do for windows / ntlm auth in HttpListener. Horrible coupling to the
host though.
On 24 Jun 2014 12:43, "Sebastien Lambla" notifications@github.com wrote:

Also, I take it hosts like Katana would have to wrap, say, Forms and
Windows principals in a ClaimsPrincipal?


Reply to this email directly or view it on GitHub
#9 (comment).

@Tratcher
Copy link
Contributor

There is no compatible type redirect for ClaimsPrincipal on .NET4.0. There was an out-of-band package but it was not directly compatible with the 4.5 version.

Specifying a min framework version for a specific feature may be appropriate. OWIN already requires .NET 4.0 because of it's dependency on Tasks.

In this case however a compromise may be possible. ClaimsPrincipal implements IPrincipal, which is available on 4.0. This means 4.0 components will always be able to consume the key, even if they can't set it. As long as such limitations are clearly spelled out, I think that gives you the room to move forward with the more useful type.

The fully compatible alternative is to say that the value MUST be an IPrincipal and MAY be a ClaimsPrincipal, but that makes consumption more difficult.

@panesofglass
Copy link
Member

What about "MUST be an IPrincipal and SHOULD be a ClaimsPrincipal when using .NET 4.5 or greater."

@skoon
Copy link
Contributor

skoon commented Jun 24, 2014

What we really need is IClaimsPrincipal. Make it so @Tratcher

@Tratcher
Copy link
Contributor

@skoon already fixed on .NET 4.5. ClaimsPrincipal : IPrincipal and WindowsPrincipal : ClaimsPrincipal

@aliostad
Copy link

@panesofglass +1

@damianh
Copy link
Author

damianh commented Jun 24, 2014

@panesofglass +1 can work with that.

@markrendle
Copy link

Following up on discussion with Ryan & Scott on Skype:

I suggest that there are two separate keys, for the sake of discussion call them owin.ClaimsPrincipal and owin.Principal.

If you are writing your middleware for .NET 4.5, you MUST set owin.ClaimsPrincipal to an instance of System.Security.Claims.ClaimsPrincipal and you SHOULD set owin.Principal to the same object. A .NET 4.5 application using .NET 4.5 middleware MUST NOT have to env.TryGetValue("owin.ClaimsPrincipal"), it MUST be able to assume (ClaimsPrincipal)env["owin.ClaimsPrincipal"].

If you are writing your middleware for .NET 4.0, you MUST set owin.Principal to an instance of an object that correctly implements the IPrincipal interface.

The two things are sufficiently different that trying to write a single middleware that meets both needs is a recipe for #if NET45 cyclomatic disaster, a pit of failure into which people will unheedingly throw themselves. So let's actively strongly discourage if not actually prohibit it.

I'd rather see a fairly simple "IF/ELSE" in the spec than any of the abominations that would otherwise materialize in implementations.

@markrendle
Copy link

Scratch that, Mono is having ClaimsPrincipal very soon.

https://github.com/mono/mono/blob/master/mcs/class/corlib/System.Security.Claims/ClaimsPrincipal.cs

@damianh
Copy link
Author

damianh commented Jun 27, 2014

Finally!

@serialseb
Copy link
Contributor

May I just ask the stupid question. if it's IPrincipal and you pass it to the constructor in whatever ClaimsPrincipal you have, and it was a ClaimsPrincipal, it is the ClaimsPrincipal you provided, no? In which case any consumer that wants to use ClaimsPrincipal can just do new ClaimsPrincipal((IPrincipal)env["current.RequestUser"]) no?

Either the key is 4.5 only and it's ClaimsPrincipal, or it's 4.0+ and it's IPrincipal. Having both is rather confusing and tries to shoehorn too many scenarios in an awkward way IMO.

Lots of people have custom principle objects implementing only IPrincipal, not counting existing modules written for 4.0 running on 4.5, and that's causing me discomfort to ask everyone to just magically change their inheritance chain on components they don't own. And I would downvote such proposal for that reason, it would impede adoption in migration scenarios.

@serialseb
Copy link
Contributor

to be clear, the scenario I'm trying to avoid is where code cannot use the custom IPrincipal implementation that the company relies on, due to it being wrapped in a ClaimsPrincipal.

Alternative as I understand it would be to ask people to change the inheritance chain in their code, which they may not control, or to write a middleware that reads from HttpContext.User and writes in a custom key (meh) or to write a middleware to custom-wrap the IPrincipal in a ClaimsPrincipal (lots of work for most).

The more I think about it, the more exposing ClaimsPrincipal sounds like a hard sell, compared to IPrincipal.

@panesofglass
Copy link
Member

I can get behind what you are saying @serialseb, but I wonder how likely it is that someone will run into this issue? Do we have specific examples? Consider who of those with a custom IPrincipal will actually use OWIN.

@augustoproiete
Copy link

+1

@panesofglass
Copy link
Member

@panesofglass
Copy link
Member

@EddieGarmon, I appreciate your concerns. Unfortunately, that ship has sailed. Changing it now would imply breaking changes. Also, this spec is intended to be prescriptive. We originally wanted to make this more open but wound up with a very gnarly delegate signature. At some point, you have to specify some types; otherwise, there's no chance at interop.

Perhaps your issue is that by specifying types, this is not, in fact, a spec?

@panesofglass
Copy link
Member

@EddieGarmon, also, please feel free to propose an alternative before voting closes. I understand you don't like the proposed spec language, but I don't know what you would prefer instead.

@horsdal
Copy link

horsdal commented Oct 16, 2014

+1

@damianh
Copy link
Author

damianh commented Oct 17, 2014

Ah right :)
On 16 Oct 2014 17:56, "Ryan Riley" notifications@github.com wrote:

@damianh https://github.com/damianh, @serialseb
https://github.com/serialseb is correct. See
http://owin.org/html/governance/decisionprocess.html.


Reply to this email directly or view it on GitHub
#9 (comment).

@panesofglass
Copy link
Member

Voting will soon close. Please submit remaining votes.

@EddieGarmon
Copy link

-0

One thing to consider with requiring .NET 4.5 is deployment. Most enterprise IT that I know are still on Win Server 2008 or earlier (I have no idea how virtual deployments in azure, et.al are), and most enterprise IT do not auto update frameworks on their servers. How much of our target audience are we abandoning by forcing a 4.5 dependency?

I like owin.RequestUser as an IPrincipal that can be upcast where appropriate.

@EddieGarmon
Copy link

Also, shouldn't we have a written spec that we are voting on? To me a +1 says I will help implement the spec, not draft it, via our current decision process.

@damianh
Copy link
Author

damianh commented Oct 20, 2014

A) They can use owin 1.0 MW.
B) They can manually install .net 4.5.x
C) There is no security concept at all in owin 1.0. If they never had it,
are they really 'abandoned'
D) Would like a quantitve answer to how many windows 2008/.net 4.0 shops
are using owin.
E) MS's security owin MW (katana) are the only ones that matter to
enterprise dev and they have already abandoned this category of customer -
this is all moot anyway.
On 20 Oct 2014 18:03, "Eddie Garmon" notifications@github.com wrote:

-0

One thing to consider with requiring .NET 4.5 is deployment. Most
enterprise IT that I know are still on Win Server 2008 or earlier (I have
no idea how virtual deployments in azure, et.al are), and most enterprise
IT do not auto update frameworks on their servers. How much of our
target audience are we abandoning by forcing a 4.5 dependency?

I like owin.RequestUser as an IPrincipal that can be upcast where
appropriate.


Reply to this email directly or view it on GitHub
#9 (comment).

@panesofglass
Copy link
Member

@EddieGarmon, language in the current process is based on OSS projects, as @serialseb has pointed out, and needs to be revised at some point. Consider "implement" == "draft".

In the present case, the draft is in the description of the issue. If accepted, we will literally copy the content above and move it to the OWIN spec verbatim. In other words, +1 and +0 are equivalent for this vote.

@panesofglass
Copy link
Member

@EddieGarmon, the key requires .NET 4.5, but it is optional, and existing solutions have their own keys, which is also a sufficient mechanism for providing data not otherwise specified.

@EddieGarmon
Copy link

oops, keys before brain on that thought...

@serialseb
Copy link
Contributor

we're moving to server, not host :-P Plus we can't make anything in a spec required without breaking all hosts, including those deployed, so that's a no go. There's no reason to not leave it optional as far as I can see.

If there is a need for a "legacy" iprincipal, i would suggest we create another optional spec for that key, independently of claimsprincipal, and with full compat with .net 4.0. Then any middleware can do feature detection very easily.

What we don't talk about are impersonation rules, which owin currently ignores completely. I would assume it would be the role of a middleware to detect either or of those keys and impersonate the thread correctly, provided the server itself didn't do it.

Due to the conversation up this chain, .net as a whole is legacying IPrincipal. I can relate to the feeling but I withdrew any objection, the compromise being acceptable due to the whole platform shifting. Nothing prevents another proposal for an owin.LegacyRequestUser to be created. Again, writing an adapting middleware for either / or scenarios would be trivial.

@EddieGarmon
Copy link

I have no problem with the shift in direction, and I do believe a claims based principal is the way to go.

@skoon
Copy link
Contributor

skoon commented Oct 20, 2014

+0, I agree but I want people who write gooder than me to write the spec.

@panesofglass
Copy link
Member

Time to close voting?

@panesofglass
Copy link
Member

Tally:

  • 4 +1
  • 3 +0
  • 1 -0

The proposal is accepted.

@panesofglass
Copy link
Member

  • Need to add this key to the spec.

@serialseb, should we include this in your draft or go ahead and move this into the current spec draft?

@panesofglass
Copy link
Member

I added the key to the OWIN spec.

@serialseb
Copy link
Contributor

It should go in additional headers separately from the spec if you ask me. I’d create a new github page for it?

@panesofglass
Copy link
Member

@serialseb, would you mind submitting a proposal to move specific headers to another spec? I think it would be good to hash it out first, and I know you have some very specific thoughts on this topic.

@panesofglass
Copy link
Member

@serialseb, would this fall in with #26? I think it would be great to see a proposal for the exact changes. I'm not entirely clear what should move and stay.

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

No branches or pull requests