[Pki-devel] TPS REST interface design

[Ade Lee]

On Fri, 2013-06-28 at 23:57 -0500, Endi Sukma Dewata wrote:
> On 6/28/2013 9:36 AM, Ade Lee wrote:
> >>> Or is your intention to send in something with an "action" parameter -
> >>> in which case, the correct operation is a POST?
> >>
> >> I use 'action' in the modify token operation because I couldn't find a
> >> better replacement for the 'question' parameter in the original
> >> op=do_token. Ideally it should be called 'status' but there's already
> >> another attribute with that name. Any suggestion? This 'action' should
> >> actually be returned by the GET operation too. I'll add it after we
> >> finalize the name.
> >
> > do_token?question=X is essentially an operation where you are attempting
> > to set the state of the token resource - for example, from "active" to
> > "lost".  There is no need for an additional parameter.
> 
> I added a Change Token State operation:
> http://pki.fedoraproject.org/wiki/TPS_REST_Interface#Change_Token_State
> 
> As discussed, for now we'll call this parameter 'next state', which will 
> update the status & reason depending on the current state of the token.
> 
> >>> 3.  Should we be worried about a XSS attack here for modifying the state
> >>> of the token?  My guess is that we need to take advantage of the nonce
> >>> mechanism, in which case, token state modification will require two
> >>> steps.
> >>
> >> Yes, but similar to our discussion in the past, nonces or ETags can be
> >> added in a later phase without changing the interface. Since the
> >> modification is a PUT operation, and the request attributes are obtained
> >> using a GET operation done earlier, this is a two-step operation.
> >> Without the nonce or ETag the GET operation will be optional, and the
> >> client can construct the request from scratch. But later the PUT
> >> operation can require a nonce or ETag from a previous GET operation.
> >
> > OK, please put the nonce in the design.  We dont want to forget it.
> 
> I added two sections related to this:
> http://pki.fedoraproject.org/wiki/TPS_REST_Interface#Concurrency_Control
> http://pki.fedoraproject.org/wiki/TPS_REST_Interface#Vulnerabilities
> 
> >>> 4. You should also note that we will be sending back BAD_REQUEST (401?)
> >>> when the token state transition is not permitted.
> >>
> >> That should fall under "Normal application errors will return HTTP 4xx
> >> return code." We can add more details as we implement it.
> >
> > Right, I'm just pointing this out because in this case, there is a state
> > machine which determines which operations are successful.  From the
> > point of view of the API, doing a token transition is just an attempt to
> > set the state of the token differently, but unlike something simple like
> > just changing an attribute value, the operation is subject to the
> > results of a state machine.
> 
> I noted that in the response of Change Token State:
> http://pki.fedoraproject.org/wiki/TPS_REST_Interface#Change_Token_State
> 
> >>> 5. In the response to PUT /tokens/X, it is not necessary to return the
> >>> state of the token.  Rather, you should return a URL pointer to the
> >>> newly modified resource.  The same comment applies to the other PUT
> >>> operations as well.
> >>
> >> Since PUT operation in general doesn't create a new resource, and also
> >> the request is sent to the resource URL being modified, there is no need
> >> to return a new location because it will be identical. In this design
> >> the new location will only be returned on POST operations when adding a
> >> new resource, which in this case the new resource location will be
> >> different from the POST target (the resource collection).
> >>
> > Agreed.  But in PUT /tokens/X, you return state information.  This is
> > not necessary.  You should probably return nothing - other than status.
> 
> See explanation in #10.
> 
> >>> 6. Same question about XSS for add/remove token/ add/remove user.
> >>
> >> Similarly, nonces or ETags can be added later. As described earlier,
> >> modifying users is a two-step process too with GET and PUT. Add and
> >> remove operations do not require the client to do any other operation,
> >> but it can be made so without changing the interface. The DELETE can
> >> require a GET, but I'm not sure what operation should be required before
> >> add.
> >>
> > Yeah - we need to think about ADD.  Being able to add a privileged user
> > for example by XSS would be troubling.
> 
> I think we're mixing up CSRF and XSS. Nonces will be useful to prevent 
> CSRF. XSS can be prevented by encoding/escaping the values.
> 
Thats correct.  I was thinking about CSRF.

> If we implement a single-page Web UI (like IPA Web UI) the client can 
> obtain a nonce during login, then the UI will keep the nonce in memory 
> throughout the session and use it for all update requests including add 
> (the nonce should really be called CSRF token). CSRF attack usually will 
> open a new page, so it will not be able to know the nonce.
> 
> If we implement a multi-page Web UI (like the current Dogtag UI), the 
> add operation can be done in 2 steps. The first page will generate the 
> nonce, then the second page will execute the operation.
> 
> >>> 9. Is there a mapping between Profile and Profile Mapping operations and
> >>> the old operations?  Please put that in so we can see whether we have
> >>> complete coverage.  In particular, I do not see an operation to
> >>> approve/enable a profile.  This is important because we have Common
> >>> Criteria requirements that two users are involved in the approval of a
> >>> profile.  In our case, IIRC we implemented it such that an admin can
> >>> configure a profile but an agent must approve it.
> >>
> >> There should be, but they are not in the docs and I have not had a
> >> chance to test all possible operations in the UI. Once I get the TPS
> >> instance back running I'll continue this. But in general we can
> >> implement something similar to the cert requests:
> >>
> >>     /tps/rest/profiles/{id}/approve (all approvers will use the same URL)
> >>     /tps/rest/profiles/{id}/enable
> >>
> > Thats fine - lets make sure we get those operations in.
> 
> I added the missing mappings. I also added a Change Profile State operation:
> http://pki.fedoraproject.org/wiki/TPS_REST_Interface#Change_Profile_State
> 
The way this is written, it looks like you are just doing a POST to
the /tps/rest/profiles/<ProfileID> and passing in "action" as a
parameter.  Thats not very RESTful at all.

I think we want:
/tps/rest/profiles/{id}/{action}

where {action} is approve etc.  This is also consistent with how we have
done this for cert-requests etc. as well.

> >>> 10.  In POST /tps/rest/profilemappings, you return the location of the
> >>> profile mappings as well as the contents of the profile mapping
> >>> resource.  You should just return the location.  In fact, its probably
> >>> better to just return locations in general.  The client would then use
> >>> GET to fetch the details if needed.  This comment applies to many of the
> >>> resources.
> >>
> >> In general I'd rather return the resource attributes in the POST
> >> response than requiring the client to do a separate GET later. This
> >> response acts as a receipt/acknowledgement for the add request, and it
> >> will return the actual attributes stored on the server before any other
> >> operation is done on it. This is rather important because sometimes a
> >> lazy UI won't do an extra GET because it assumes the attributes don't
> >> change, where in fact they could be normalized, ignored, or generated by
> >> the server. It also prevents a possible (although unlikely) attack that
> >> inserts an operation between the POST and GET.
> >
> > I guess you are suggesting the same for PUT requests as above.
> > I'm not sure whether this is vaid reasoning or not.  Just because you
> > see what has been posted (or PUT) in the server does not mean that this
> > data has not been changed the next time the data is accessed.
> 
> There's never any guarantee that the data we get (either from PUT, POST, 
> or even GET) has not been changed when we submit it for another update 
> operation. The advantage of returning the data in PUT/POST response is 
> we're pushing the updated data to the client's feet.
> 
> > If anything, returning this data encourages lazy clients.
> 
> On the contrary, by returning the data in the PUT/POST response the 
> client will have little reason not to use it. If the client has no 
> intention to get the new data anyway (either from PUT/POST response or 
> by doing another GET) there's nothing we can do about it. But if the 
> client wants to get the new data, using PUT/POST response is a much 
> cheaper option.
> 
> Compare the following code:
> 
>      user = users.update(user);
> 
> with this:
> 
>      users.update(user);
>      user = users.get(user.getId());
> 
> > The fact is
> > though that we will have to implement nonces and possibly etags, and so
> > GETs will become mandatory for any changes.
> 
> Not necessarily. ETag is only an optimistic lock; it's not a guarantee 
> that the data won't change. The PUT/POST response will also provide 
> ETag. Also see comment #6, we may be able to reuse the nonce.
> 
> If there's nobody else updating the same data, we could reuse the 
> PUT/POST response and the ETag that came with it to make subsequent 
> update. So we're still using a two-step update process, but the 2nd step 
> of the first update may become the 1st step of the second update. See 
> also the Concurrency Control section in the wiki page.
> 
> Imagine this, you updated a user, then you went to do some other 
> operations. Then you came back to this user and edited it again. If 
> there's nobody else changing the user, then the new data and ETag that 
> you got from the first update would still be valid. But if first update 
> happened a long time ago, the client may decide according to the caching 
> policy that the data has expired and then issue a GET to get a newer 
> data and ETag.
> 

OK - I like the idea that the etag returned by the PUT/POST can be used
for subsequent updates.  We can go with this for now.

> >>> 11. In the PUT /tps/rest/config, we have requirements for having than
> >>> one user to approve changes.  Admins make changes and agents approve
> >>> them if I recall correctly.  You should look at the differences between
> >>> the agent and admin pages.
> >>
> >> I'll take a look again after I have the TPS instance back. Is there any
> >> documents or diagrams showing the workflow of all approval processes in
> >> TPS UI? Thanks.
> >>
> > The CC docs are probably the best bet for that (other than looking at
> > the code).
> 
> The only approval process I saw in the UI is for profiles, not for 
> general config. Could you point me to a specific page in the docs?
> 
Yeah - I think thats right actually.  All of the stuff thats in config
did not require two agent approval.

> Thanks.
>