Re: [Pki-devel] TPS REST interface design

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. 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 >>> 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. >>> 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? Thanks.