Add Authentication Metadata Extension #275

rwinch · 2019-09-23T21:00:37Z

spencergibb · 2019-09-23T21:49:11Z

Extensions/Security/Basic.md

+
+* **Username Length**: Username Length in bytes.
+* **Username**:  The UTF-8 encoded username.  The string MUST NOT be null terminated.
+* **Password**:  The UTF-8 encoded password.  The string MUST NOT be null terminated.


Password Length?

How about tagging for username and password? https://github.com/rsocket/rsocket/blob/master/Extensions/Routing.md . The first tag is user name, and the second is password?

Password Length?

Isn't that just the remaining bytes?

How about tagging for username and password? /Extensions/Routing.md@master . The first tag is user name, and the second is password?

I don't know that tags really make sense here.

First, the description you linked to seems like the tags are used for routing. Credentials (at least primary use) is not for routing. Perhaps routing is just one use of tags? If so, can you point me to where I can learn about general tags vs just in the context of routing?

Second, I don't know that tags really make sense for credentials. The username and password belong together and don't really make sense as separate entities.

Finally, I believe that something as widely used as credentials should likely have first class support.

OlegDokuka · 2019-11-19T13:28:34Z

@rwinch, I'm thinking will not be it simpler to having something similar to what there is in HTTP, e.g having a type of auth as a part of metadata, which limits the number of possible metadata types to a minimum and does not require further extending to the extension.

for example:

     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |Auth ID/Length |         Auth Type                            ...
    +---------------+-----------------------------------------------+
	|   					Encoded Credentials					   ...
    +---------------------------------------------------------------+

where Auth ID could be additionally listed (as for WellKnown MimeTypes) or it could be any Auth Type encoded as a string.

Then, it leads to a single additional MimeType for Auth, like message/x.rsocket.authentication.v0 which simplify the further extension of the spect

Then, for Basic, it could be Base64 encoded colons separated content/string where for Bearer it would be just a token.

Just my thoughts on the topic. Feel free to ignore it :D

OlegDokuka · 2019-12-02T15:07:19Z

Hey, @rwinch @spencergibb

Do you have any updates?

Regards,
Oleh

This changes adds an extension specification for including credentials for authentication

This changes adds an authentication type for including a username and password.

This change adds an authentication type for including a bearer token.

This change adds well known authentication types.

rwinch · 2019-12-03T19:16:58Z

@OlegDokuka and I spoke about the changes offline. The goal is to avoid having a metadata extension for each type of authentication. However, I believe we should avoid using constructs that work around limitations in HTTP (i.e. base64(username:password) in RSocket.

We have defined something similar to Composite Metadata Extension. However, it is specific to Authentication. This allows for custom encoding of the credentials without the need for a custom mime type.

I have pushed the changes for review.

OlegDokuka

Yeah! I like it!

OlegDokuka · 2019-12-03T19:45:17Z

@linux-china @spencergibb @rdegnan @robertroeser @nebhale @stevegury, @yschimke folks, do you have any comments/objections?

yschimke · 2019-12-03T20:25:12Z

No strong objections. Happy for those using it to prove it in v0 and burn a version if there are mistakes.

General concerns: doesn’t cover challenges from HTTP and not sure the optimisation is worth having to invent our own non standard encodings.

stevegury

Ditto, no strong objections.

stevegury · 2019-12-04T00:53:30Z

Extensions/Security/Authentication.md

+    +---------------+-----------------------------------------------+
+```
+
+* (**A**)uthentication Type: Authentication type is a well known value represented by a unique integer.


It's not clear which bit value does what.
Maybe we should be more explicit, i.e. 0 means Auth Length, 1 means Auth Id.

@stevegury Thanks for the feedback. I tried to follow the conventions I found in the existing specs. The clarification is on the next line just as it is done in Composite Metadata:

If A flag is set, indicates a [Well-known Auth Type ID][wk]. If A flag is not set, indicates the Authentication Type Length in bytes.

I pushed a fix in de71eaa I kept the commit separate, so it was easier to see the changes. I can clean up the commits later if desired.

If we decide to stick with these changes, I'd suggest we also update Composite Metadata.

rwinch · 2019-12-04T22:58:23Z

@yschimke

doesn’t cover challenges from HTTP

I'm sorry. I don't understand. Can you please expand on what you mean by this?

not sure the optimisation is worth having to invent our own non standard encodings.

How would you encode it then?

The way a username/password is encoded when using HTTP are based on context. HTTP Basic is base64 encoded because it makes sense for HTTP headers. When a username/password is passed in the body of the request as form parameters (I'd argue this is the most common way username/password is submitted with HTTP), they are URL encoded because that is what makes sense for a form submission. If credentials are submitted as JSON, then they are encoded yet another way.

In short, the way a username/password is encoded when using HTTP is based upon the context. In the same manner, when using RSocket a username/password should be encoded in a way that makes sense for RSocket.

linux-china

Agree. Now we use custom metadata type for JWT, and it should be a standard for authentication metadata.

yschimke · 2019-12-05T06:58:02Z

I'm sorry. I don't understand. Can you please expand on what you mean by this?

This spec is obviously inspired by existing HTTP standards but doesn't consider the full flow of challenge response.

Instead its focus on prior knowledge usage.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication

yschimke · 2019-12-05T07:00:56Z

In short, the way a username/password is encoded when using HTTP is based upon the context.

That's not true for basic auth. There is a spec for basic auth including an encoding.

https://en.m.wikipedia.org/wiki/Basic_access_authentication

Maybe just rename it to username and password instead of basic.

yschimke · 2019-12-05T06:48:36Z

Extensions/Security/Authentication.md

+Authentication is a necessary component to any real world application. This extension specification provides a standardized mechanism for including both the type of credentials and the credentials in metadata payloads.
+
+## Metadata Payload
+This metadata type can be used in a per connection or per stream, and not individual payloads and as such it **MUST** only be used in frame types used to initiate interactions and payloads.  This includes [`SETUP`][s], [`REQUEST_FNF`][rf], [`REQUEST_RESPONSE`][rr], [`REQUEST_STREAM`][rs], [`REQUEST_CHANNEL`][rc], and [`PAYLOAD`][p].  The Metadata MIME Type is `message/x.rsocket.authentication.v0`.


Why is payload listed here? Payload doesn't start a stream (interaction?) or connection.

Do we talk about interactions elsewhere?

Looks like it is a common part of every extension definition -> for instance https://github.com/rsocket/rsocket/blob/master/Extensions/Routing.md#metadata-payload

@yschimke You are right that we should not list PAYLOAD here because it is not a start of a stream or connection. I pushed fa1a80b to address this.

I kept the commit separate, so it was easier to see the changes. I can clean up the commits later if desired.

rwinch · 2019-12-05T14:26:21Z

This spec is obviously inspired by existing HTTP standards but doesn't consider the full flow of challenge response.

Are you pointing out that RSocket does not define how to request a user provide credentials? If so, I agree.

@OlegDokuka and I discussed this offline and I believe he was planning on adding a separate ticket to discuss how to handle errors for authentication required, invalid authentication, and not authorized.

I personally felt that including credentials and credential error handling would be separate specifications that eventually link to one another. This would allow for reusing the error handling when including the credentials in a different way. If we want to combine this into a single specification or flushing that out before we proceed here, I'd be open to discussing that. Just let me know.

That's not true at all for basic auth. There is a spec for basic auth including an encoding.

HTTP Basic Auth defines an encoding, but it also defines the context - an HTTP header field. Username/password included in a form post, JSON, etc do not use the same encoding as HTTP Basic Auth because they are not included in the header. Instead, they use the appropriate encoding for their contexts. Just as a form submission does not use HTTP basic encoding, RSocket should use an encoding that makes sense for the context of metadata.

Maybe just rename it to username and password instead of basic.

I agree the name basic is not a good choice due to the fact that many will confuse it with HTTP Basic. My hope is that this is differentiated by HTTP Basic Auth vs RSocket basic, but this is still not as clear as I'd prefer.

I'm personally not a fan of username and password as it seems pretty verbose. Perhaps I am alone with this though. Perhaps we could use simple? If anyone has thoughts on naming, I'd like to hear it.

OlegDokuka · 2019-12-05T14:46:13Z

@OlegDokuka and I discussed this offline and I believe he was planning on adding a separate ticket to discuss how to handle errors for authentication required, invalid authentication, and not authorized.

yes, I’m planning to rise another ticket for that

I'm personally not a fan of username and password as it seems pretty verbose. Perhaps I am alone with this though. Perhaps we could use simple? If anyone has thoughts on naming, I'd like to hear it.

Yeah, I like simple for that purpose

PAYLOAD is not a start of a stream or a connection and should not be listed.

This ensures that users do not confuse the specification with HTTP Basic Auth.

rwinch · 2019-12-05T15:56:34Z

Yeah, I like simple for that purpose

I pushed a change for this 732ad71 I kept the commit separate, so it was easier to see the changes. I can clean up the commits later if desired.

yschimke · 2019-12-05T16:06:44Z

To be clear. No intention from me to block forward progress. Just feedback to consider. Happy to ship this. Usage trumps drive by opinions :)

Extensions/Security/Authentication.md

spencergibb reviewed Sep 23, 2019

View reviewed changes

OlegDokuka added the enhancement label Nov 23, 2019

spencergibb approved these changes Dec 2, 2019

View reviewed changes

rwinch added 4 commits December 3, 2019 13:03

Authentication Extension

abcc36e

This changes adds an extension specification for including credentials for authentication

Basic Authentication Type

66e58aa

This changes adds an authentication type for including a username and password.

Bearer Authentication Type

e62743c

This change adds an authentication type for including a bearer token.

Well Known Authentication Types

19abd5b

This change adds well known authentication types.

rwinch force-pushed the gh-272 branch from 3553867 to 19abd5b Compare December 3, 2019 19:13

rwinch changed the title ~~Add Security Metadata Extensions~~ Add Authentication Metadata Extension Dec 3, 2019

OlegDokuka approved these changes Dec 3, 2019

View reviewed changes

OlegDokuka requested a review from linux-china December 3, 2019 19:54

stevegury approved these changes Dec 4, 2019

View reviewed changes

linux-china approved these changes Dec 5, 2019

View reviewed changes

yschimke reviewed Dec 5, 2019

View reviewed changes

rwinch added 3 commits December 5, 2019 09:32

Remove PAYLOAD frame type

fa1a80b

PAYLOAD is not a start of a stream or a connection and should not be listed.

Clarify 0 and 1 for Auth Length and Auth Id

de71eaa

Switch Authentication basic to simple

732ad71

This ensures that users do not confuse the specification with HTTP Basic Auth.

OlegDokuka reviewed Dec 5, 2019

View reviewed changes

Extensions/Security/Authentication.md Show resolved Hide resolved

OlegDokuka merged commit 7dfc9f4 into rsocket:master Dec 5, 2019

Add Authentication Metadata Extension #275

Add Authentication Metadata Extension #275

Uh oh!

Conversation

rwinch commented Sep 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegDokuka commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlegDokuka commented Dec 2, 2019

Uh oh!

rwinch commented Dec 3, 2019

Uh oh!

OlegDokuka left a comment

Choose a reason for hiding this comment

Uh oh!

OlegDokuka commented Dec 3, 2019

Uh oh!

yschimke commented Dec 3, 2019

Uh oh!

stevegury left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch commented Dec 4, 2019

Uh oh!

linux-china left a comment

Choose a reason for hiding this comment

Uh oh!

yschimke commented Dec 5, 2019

Uh oh!

yschimke commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlegDokuka commented Dec 5, 2019

Uh oh!

rwinch commented Dec 5, 2019

Uh oh!

yschimke commented Dec 5, 2019

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

OlegDokuka commented Nov 19, 2019 •

edited

Loading

yschimke commented Dec 5, 2019 •

edited

Loading

rwinch commented Dec 5, 2019 •

edited

Loading