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

document CAS login and m.login.token login #367

Merged
merged 6 commits into from Aug 9, 2016

Conversation

Projects
None yet
2 participants
@richvdh
Member

richvdh commented Aug 8, 2016

Following the spirit of "document how it is, not how we wish it was", document the CAS login bits.

This supercedes #171. CAS login doesn't actually work with the user-interactive API right now, so I've moved it out of there. Indeed it uses a couple of endpoints which are entirely independent of the rest of the api (modulo m.login.token login), so I've actually stuck them in a separate module.

document CAS login
Following the spirit of "document how it is, not how we wish it was", document
the CAS login bits.

@richvdh richvdh changed the title from document CAS login to document CAS login and m.login.token login Aug 8, 2016

7. The Matrix client receives the login token and passes it to the |/login|_
API.
If successful, the user is redirected back to the client via the redirectUrl given to the

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Backtick on redirectUrl please.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

Sorry, this whole paragraph should have gone away. It is legacy from before I put more documentation into the endpoints; this behaviour in particular is described under the /login/cas/ticket endpoint.

API.
If successful, the user is redirected back to the client via the redirectUrl given to the
homeserver on the initial redirect request. In the url will be a loginToken query parameter

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Be more specific please. There are many URLs flying around for CAS/OAuth logins, which URL are you talking about? Also, backtick on loginToken please.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

as above

If successful, the user is redirected back to the client via the redirectUrl given to the
homeserver on the initial redirect request. In the url will be a loginToken query parameter
which contains a Matrix login token which the client should then use to complete the login
via the Token-based process described above.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Link to the "Token-based process" please.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

as above

1. The Matrix client instructs the user's browser to navigate to the
|/login/cas/redirect|_ endpoint on the user's homeserver.
2. The homeserver responds with an HTTP redirect to the CAS user interface,

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

I don't know the intricacies of CAS. I do know OAuth2 though, and both are virtually identical if you rename some query string parameters. In OAuth2, you can supply a state query parameter which is a randomly generated string used to protect against CSRF.

  • Does CAS have a similar parameter?
  • If it does, are we using it?
  • If we aren't using it, why not?

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

I don't know. What you see here is my attempt to document what I've managed to understand about CAS from reverse-engineering it from the synapse and vector code while trying to fix what I assumed I had broken (though it turned out that Erik and Dave had broken and I was innocent...) I am not purporting to be a CAS expert - I've just written down what is currently happening.

That said, could CSRF protection be achieved by attaching a random string as a query-param to the redirectUrl? The client would get the string back at step 6, and could check it before using the using the login token at step 7. Would presumably require some use of cookies to stash the random string while we go to the CAS site.

In other words, we can make it an entirely client-side problem. I'm somewhat reluctant to recommend this system without testing it, and and somewhat reluctant to go even deeper into a CAS rabbit-hole I never meant to get into in the first place. WDYT?

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

A TODO marker would suffice if you don't want to go down the rabbit hole now.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

done.

description: The login type being used. Currently only "m.login.password" is supported.
description: |-
The login type being used. One of ``m.login.password`` or
``m.login.token``.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

IIRC the templating system will read out enum properties and automatically set the "One of" blurb for you. Maybe we should use that instead?

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

done.

URI to which the user will be redirected after the homeserver has
authenticated the user with CAS.
required: true
responses:

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

What if the redirectUrl is flibble? Do we validate this at all? If so, we need to document the error codes.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

We do not. If it is flibble then the client will get a redirect to flibble?loginToken=xyz.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

I see. I guess we either leave this hanging for people reading the spec to figure out on their own, or we patch Synapse and add it to the spec. If we were writing the spec as it "should" be rather than as it actually is, we could document the error codes without having to patch Synapse, but oh well. Patch please :)

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

sorry, I'm not quite clear what you want me to do. I'm not keen to do any more patching of synapse than I already have; and it's not clear to me that lack of validation on the redirectUrl is a critical problem. I've added a TODO to the 'server behaviour' section for now.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

It's not critical, but it's a shame that because you're not keen to patch Synapse (and I'm totally okay with that btw) we can't specify error responses. That is all I was saying.

Location:
type: "string"
x-example: |-
redirectUrl=https://client.example.com/?hs=https%3A%2F%2Fserver.example.com&loginToken=secrettoken

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Woah woah woah! What's the deal with this ?hs= parameter? I don't see any docs on it anywhere. What does it do?

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

Argh; it's just an example of a query-param which might have been provided in the original redirectUrl on the call to /login/cas/redirect. It was supposed to match the example there. Now updated (and the spurious redirectUrl= removed).

Central Authentication Service (CAS) is a web-based single sign-on protocol.
Client behaviour

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

No "Server behaviour" section at all? It seems highly unlikely that there is nothing we want to say about server implementations wrt to CAS. How about:

  • How many bits of entropy should the login token have? We just say it should be a token and provide no guidance as to how secure this should be.
  • We don't say anything about where the service= param is coming from. In Synapse I think this is from the config file (which only the admin has control over). The API shape we have doesn't support selecting >1 service per HS. Having something about this parameter would be nice.
  • We don't say anything about what the user_id should be for the logged in user. We may not actually do anything special (just accept client input in the original request), but we should say something about this because the /proxyValidate will return the CAS username of the client who just authed. Can we use that? Can we not?

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

No "Server behaviour" section at all? It seems highly unlikely that there is nothing we want to say about server implementations wrt to CAS.

I have to say that I find the structure of the modules, with the API endpoints listed under "client behaviour", difficult to work with. The API endpoints themselves necessarily document a bunch of server behaviour.

Still, you raise good points. I've added a section which attempts to address them

How about:

How many bits of entropy should the login token have? We just say it should be a token and provide no guidance as to how secure this should be.

. I've made it clear where it is used, that it should be a macaroon, and that it should have a limited lifetime.

We don't say anything about where the service= param is coming from. In Synapse I think this is from the config file (which only the admin has control over). The API shape we have doesn't support selecting >1 service per HS. Having something about this parameter would be nice.

I've attempted to clarify this

We don't say anything about what the user_id should be for the logged in user. We may not actually do anything special (just accept client input in the original request), but we should say something about this because the /proxyValidate will return the CAS username of the client who just authed. Can we use that? Can we not?

Again, I've attempted to clarify this.

PR feedback
Fix some typos, and clarify several aspects of server behaviour.

@richvdh richvdh referenced this pull request Aug 9, 2016

Closed

Document CAS auth #171

More PR feedback
Add a couple of TODO sections
If validation is successful, the server must generate a Matrix login
token. It must then respond with an HTTP redirect to the URI given in
the ``redirectUrl`` parameter, adding a ``loginToken`` query parameter

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Nit: Elsewhere you refer to it as query-parameter but here it is query parameter. Choose one format please (I personally would say it is query parameter but equally I don't care so long as we are consistent).

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

replaced with query parameter, hopefully.

It might be nice if the server did some validation of the ``redirectUrl``
parameter, so that we could give more meaningful errors in the case of
faulty/poorly-configured clients.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

I care more that we will allow HTTP (not S) redirects due to our lack of validating the URL rather than catching obvious typos.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

updated the TODO

should be configured by the server administrator.
Once the ticket has been validated, the server MUST map the CAS ``user_id``
to a valid `Matrix user idenitifier <../intro.html#user-identifiers>`_. The

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

idenitifier

This comment has been minimized.

@richvdh
{
"type": "m.login.token",
"token": "<login token>"

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Can you please confirm that this request will NOT check for a user key (as with m.login.password) and so the server MUST know what user localpart to assign to this login request?

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

correct. The token is a macaroon, with a user_id caveat.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

I've updated the m.login.token section of the user-interactive API to reflect this difference too (I don't actually think that synapse implements m.login.token for the user-interactive API, but that's a separate problem.)

.. code:: json
{
"type": "m.login.token",

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

What happens if the token is invalid? Expired? Same error codes or different error codes?

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

currently, synapse returns a 401 with M_UNKNOWN_TOKEN (in both cases), but I'm not sure we should specify that.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

We document what Synapse does, right? So even if it's suboptimal, we should I guess :/

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

No, I don't buy that. We document what synapse does so that people can write clients.

There is no need for a client to depend on this behaviour, and documenting the broken behaviour only encourages client writers to do so. By leaving it unspecified we leave the door open wider for us to fix it.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

As said in #matrix-core - this flies in the face of agreements made at the offsite, which you summarised in #286 (comment) - We should document what Synapse does, warts and all.

<../intro.html#mapping-from-other-character-sets>`_ may be useful.
If the generated user identifier represents a new user, it should be registered
as a new user.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Do we restrict the ability for a password to be specified for this user ID in the future? We should be clear either way.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

should we? It's up to the server as far as I'm concerned.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

I ask because internally we actually rely on Synapse not allowing a password to be specified, to force them to use CAS login every time they want to log in. This means that if your CAS access is revoked, your access to the HS is also revoked (modulo existing access tokens). If we allow a password to be set however, this is no longer the case, and you can log in even without CAS access.

as a new user.
Finally, the server should generate a short-term login token. The generated
token should be a macaroon, suitable for use with the ``m.login.token`` type of

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Are we saying this should be a macaroon because Reasons or because we actually have a good reason to, like because we inspect some caveats? If we do inspect caveats, you should mention which caveats. If we don't inspect caveats, well, whatever (I personally don't like saying they should be macaroons for seemingly no good reason).

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

correct. The token is a macaroon, with a user_id caveat.

This partly answers this for me. We should mention that servers should do this.

This comment has been minimized.

@richvdh

richvdh Aug 9, 2016

Member

I think the important phrase here is "suitable for use with ...". The best place for documenting the format of those tokens is (currently) where they are used, not where they are generated.

This comment has been minimized.

@Kegsay

Kegsay Aug 9, 2016

Contributor

Sounds good to me.

richvdh added some commits Aug 9, 2016

@Kegsay

This comment has been minimized.

Contributor

Kegsay commented Aug 9, 2016

LGTM

@richvdh richvdh merged commit e910a39 into master Aug 9, 2016

2 checks passed

Docs (Commit) Build #1747 succeeded in 25 sec
Details
Docs (Merged PR) Build finished.
Details

@richvdh richvdh deleted the rav/document_cas_login branch Aug 9, 2016

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